From 93bb37394a484c03ba84d59d60e39f90677240c7 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 9 Jan 2019 01:16:37 +1100 Subject: [PATCH] Use bounds checks in Huffman ctr. Fix #798 --- .../Jpeg/Components/Decoder/HuffmanTable.cs | 23 ++++++++++-------- .../Formats/Jpg/JpegDecoderTests.Baseline.cs | 10 +++++--- tests/ImageSharp.Tests/TestImages.cs | 1 + .../Issue798-AccessViolationException.jpg | Bin 0 -> 381 bytes 4 files changed, 20 insertions(+), 14 deletions(-) create mode 100644 tests/Images/Input/Jpg/issues/Issue798-AccessViolationException.jpg diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanTable.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanTable.cs index 9e134746bc..9e11981b12 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanTable.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanTable.cs @@ -50,8 +50,10 @@ internal unsafe struct HuffmanTable /// The huffman values public HuffmanTable(MemoryAllocator memoryAllocator, ReadOnlySpan codeLengths, ReadOnlySpan values) { - const int Length = 257; - using (IMemoryOwner huffcode = memoryAllocator.Allocate(Length)) + // We do some bounds checks in the code here to protect against AccessViolationExceptions + const int HuffCodeLength = 257; + const int MaxSizeLength = HuffCodeLength - 1; + using (IMemoryOwner huffcode = memoryAllocator.Allocate(HuffCodeLength)) { ref short huffcodeRef = ref MemoryMarshal.GetReference(huffcode.GetSpan()); ref byte codeLengthsRef = ref MemoryMarshal.GetReference(codeLengths); @@ -63,7 +65,7 @@ public HuffmanTable(MemoryAllocator memoryAllocator, ReadOnlySpan codeLeng for (short i = 1; i < 17; i++) { byte length = Unsafe.Add(ref codeLengthsRef, i); - for (short j = 0; j < length; j++) + for (short j = 0; j < length && x < MaxSizeLength; j++) { Unsafe.Add(ref sizesRef, x++) = i; } @@ -84,7 +86,7 @@ public HuffmanTable(MemoryAllocator memoryAllocator, ReadOnlySpan codeLeng Unsafe.Add(ref valOffsetRef, k) = (int)(si - code); if (Unsafe.Add(ref sizesRef, si) == k) { - while (Unsafe.Add(ref sizesRef, si) == k) + while (Unsafe.Add(ref sizesRef, si) == k && si < HuffCodeLength) { Unsafe.Add(ref huffcodeRef, si++) = (short)code++; } @@ -100,19 +102,20 @@ public HuffmanTable(MemoryAllocator memoryAllocator, ReadOnlySpan codeLeng // Generate non-spec lookup tables to speed up decoding. const int FastBits = ScanDecoder.FastBits; - ref byte fastRef = ref this.Lookahead[0]; - Unsafe.InitBlockUnaligned(ref fastRef, 0xFF, 1 << FastBits); // Flag for non-accelerated + ref byte lookaheadRef = ref this.Lookahead[0]; + const uint MaxFastLength = 1 << FastBits; + Unsafe.InitBlockUnaligned(ref lookaheadRef, 0xFF, MaxFastLength); // Flag for non-accelerated for (int i = 0; i < si; i++) { int size = Unsafe.Add(ref sizesRef, i); if (size <= FastBits) { - int c = Unsafe.Add(ref huffcodeRef, i) << (FastBits - size); - int m = 1 << (FastBits - size); - for (int l = 0; l < m; l++) + int huffCode = Unsafe.Add(ref huffcodeRef, i) << (FastBits - size); + int max = 1 << (FastBits - size); + for (int left = 0; left < max; left++) { - Unsafe.Add(ref fastRef, c + l) = (byte)i; + Unsafe.Add(ref lookaheadRef, huffCode + left) = (byte)i; } } } diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Baseline.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Baseline.cs index a2f57e485a..1c9d207cd1 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Baseline.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Baseline.cs @@ -46,9 +46,11 @@ public void DecodeBaselineJpeg_CriticalEOF_ShouldThrow(TestImageProvider [Theory] [WithFile(TestImages.Jpeg.Issues.InvalidJpegThrowsWrongException797, PixelTypes.Rgba32)] public void LoadingImage_InvalidTagLength_ShouldThrow(TestImageProvider provider) - where TPixel : struct, IPixel - { - Assert.Throws(() => provider.GetImage()); - } + where TPixel : struct, IPixel => Assert.Throws(() => provider.GetImage()); + + [Theory] + [WithFile(TestImages.Jpeg.Issues.AccessViolationException798, PixelTypes.Rgba32)] + public void LoadingImage_BadHuffman_ShouldNotThrow(TestImageProvider provider) + where TPixel : struct, IPixel => Assert.NotNull(provider.GetImage()); } } \ No newline at end of file diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 460aceba0a..8d8a32fba3 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -168,6 +168,7 @@ public static class Issues public const string ExifGetString750Transform = "Jpg/issues/issue750-exif-tranform.jpg"; public const string ExifGetString750Load = "Jpg/issues/issue750-exif-load.jpg"; public const string InvalidJpegThrowsWrongException797 = "Jpg/issues/Issue797-InvalidImage.jpg"; + public const string AccessViolationException798 = "Jpg/issues/Issue798-AccessViolationException.jpg"; } public static readonly string[] All = Baseline.All.Concat(Progressive.All).ToArray(); diff --git a/tests/Images/Input/Jpg/issues/Issue798-AccessViolationException.jpg b/tests/Images/Input/Jpg/issues/Issue798-AccessViolationException.jpg new file mode 100644 index 0000000000000000000000000000000000000000..30f61c86305f4533b01e93287ff68ca4f96aa62a GIT binary patch literal 381 zcmex=ez+{VquD8S9pEh;o1zT!kvMP6RT*Y!Im zp7>8302;y=8WmNMS@F85={0eBFjO625aeJ`U{GLYRAOKfWMmd({C|W&oPmLvkpTrT zGBE=sR0S9rm>5`@S(ssxK)#@mqTxozqDjn&Nf(uj0)fiJfIfxk7gYRzi-8Ad9Frik zAcH-_^>3lMeVR)ruPRBLe(zrX9qr9`^CCVn2G+N1%X+iv_wBR4JEKh9Zrw25^y&0A zrjlS8weov&pBq|l4Nu>7XXETip^JL;UhN8(z1nSm_{zs5<*m}|Lnqh1yjRTgpMgL4 UXxsL!Ow;WZukCPQ&;NfD0NVq=R{#J2 literal 0 HcmV?d00001