From 0337b1ac5193c95d9f4275e72594123dc9e55201 Mon Sep 17 00:00:00 2001 From: Jason Nelson Date: Mon, 5 Nov 2018 19:21:16 -0800 Subject: [PATCH 1/5] Fix infinate loop when a GIF prematurely terminates in Skip --- src/ImageSharp/Formats/Gif/GifDecoderCore.cs | 29 ++++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs index b8a270b3f5..bfa91416aa 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -142,8 +142,7 @@ namespace SixLabors.ImageSharp.Formats.Gif this.ReadApplicationExtension(); break; case GifConstants.PlainTextLabel: - int plainLength = stream.ReadByte(); - this.Skip(plainLength); // Not supported by any known decoder. + this.SkipBlock(); // Not supported by any known decoder. break; } } @@ -190,9 +189,7 @@ namespace SixLabors.ImageSharp.Formats.Gif switch (stream.ReadByte()) { case GifConstants.GraphicControlLabel: - - // Skip graphic control extension block - this.Skip(0); + this.SkipBlock(); // Skip graphic control extension block break; case GifConstants.CommentLabel: this.ReadComments(); @@ -201,8 +198,7 @@ namespace SixLabors.ImageSharp.Formats.Gif this.ReadApplicationExtension(); break; case GifConstants.PlainTextLabel: - int plainLength = stream.ReadByte(); - this.Skip(plainLength); // Not supported by any known decoder. + this.SkipBlock(); // Not supported by any known decoder. break; } } @@ -288,24 +284,27 @@ namespace SixLabors.ImageSharp.Formats.Gif // Could be XMP or something else not supported yet. // Back up and skip. this.stream.Position -= appLength + 1; - this.Skip(appLength); + this.SkipBlock(appLength); return; } - this.Skip(appLength); // Not supported by any known decoder. + this.SkipBlock(appLength); // Not supported by any known decoder. } /// - /// Skips the designated number of bytes in the stream. + /// Skips over a block or reads its terminator. + /// The length of the block to skip. /// - /// The number of bytes to skip. - private void Skip(int length) + private void SkipBlock(int blockSize = 0) { - this.stream.Skip(length); + if (blockSize > 0) + { + this.stream.Skip(blockSize); + } int flag; - while ((flag = this.stream.ReadByte()) != 0) + while ((flag = this.stream.ReadByte()) > 0) { this.stream.Skip(flag); } @@ -370,7 +369,7 @@ namespace SixLabors.ImageSharp.Formats.Gif this.ReadFrameColors(ref image, ref previousFrame, indices.GetSpan(), colorTable, this.imageDescriptor); // Skip any remaining blocks - this.Skip(0); + this.SkipBlock(); } finally { From 0ee2a8ac4bd95489ec00f03c6ea39916a5186bad Mon Sep 17 00:00:00 2001 From: Jason Nelson Date: Tue, 6 Nov 2018 08:40:08 -0800 Subject: [PATCH 2/5] Add test coverage for #770 --- .../ImageSharp.Sandbox46.csproj | 2 +- .../Formats/Gif/GifDecoderTests.cs | 33 +++++++++++++++---- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/tests/ImageSharp.Sandbox46/ImageSharp.Sandbox46.csproj b/tests/ImageSharp.Sandbox46/ImageSharp.Sandbox46.csproj index bb559b70dd..4d7b7de759 100644 --- a/tests/ImageSharp.Sandbox46/ImageSharp.Sandbox46.csproj +++ b/tests/ImageSharp.Sandbox46/ImageSharp.Sandbox46.csproj @@ -11,7 +11,7 @@ James Jackson-South and contributors James Jackson-South SixLabors.ImageSharp.Sandbox46 - 7.2 + 7.3 diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs index 6d2a74c03b..5bfbb058be 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -1,20 +1,20 @@ // Copyright (c) Six Labors and contributors. // Licensed under the Apache License, Version 2.0. +using System; +using System.Collections.Generic; +using System.IO; using System.Text; +using SixLabors.ImageSharp.Advanced; using SixLabors.ImageSharp.Formats.Gif; +using SixLabors.ImageSharp.MetaData; using SixLabors.ImageSharp.PixelFormats; +using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison; using Xunit; -using System.IO; -using SixLabors.ImageSharp.Advanced; // ReSharper disable InconsistentNaming namespace SixLabors.ImageSharp.Tests.Formats.Gif { - using System.Collections.Generic; - using SixLabors.ImageSharp.MetaData; - using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison; - public class GifDecoderTests { private const PixelTypes TestPixelTypes = PixelTypes.Rgba32 | PixelTypes.RgbaVector | PixelTypes.Argb32; @@ -70,6 +70,27 @@ namespace SixLabors.ImageSharp.Tests.Formats.Gif } } + [Fact] + public unsafe void Decode_NonTerminatedFinalFrame() + { + var testFile = TestFile.Create(TestImages.Gif.Rings); + + int length = testFile.Bytes.Length - 2; + + fixed (byte* data = testFile.Bytes.AsSpan(0, length)) + { + using (var stream = new UnmanagedMemoryStream(data, length)) + { + var decoder = new GifDecoder(); + + using (Image image = decoder.Decode(Configuration.Default, stream)) + { + Assert.Equal((200, 200), (image.Width, image.Height)); + } + } + } + } + [Theory] [MemberData(nameof(RatioFiles))] public void Decode_VerifyRatio(string imagePath, int xResolution, int yResolution, PixelResolutionUnit resolutionUnit) From c2409c6fe57955fe6a3b1c4051202e45e361bce7 Mon Sep 17 00:00:00 2001 From: Peter Amrehn Date: Sun, 4 Nov 2018 14:47:17 +0100 Subject: [PATCH 3/5] simplify BoxBlurProcessor kernel initialization old code seemed to be a relict from the GaussianBlurProcessor where this pattern made sense. Here each value of the matrix is just 1.0/size as a result, and we can leverage DenseMatrix.Fill() for that. --- .../Convolution/BoxBlurProcessor.cs | 31 +------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/src/ImageSharp/Processing/Processors/Convolution/BoxBlurProcessor.cs b/src/ImageSharp/Processing/Processors/Convolution/BoxBlurProcessor.cs index 0ec62ac3d4..38dc638b90 100644 --- a/src/ImageSharp/Processing/Processors/Convolution/BoxBlurProcessor.cs +++ b/src/ImageSharp/Processing/Processors/Convolution/BoxBlurProcessor.cs @@ -66,36 +66,7 @@ namespace SixLabors.ImageSharp.Processing.Processors.Convolution ? new DenseMatrix(size, 1) : new DenseMatrix(1, size); - float sum = 0F; - for (int i = 0; i < size; i++) - { - float x = 1; - sum += x; - if (horizontal) - { - kernel[0, i] = x; - } - else - { - kernel[i, 0] = x; - } - } - - // Normalize kernel so that the sum of all weights equals 1 - if (horizontal) - { - for (int i = 0; i < size; i++) - { - kernel[0, i] = kernel[0, i] / sum; - } - } - else - { - for (int i = 0; i < size; i++) - { - kernel[i, 0] = kernel[i, 0] / sum; - } - } + kernel.Fill(1.0F / size); return kernel; } From 434a5ee4317824696a909182afd21e91e5d53444 Mon Sep 17 00:00:00 2001 From: Peter Amrehn Date: Sun, 4 Nov 2018 14:38:33 +0100 Subject: [PATCH 4/5] fix typo in KirschKernels class name --- .../{KirshKernels.cs => KirschKernels.cs} | 4 ++-- .../Processors/Convolution/KirschProcessor.cs | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) rename src/ImageSharp/Processing/Processors/Convolution/{KirshKernels.cs => KirschKernels.cs} (96%) diff --git a/src/ImageSharp/Processing/Processors/Convolution/KirshKernels.cs b/src/ImageSharp/Processing/Processors/Convolution/KirschKernels.cs similarity index 96% rename from src/ImageSharp/Processing/Processors/Convolution/KirshKernels.cs rename to src/ImageSharp/Processing/Processors/Convolution/KirschKernels.cs index d315875089..86232e306a 100644 --- a/src/ImageSharp/Processing/Processors/Convolution/KirshKernels.cs +++ b/src/ImageSharp/Processing/Processors/Convolution/KirschKernels.cs @@ -6,9 +6,9 @@ using SixLabors.ImageSharp.Primitives; namespace SixLabors.ImageSharp.Processing.Processors.Convolution { /// - /// Contains the eight matrices used for Kirsh edge detection + /// Contains the eight matrices used for Kirsch edge detection /// - internal static class KirshKernels + internal static class KirschKernels { /// /// Gets the North gradient operator diff --git a/src/ImageSharp/Processing/Processors/Convolution/KirschProcessor.cs b/src/ImageSharp/Processing/Processors/Convolution/KirschProcessor.cs index 46cf00c226..c3188676f3 100644 --- a/src/ImageSharp/Processing/Processors/Convolution/KirschProcessor.cs +++ b/src/ImageSharp/Processing/Processors/Convolution/KirschProcessor.cs @@ -23,27 +23,27 @@ namespace SixLabors.ImageSharp.Processing.Processors.Convolution } /// - public override DenseMatrix North => KirshKernels.KirschNorth; + public override DenseMatrix North => KirschKernels.KirschNorth; /// - public override DenseMatrix NorthWest => KirshKernels.KirschNorthWest; + public override DenseMatrix NorthWest => KirschKernels.KirschNorthWest; /// - public override DenseMatrix West => KirshKernels.KirschWest; + public override DenseMatrix West => KirschKernels.KirschWest; /// - public override DenseMatrix SouthWest => KirshKernels.KirschSouthWest; + public override DenseMatrix SouthWest => KirschKernels.KirschSouthWest; /// - public override DenseMatrix South => KirshKernels.KirschSouth; + public override DenseMatrix South => KirschKernels.KirschSouth; /// - public override DenseMatrix SouthEast => KirshKernels.KirschSouthEast; + public override DenseMatrix SouthEast => KirschKernels.KirschSouthEast; /// - public override DenseMatrix East => KirshKernels.KirschEast; + public override DenseMatrix East => KirschKernels.KirschEast; /// - public override DenseMatrix NorthEast => KirshKernels.KirschNorthEast; + public override DenseMatrix NorthEast => KirschKernels.KirschNorthEast; } } \ No newline at end of file From cf71cf5c534f2a53b0011da8f4412328c90d4cdf Mon Sep 17 00:00:00 2001 From: Peter Amrehn Date: Sun, 4 Nov 2018 15:13:24 +0100 Subject: [PATCH 5/5] fix typo in local variable name --- .../Profiles/ICC/DataReader/IccDataReader.Curves.cs | 8 ++++---- .../ICC/DataReader/IccDataReader.TagDataEntry.cs | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.Curves.cs b/src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.Curves.cs index ee91ad7a18..b27083dc49 100644 --- a/src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.Curves.cs +++ b/src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.Curves.cs @@ -41,10 +41,10 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Icc public IccResponseCurve ReadResponseCurve(int channelCount) { var type = (IccCurveMeasurementEncodings)this.ReadUInt32(); - uint[] measurment = new uint[channelCount]; + uint[] measurement = new uint[channelCount]; for (int i = 0; i < channelCount; i++) { - measurment[i] = this.ReadUInt32(); + measurement[i] = this.ReadUInt32(); } Vector3[] xyzValues = new Vector3[channelCount]; @@ -56,8 +56,8 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Icc IccResponseNumber[][] response = new IccResponseNumber[channelCount][]; for (int i = 0; i < channelCount; i++) { - response[i] = new IccResponseNumber[measurment[i]]; - for (uint j = 0; j < measurment[i]; j++) + response[i] = new IccResponseNumber[measurement[i]]; + for (uint j = 0; j < measurement[i]; j++) { response[i][j] = this.ReadResponseNumber(); } diff --git a/src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.TagDataEntry.cs b/src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.TagDataEntry.cs index e41d9b3b8c..c572b7f210 100644 --- a/src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.TagDataEntry.cs +++ b/src/ImageSharp/MetaData/Profiles/ICC/DataReader/IccDataReader.TagDataEntry.cs @@ -628,16 +628,16 @@ namespace SixLabors.ImageSharp.MetaData.Profiles.Icc { int start = this.currentIndex - 8; // 8 is the tag header size ushort channelCount = this.ReadUInt16(); - ushort measurmentCount = this.ReadUInt16(); + ushort measurementCount = this.ReadUInt16(); - uint[] offset = new uint[measurmentCount]; - for (int i = 0; i < measurmentCount; i++) + uint[] offset = new uint[measurementCount]; + for (int i = 0; i < measurementCount; i++) { offset[i] = this.ReadUInt32(); } - var curves = new IccResponseCurve[measurmentCount]; - for (int i = 0; i < measurmentCount; i++) + var curves = new IccResponseCurve[measurementCount]; + for (int i = 0; i < measurementCount; i++) { this.currentIndex = (int)(start + offset[i]); curves[i] = this.ReadResponseCurve(channelCount);