From ed3860cfda28f3087f962e4871bec50ce103b7d4 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 30 Aug 2023 12:01:07 +1000 Subject: [PATCH 1/2] Handle EOF in Jpeg bit reader when data is bad to prevent DOS attack. (#2516) * Handle EOF in bit reader when data is bad. * Allow parallel processing of multi-megapixel image * Stream seek can exceed the length of a stream * Try triggering on release branches * Update JpegBitReader.cs * Skin on Win .NET 6 * All Win OS is an issue * Address feedback * add validation to CanIterateWithoutIntOverflow --------- Co-authored-by: antonfirsov --- .github/workflows/build-and-test.yml | 1 + .../Advanced/ParallelRowIterator.cs | 10 ++--- .../Jpeg/Components/Decoder/JpegBitReader.cs | 7 +++- .../Formats/Jpg/JpegDecoderTests.cs | 17 ++++++++ .../Helpers/ParallelRowIteratorTests.cs | 39 +++++++++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + .../Images/Input/Jpg/issues/Hang_C438A851.jpg | 3 ++ 7 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 tests/Images/Input/Jpg/issues/Hang_C438A851.jpg diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index b5cc5daca..853cad738 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: diff --git a/src/ImageSharp/Advanced/ParallelRowIterator.cs b/src/ImageSharp/Advanced/ParallelRowIterator.cs index 0eb5952a6..657654a84 100644 --- a/src/ImageSharp/Advanced/ParallelRowIterator.cs +++ b/src/ImageSharp/Advanced/ParallelRowIterator.cs @@ -50,7 +50,7 @@ public static partial class ParallelRowIterator 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: @@ -115,7 +115,7 @@ public static partial class ParallelRowIterator 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 partial class ParallelRowIterator 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 partial class ParallelRowIterator 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); @@ -270,7 +270,7 @@ public static partial class ParallelRowIterator } [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) { diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index d80679db6..0877dbc92 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -212,7 +212,12 @@ internal struct JpegBitReader private int ReadStream() { int value = this.badData ? 0 : this.stream.ReadByte(); - if (value == -1) + + // 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 // 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 594e7ebe7..eaa9f82cb 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -325,4 +325,21 @@ public partial class JpegDecoderTests image.DebugSave(provider); image.CompareToOriginal(provider); } + + [Theory] + [WithFile(TestImages.Jpeg.Issues.HangBadScan, PixelTypes.L8)] + public void DecodeHang(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + if (TestEnvironment.IsWindows && + TestEnvironment.RunsOnCI) + { + // Windows CI runs consistently fail with OOM. + return; + } + + using Image image = provider.GetImage(JpegDecoder.Instance); + Assert.Equal(65503, image.Width); + Assert.Equal(65503, image.Height); + } } diff --git a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs index 1700b4a73..d393850d6 100644 --- a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs +++ b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs @@ -2,6 +2,8 @@ // 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; using SixLabors.ImageSharp.PixelFormats; @@ -406,6 +408,43 @@ public class ParallelRowIteratorTests Assert.Contains(width <= 0 ? "Width" : "Height", ex.Message); } + [Fact] + public void CanIterateWithoutIntOverflow() + { + ParallelExecutionSettings parallelSettings = ParallelExecutionSettings.FromConfiguration(Configuration.Default); + const int max = 100_000; + + Rectangle rect = new(0, 0, max, max); + int intervalMaxY = 0; + void RowAction(RowInterval rows, Span memory) => intervalMaxY = Math.Max(rows.Max, intervalMaxY); + + 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); + } + } + } + private readonly struct TestRowIntervalOperation : IRowIntervalOperation { private readonly Action action; diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index a25424b6d..afde66ffa 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -291,6 +291,7 @@ public static class TestImages 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 Issue2478_JFXX = "Jpg/issues/issue-2478-jfxx.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 000000000..97ab9ad0f --- /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 54b7e04f7a3c2921af3c769bd6c27fd3d5156f04 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Sat, 2 Sep 2023 13:04:03 +0200 Subject: [PATCH 2/2] Fix #2518 (#2519) * OilPaint benchmark * fix #2518 * Update OilPaintingProcessor{TPixel}.cs * clamp the vector to 0..1 and undo buffer overallocation * throw ImageProcessingException instead of clamping --------- Co-authored-by: James Jackson-South --- .../Effects/OilPaintingProcessor{TPixel}.cs | 47 +++++++++++-------- .../Processing/OilPaint.cs | 19 ++++++++ .../Processors/Effects/OilPaintTest.cs | 13 +++-- 3 files changed, 55 insertions(+), 24 deletions(-) create mode 100644 tests/ImageSharp.Benchmarks/Processing/OilPaint.cs diff --git a/src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs b/src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs index 6352230de..1491fe073 100644 --- a/src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs +++ b/src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs @@ -4,6 +4,7 @@ using System.Buffers; using System.Numerics; using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; using SixLabors.ImageSharp.Advanced; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -34,17 +35,25 @@ internal class OilPaintingProcessor : ImageProcessor /// protected override void OnFrameApply(ImageFrame source) { + int levels = Math.Clamp(this.definition.Levels, 1, 255); int brushSize = Math.Clamp(this.definition.BrushSize, 1, Math.Min(source.Width, source.Height)); using Buffer2D targetPixels = this.Configuration.MemoryAllocator.Allocate2D(source.Size()); source.CopyTo(targetPixels); - RowIntervalOperation operation = new(this.SourceRectangle, targetPixels, source.PixelBuffer, this.Configuration, brushSize >> 1, this.definition.Levels); - ParallelRowIterator.IterateRowIntervals( + RowIntervalOperation operation = new(this.SourceRectangle, targetPixels, source.PixelBuffer, this.Configuration, brushSize >> 1, levels); + try + { + ParallelRowIterator.IterateRowIntervals( this.Configuration, this.SourceRectangle, in operation); + } + catch (Exception ex) + { + throw new ImageProcessingException("The OilPaintProcessor failed. The most likely reason is that a pixel component was outside of its' allowed range.", ex); + } Buffer2D.SwapOrCopyContent(source.PixelBuffer, targetPixels); } @@ -105,18 +114,18 @@ internal class OilPaintingProcessor : ImageProcessor Span targetRowVector4Span = targetRowBuffer.Memory.Span; Span targetRowAreaVector4Span = targetRowVector4Span.Slice(this.bounds.X, this.bounds.Width); - ref float binsRef = ref bins.GetReference(); - ref int intensityBinRef = ref Unsafe.As(ref binsRef); - ref float redBinRef = ref Unsafe.Add(ref binsRef, (uint)this.levels); - ref float blueBinRef = ref Unsafe.Add(ref redBinRef, (uint)this.levels); - ref float greenBinRef = ref Unsafe.Add(ref blueBinRef, (uint)this.levels); + Span binsSpan = bins.GetSpan(); + Span intensityBinsSpan = MemoryMarshal.Cast(binsSpan); + Span redBinSpan = binsSpan[this.levels..]; + Span blueBinSpan = redBinSpan[this.levels..]; + Span greenBinSpan = blueBinSpan[this.levels..]; for (int y = rows.Min; y < rows.Max; y++) { Span sourceRowPixelSpan = this.source.DangerousGetRowSpan(y); Span sourceRowAreaPixelSpan = sourceRowPixelSpan.Slice(this.bounds.X, this.bounds.Width); - PixelOperations.Instance.ToVector4(this.configuration, sourceRowAreaPixelSpan, sourceRowAreaVector4Span); + PixelOperations.Instance.ToVector4(this.configuration, sourceRowAreaPixelSpan, sourceRowAreaVector4Span, PixelConversionModifiers.Scale); for (int x = this.bounds.X; x < this.bounds.Right; x++) { @@ -140,7 +149,7 @@ internal class OilPaintingProcessor : ImageProcessor int offsetX = x + fxr; offsetX = Numerics.Clamp(offsetX, 0, maxX); - Vector4 vector = sourceOffsetRow[offsetX].ToVector4(); + Vector4 vector = sourceOffsetRow[offsetX].ToScaledVector4(); float sourceRed = vector.X; float sourceBlue = vector.Z; @@ -148,21 +157,21 @@ internal class OilPaintingProcessor : ImageProcessor int currentIntensity = (int)MathF.Round((sourceBlue + sourceGreen + sourceRed) / 3F * (this.levels - 1)); - Unsafe.Add(ref intensityBinRef, (uint)currentIntensity)++; - Unsafe.Add(ref redBinRef, (uint)currentIntensity) += sourceRed; - Unsafe.Add(ref blueBinRef, (uint)currentIntensity) += sourceBlue; - Unsafe.Add(ref greenBinRef, (uint)currentIntensity) += sourceGreen; + intensityBinsSpan[currentIntensity]++; + redBinSpan[currentIntensity] += sourceRed; + blueBinSpan[currentIntensity] += sourceBlue; + greenBinSpan[currentIntensity] += sourceGreen; - if (Unsafe.Add(ref intensityBinRef, (uint)currentIntensity) > maxIntensity) + if (intensityBinsSpan[currentIntensity] > maxIntensity) { - maxIntensity = Unsafe.Add(ref intensityBinRef, (uint)currentIntensity); + maxIntensity = intensityBinsSpan[currentIntensity]; maxIndex = currentIntensity; } } - float red = MathF.Abs(Unsafe.Add(ref redBinRef, (uint)maxIndex) / maxIntensity); - float blue = MathF.Abs(Unsafe.Add(ref blueBinRef, (uint)maxIndex) / maxIntensity); - float green = MathF.Abs(Unsafe.Add(ref greenBinRef, (uint)maxIndex) / maxIntensity); + float red = redBinSpan[maxIndex] / maxIntensity; + float blue = blueBinSpan[maxIndex] / maxIntensity; + float green = greenBinSpan[maxIndex] / maxIntensity; float alpha = sourceRowVector4Span[x].W; targetRowVector4Span[x] = new Vector4(red, green, blue, alpha); @@ -171,7 +180,7 @@ internal class OilPaintingProcessor : ImageProcessor Span targetRowAreaPixelSpan = this.targetPixels.DangerousGetRowSpan(y).Slice(this.bounds.X, this.bounds.Width); - PixelOperations.Instance.FromVector4Destructive(this.configuration, targetRowAreaVector4Span, targetRowAreaPixelSpan); + PixelOperations.Instance.FromVector4Destructive(this.configuration, targetRowAreaVector4Span, targetRowAreaPixelSpan, PixelConversionModifiers.Scale); } } } diff --git a/tests/ImageSharp.Benchmarks/Processing/OilPaint.cs b/tests/ImageSharp.Benchmarks/Processing/OilPaint.cs new file mode 100644 index 000000000..239d5a93b --- /dev/null +++ b/tests/ImageSharp.Benchmarks/Processing/OilPaint.cs @@ -0,0 +1,19 @@ +// Copyright (c) Six Labors. +// Licensed under the Six Labors Split License. + +using BenchmarkDotNet.Attributes; +using SixLabors.ImageSharp.PixelFormats; +using SixLabors.ImageSharp.Processing; + +namespace SixLabors.ImageSharp.Benchmarks.Processing; + +[Config(typeof(Config.MultiFramework))] +public class OilPaint +{ + [Benchmark] + public void DoOilPaint() + { + using Image image = new Image(1920, 1200, new(127, 191, 255)); + image.Mutate(ctx => ctx.OilPaint()); + } +} diff --git a/tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs b/tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs index 990a97bed..10811a559 100644 --- a/tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs +++ b/tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs @@ -27,8 +27,7 @@ public class OilPaintTest [WithFileCollection(nameof(InputImages), nameof(OilPaintValues), PixelTypes.Rgba32)] public void FullImage(TestImageProvider provider, int levels, int brushSize) where TPixel : unmanaged, IPixel - { - provider.RunValidatingProcessorTest( + => provider.RunValidatingProcessorTest( x => { x.OilPaint(levels, brushSize); @@ -36,17 +35,21 @@ public class OilPaintTest }, ImageComparer.TolerantPercentage(0.01F), appendPixelTypeToFileName: false); - } [Theory] [WithFileCollection(nameof(InputImages), nameof(OilPaintValues), PixelTypes.Rgba32)] [WithTestPatternImages(nameof(OilPaintValues), 100, 100, PixelTypes.Rgba32)] public void InBox(TestImageProvider provider, int levels, int brushSize) where TPixel : unmanaged, IPixel - { - provider.RunRectangleConstrainedValidatingProcessorTest( + => provider.RunRectangleConstrainedValidatingProcessorTest( (x, rect) => x.OilPaint(levels, brushSize, rect), $"{levels}-{brushSize}", ImageComparer.TolerantPercentage(0.01F)); + + [Fact] + public void Issue2518_PixelComponentOutsideOfRange_ThrowsImageProcessingException() + { + using Image image = new(10, 10, new RgbaVector(1, 1, 100)); + Assert.Throws(() => image.Mutate(ctx => ctx.OilPaint())); } }