-
-
Notifications
You must be signed in to change notification settings - Fork 893
Text segment ReadOnlySpan<byte> initialization #1133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
4e8de95
c83d066
aa65526
da79756
4dbec4c
714c960
bb7b041
f907f90
8ad8a59
b251c00
0577690
68387a7
f3f6d9c
5b7ac75
70ed21e
4b0dfd1
719b5a3
99b88c0
b43a66c
cbd1872
20bf5fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,8 +40,6 @@ internal sealed unsafe class DeflaterHuffman : IDisposable | |||||||||||||||||||
| // probability, to avoid transmitting the lengths for unused bit length codes. | ||||||||||||||||||||
| private static readonly int[] BitLengthOrder = { 16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15 }; | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might well be better off using the I know we do that in the jpeg encoder for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 5b7ac75.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't tell you for certain I'm afraid. That port was an act of desperation over understanding.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Should I just switch those accesses to a direct
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we lack understanding, we shall go for safety. Btw if we use the data in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get what you're saying now, yeah 👍 ImageSharp/src/ImageSharp/Formats/Png/Zlib/DeflaterHuffman.cs Lines 384 to 392 in 4b0dfd1
All the indexing values there are
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh sorry, I was wrong the values for We should deal with Guard however to make sure we don't regress. Do you plan to fix it globally in SixLabors/SharedInfrastructure, or will you replace it in this PR with a manual check + ThrowHelper calls?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahahah no problem, glad we could figure this out, I was wondering what was I missing there 😄 As for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you can do it, that would be the best!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, always happy to bring more of my discoveries and optimizations to ImageSharp! 😄 |
||||||||||||||||||||
|
|
||||||||||||||||||||
| private static readonly byte[] Bit4Reverse = { 0, 8, 4, 12, 2, 10, 6, 14, 1, 9, 5, 13, 3, 11, 7, 15 }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private static readonly short[] StaticLCodes; | ||||||||||||||||||||
| private static readonly byte[] StaticLLength; | ||||||||||||||||||||
| private static readonly short[] StaticDCodes; | ||||||||||||||||||||
|
|
@@ -128,6 +126,8 @@ public DeflaterHuffman(MemoryAllocator memoryAllocator) | |||||||||||||||||||
| this.pinnedLiteralBuffer = (short*)this.literalBufferHandle.Pointer; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private static ReadOnlySpan<byte> Bit4Reverse => new byte[] { 0, 8, 4, 12, 2, 10, 6, 14, 1, 9, 5, 13, 3, 11, 7, 15 }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||
| /// Gets the pending buffer to use. | ||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||
|
|
@@ -363,10 +363,17 @@ public bool TallyDist(int distance, int length) | |||||||||||||||||||
| [MethodImpl(InliningOptions.ShortMethod)] | ||||||||||||||||||||
| public static short BitReverse(int toReverse) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| return (short)(Bit4Reverse[toReverse & 0xF] << 12 | ||||||||||||||||||||
| | Bit4Reverse[(toReverse >> 4) & 0xF] << 8 | ||||||||||||||||||||
| | Bit4Reverse[(toReverse >> 8) & 0xF] << 4 | ||||||||||||||||||||
| | Bit4Reverse[toReverse >> 12]); | ||||||||||||||||||||
| DebugGuard.MustBeLessThan(toReverse & 0xF, Bit4Reverse.Length, nameof(toReverse)); | ||||||||||||||||||||
| DebugGuard.MustBeLessThan((toReverse >> 4) & 0xF, Bit4Reverse.Length, nameof(toReverse)); | ||||||||||||||||||||
| DebugGuard.MustBeLessThan((toReverse >> 8) & 0xF, Bit4Reverse.Length, nameof(toReverse)); | ||||||||||||||||||||
| DebugGuard.MustBeLessThan(toReverse >> 12, Bit4Reverse.Length, nameof(toReverse)); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ref byte bit4ReverseRef = ref MemoryMarshal.GetReference(Bit4Reverse); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return (short)(Unsafe.Add(ref bit4ReverseRef, toReverse & 0xF) << 12 | ||||||||||||||||||||
| | Unsafe.Add(ref bit4ReverseRef, (toReverse >> 4) & 0xF) << 8 | ||||||||||||||||||||
| | Unsafe.Add(ref bit4ReverseRef, (toReverse >> 8) & 0xF) << 4 | ||||||||||||||||||||
| | Unsafe.Add(ref bit4ReverseRef, toReverse >> 12)); | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DebugGuard won't save us in production. Where does
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I mostly added those to validate the code path in our CI tests. I had not considered possible attempts to exploit this from a security standpoint, you're right 😅 What if we figured out the upper/lower bounds of those 4 indices, and validated just those? It would still result in just 2 conditional jumps, instead of 4 (one for each access). Actually, now that I think about it:
That's still just a single branch compared to 4, so... Yay? 😁
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, let's do it like that!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome, done in 0577690. Also added a detailed comment for future reference 😊
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My comment on
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 4b0dfd1. Also I should really stress out how if we care about micro-optimizations, we should really consider refactoring our
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well this is just terrible, wish we have noticed that earlier. If you have some time, feel free to file a PR in SixLabors/SharedInfrastructure. For the comparison methods I'd rather see a set of duplicate non-generic methods, than |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <inheritdoc/> | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,26 @@ | ||
| // Copyright (c) Six Labors and contributors. | ||
| // Copyright (c) Six Labors and contributors. | ||
| // Licensed under the Apache License, Version 2.0. | ||
|
|
||
| using System; | ||
|
|
||
| namespace SixLabors.ImageSharp.Metadata.Profiles.Exif | ||
| { | ||
| internal static class ExifConstants | ||
| { | ||
| public static readonly byte[] LittleEndianByteOrderMarker = | ||
| public static ReadOnlySpan<byte> LittleEndianByteOrderMarker => new byte[] | ||
| { | ||
| (byte)'I', | ||
| (byte)'I', | ||
| 0x2A, | ||
| 0x00, | ||
| }; | ||
|
|
||
| public static readonly byte[] BigEndianByteOrderMarker = | ||
| public static ReadOnlySpan<byte> BigEndianByteOrderMarker => new byte[] | ||
| { | ||
| (byte)'M', | ||
| (byte)'M', | ||
| 0x00, | ||
| 0x2A | ||
| }; | ||
| } | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.