From 008c41745d18776b0d5dda492cc77cac0d9154db Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 23 Aug 2023 21:02:05 +1000 Subject: [PATCH 1/9] Handle EOF in bit reader when data is bad. --- .../Formats/Jpeg/Components/Decoder/JpegBitReader.cs | 2 +- tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs | 10 ++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + tests/Images/Input/Jpg/issues/Hang_C438A851.jpg | 3 +++ 4 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 tests/Images/Input/Jpg/issues/Hang_C438A851.jpg diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index d80679db69..50188129cb 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -212,7 +212,7 @@ public bool FindNextMarker() private int ReadStream() { int value = this.badData ? 0 : this.stream.ReadByte(); - if (value == -1) + if (value == -1 || this.stream.Position == this.stream.Length) { // We've encountered the end of the file stream which means there's no EOI marker // in the image or the SOS marker has the wrong dimensions set. diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index 80789178d1..c2ab90afc8 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -314,4 +314,14 @@ public void Issue2315_DecodeWorks(TestImageProvider provider) image.DebugSave(provider); image.CompareToOriginal(provider); } + + [Theory] + [WithFile(TestImages.Jpeg.Issues.HangBadScan, PixelTypes.L8)] + public void DecodeHang(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(JpegDecoder.Instance); + Assert.Equal(65503, image.Width); + Assert.Equal(65503, image.Height); + } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 589168f009..26d91cbc08 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -284,6 +284,7 @@ public static class Issues public const string Issue2315_NotEnoughBytes = "Jpg/issues/issue-2315.jpg"; public const string Issue2334_NotEnoughBytesA = "Jpg/issues/issue-2334-a.jpg"; public const string Issue2334_NotEnoughBytesB = "Jpg/issues/issue-2334-b.jpg"; + public const string HangBadScan = "Jpg/issues/Hang_C438A851.jpg"; public static class Fuzz { diff --git a/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg b/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg new file mode 100644 index 0000000000..97ab9ad0fb --- /dev/null +++ b/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:580760756f2e7e3ed0752a4ec53d6b6786a4f005606f3a50878f732b3b2a1bcb +size 413 From e686680fbbec3654c2cf596e01b6f122a7cce5ad Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 23 Aug 2023 21:02:34 +1000 Subject: [PATCH 2/9] Allow parallel processing of multi-megapixel image --- src/ImageSharp/Advanced/ParallelRowIterator.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Advanced/ParallelRowIterator.cs b/src/ImageSharp/Advanced/ParallelRowIterator.cs index 0eb5952a63..4b00d93d6c 100644 --- a/src/ImageSharp/Advanced/ParallelRowIterator.cs +++ b/src/ImageSharp/Advanced/ParallelRowIterator.cs @@ -50,7 +50,7 @@ public static void IterateRows( int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); // Avoid TPL overhead in this trivial case: @@ -270,7 +270,7 @@ public static void IterateRowIntervals( } [MethodImpl(InliningOptions.ShortMethod)] - private static int DivideCeil(int dividend, int divisor) => 1 + ((dividend - 1) / divisor); + private static int DivideCeil(long dividend, int divisor) => (int)Math.Min(1 + ((dividend - 1) / divisor), int.MaxValue); private static void ValidateRectangle(Rectangle rectangle) { From 62aaa4df64c46b5f35ff4e18229d70aa49afcb92 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 23 Aug 2023 21:21:59 +1000 Subject: [PATCH 3/9] Stream seek can exceed the length of a stream --- src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index 50188129cb..4a1e43f026 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -212,7 +212,7 @@ public bool FindNextMarker() private int ReadStream() { int value = this.badData ? 0 : this.stream.ReadByte(); - if (value == -1 || this.stream.Position == this.stream.Length) + if (value == -1 || this.stream.Position >= this.stream.Length) { // We've encountered the end of the file stream which means there's no EOI marker // in the image or the SOS marker has the wrong dimensions set. From 51de8595c9ba593ca20eceaf087eeb0db906cc92 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 23 Aug 2023 21:39:01 +1000 Subject: [PATCH 4/9] Try triggering on release branches --- .github/workflows/build-and-test.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index b5cc5daca2..853cad738b 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -9,6 +9,7 @@ on: pull_request: branches: - main + - release/* types: [ labeled, opened, synchronize, reopened ] jobs: Build: From dc018fab5ca77874cacbeca19dab49208484c911 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 23 Aug 2023 22:56:45 +1000 Subject: [PATCH 5/9] Update JpegBitReader.cs --- src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index 4a1e43f026..7d278c1fb2 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -212,7 +212,7 @@ public bool FindNextMarker() private int ReadStream() { int value = this.badData ? 0 : this.stream.ReadByte(); - if (value == -1 || this.stream.Position >= this.stream.Length) + if (value == -1 || (this.badData && this.data == 0 && this.stream.Position >= this.stream.Length)) { // We've encountered the end of the file stream which means there's no EOI marker // in the image or the SOS marker has the wrong dimensions set. From c129720ae5bbdafb876a0395b6679c77b8d3fa07 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 24 Aug 2023 13:59:25 +1000 Subject: [PATCH 6/9] Skin on Win .NET 6 --- tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index c2ab90afc8..dc82a5ce4e 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -320,6 +320,14 @@ public void Issue2315_DecodeWorks(TestImageProvider provider) public void DecodeHang(TestImageProvider provider) where TPixel : unmanaged, IPixel { + if (TestEnvironment.IsWindows && + TestEnvironment.RunsOnCI && + TestEnvironment.NetCoreVersion.Major == 6) + { + // Windows CI runs on .NET 6 consistently fail with OOM. + return; + } + using Image image = provider.GetImage(JpegDecoder.Instance); Assert.Equal(65503, image.Width); Assert.Equal(65503, image.Height); From 1499213024907f1d35327f8a922863abc7eb65bd Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 24 Aug 2023 16:57:46 +1000 Subject: [PATCH 7/9] All Win OS is an issue --- tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index dc82a5ce4e..e24568ee02 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -321,10 +321,9 @@ public void DecodeHang(TestImageProvider provider) where TPixel : unmanaged, IPixel { if (TestEnvironment.IsWindows && - TestEnvironment.RunsOnCI && - TestEnvironment.NetCoreVersion.Major == 6) + TestEnvironment.RunsOnCI) { - // Windows CI runs on .NET 6 consistently fail with OOM. + // Windows CI runs consistently fail with OOM. return; } From e96f1fed56825e86ed64ac5a67a6f4d056ee9673 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 28 Aug 2023 20:48:15 +1000 Subject: [PATCH 8/9] Address feedback --- .../Advanced/ParallelRowIterator.cs | 6 ++--- .../Jpeg/Components/Decoder/JpegBitReader.cs | 5 ++++ .../Helpers/ParallelRowIteratorTests.cs | 27 +++++++++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/ImageSharp/Advanced/ParallelRowIterator.cs b/src/ImageSharp/Advanced/ParallelRowIterator.cs index 4b00d93d6c..657654a84b 100644 --- a/src/ImageSharp/Advanced/ParallelRowIterator.cs +++ b/src/ImageSharp/Advanced/ParallelRowIterator.cs @@ -115,7 +115,7 @@ public static void IterateRows( int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); MemoryAllocator allocator = parallelSettings.MemoryAllocator; int bufferLength = Unsafe.AsRef(operation).GetRequiredBufferLength(rectangle); @@ -180,7 +180,7 @@ public static void IterateRowIntervals( int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); // Avoid TPL overhead in this trivial case: @@ -242,7 +242,7 @@ public static void IterateRowIntervals( int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); MemoryAllocator allocator = parallelSettings.MemoryAllocator; int bufferLength = Unsafe.AsRef(operation).GetRequiredBufferLength(rectangle); diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index 7d278c1fb2..0877dbc922 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -212,6 +212,11 @@ public bool FindNextMarker() private int ReadStream() { int value = this.badData ? 0 : this.stream.ReadByte(); + + // We've encountered the end of the file stream which means there's no EOI marker or the marker has been read + // during decoding of the SOS marker. + // When reading individual bits 'badData' simply means we have hit a marker, When data is '0' and the stream is exhausted + // we know we have hit the EOI and completed decoding the scan buffer. if (value == -1 || (this.badData && this.data == 0 && this.stream.Position >= this.stream.Length)) { // We've encountered the end of the file stream which means there's no EOI marker diff --git a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs index 1700b4a734..5fcaa2a5bb 100644 --- a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs +++ b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs @@ -2,6 +2,7 @@ // Licensed under the Six Labors Split License. using System.Numerics; +using Castle.Core.Configuration; using SixLabors.ImageSharp.Advanced; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -406,6 +407,32 @@ void RowAction(RowInterval rows, Span memory) Assert.Contains(width <= 0 ? "Width" : "Height", ex.Message); } + [Fact] + public void CanIterateWithoutIntOverflow() + { + ParallelExecutionSettings parallelSettings = ParallelExecutionSettings.FromConfiguration(Configuration.Default); + + Rectangle rect = new(0, 0, 65535, 65535); + + static void RowAction(RowInterval rows, Span memory) + { + } + + TestRowOperation operation = default; + TestRowIntervalOperation intervalOperation = new(RowAction); + + ParallelRowIterator.IterateRows(Configuration.Default, rect, in operation); + + ParallelRowIterator.IterateRowIntervals, Rgba32>(rect, in parallelSettings, in intervalOperation); + } + + private readonly struct TestRowOperation : IRowOperation + { + public void Invoke(int y) + { + } + } + private readonly struct TestRowIntervalOperation : IRowIntervalOperation { private readonly Action action; From 1bb07cb3777baad0dc663e09bd87198ae45b0080 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Tue, 29 Aug 2023 20:24:41 +0200 Subject: [PATCH 9/9] add validation to CanIterateWithoutIntOverflow --- .../Helpers/ParallelRowIteratorTests.cs | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs index 5fcaa2a5bb..d393850d6b 100644 --- a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs +++ b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs @@ -2,6 +2,7 @@ // Licensed under the Six Labors Split License. using System.Numerics; +using System.Runtime.CompilerServices; using Castle.Core.Configuration; using SixLabors.ImageSharp.Advanced; using SixLabors.ImageSharp.Memory; @@ -411,25 +412,36 @@ void RowAction(RowInterval rows, Span memory) public void CanIterateWithoutIntOverflow() { ParallelExecutionSettings parallelSettings = ParallelExecutionSettings.FromConfiguration(Configuration.Default); + const int max = 100_000; - Rectangle rect = new(0, 0, 65535, 65535); + Rectangle rect = new(0, 0, max, max); + int intervalMaxY = 0; + void RowAction(RowInterval rows, Span memory) => intervalMaxY = Math.Max(rows.Max, intervalMaxY); - static void RowAction(RowInterval rows, Span memory) - { - } - - TestRowOperation operation = default; + TestRowOperation operation = new(); TestRowIntervalOperation intervalOperation = new(RowAction); ParallelRowIterator.IterateRows(Configuration.Default, rect, in operation); + Assert.Equal(max - 1, operation.MaxY.Value); ParallelRowIterator.IterateRowIntervals, Rgba32>(rect, in parallelSettings, in intervalOperation); + Assert.Equal(max, intervalMaxY); } private readonly struct TestRowOperation : IRowOperation { + public TestRowOperation() + { + } + + public StrongBox MaxY { get; } = new StrongBox(); + public void Invoke(int y) { + lock (this.MaxY) + { + this.MaxY.Value = Math.Max(y, this.MaxY.Value); + } } }