Merge pull request #3224 from jafin/jf/gif-lzw-and-frame-geometry

Drop GIF frame rows outside the logical screen and stop on LZW codes past the code tables

Author: JimBobSquarePants

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

@@ -175,6 +175,12 @@ protected override Image<TPixel> Decode<TPixel>(BufferedReadStream stream, Cance
                 this.gifMetadata.ColorTableMode = FrameColorTableMode.Local;
             }
         }
+        catch
+        {
+            // Ownership transfers to the caller only after decoding succeeds. Dispose a partially decoded image on failure.
+            image?.Dispose();
+            throw;
+        }
         finally
         {
             this.globalColorTable?.Dispose();
@@ -589,7 +595,15 @@ private bool ReadFrame<TPixel>(
             }
         }
 
-        this.ReadFrameColors(stream, ref image, ref previousFrame, ref previousDisposalMode, colorTable, backgroundColor.ToPixel<TPixel>());
+        try
+        {
+            this.ReadFrameColors(stream, ref image, ref previousFrame, ref previousDisposalMode, colorTable, backgroundColor.ToPixel<TPixel>());
+        }
+        catch (InvalidImageContentException) when (this.Options.SegmentIntegrityHandling is SegmentIntegrityHandling.IgnoreImageData)
+        {
+            // Keep completed rows. SkipBlock below consumes the rest of this frame so the next descriptor remains readable.
+            // Header and palette parsing stay outside this recovery boundary; structural errors remain fatal.
+        }
 
         // Update from newly decoded frame.
         FrameDisposalMode disposalMethod = this.graphicsControlExtension.DisposalMethod;
@@ -723,12 +737,14 @@ private void ReadFrameColors<TPixel>(
         using IMemoryOwner<byte> indicesRowOwner = this.memoryAllocator.Allocate<byte>(descriptor.Width);
         Span<byte> indicesRow = indicesRowOwner.Memory.Span;
 
+        // A frame that starts at or beyond the right or bottom edge of the logical screen has nothing to draw.
+        // Its image data is then left to the caller, which skips the remaining data sub-blocks.
         int minCodeSize = stream.ReadByte();
-        if (LzwDecoder.IsValidMinCodeSize(minCodeSize))
+        if (descriptorTop < imageHeight && descriptorLeft < imageWidth && LzwDecoder.IsValidMinCodeSize(minCodeSize))
         {
             using LzwDecoder lzwDecoder = new(this.configuration.MemoryAllocator, stream, minCodeSize);
 
-            for (int y = descriptorTop; y < descriptorBottom && y < imageHeight; y++)
+            for (int y = descriptorTop; y < descriptorBottom; y++)
             {
                 // Check if this image is interlaced.
                 int writeY; // the target y offset to write to
@@ -737,7 +753,10 @@ private void ReadFrameColors<TPixel>(
                     // If so then we read lines at predetermined offsets.
                     // When an entire image height worth of offset lines has been read we consider this a pass.
                     // With each pass the number of offset lines changes and the starting line changes.
-                    if (interlaceY >= descriptor.Height)
+                    // A frame shorter than 5 lines has passes without any lines, so keep advancing until a
+                    // pass has a line left. The four passes visit every frame line exactly once and the loop
+                    // runs once per frame line, so a pass with a line left always exists.
+                    while (interlaceY >= descriptor.Height)
                     {
                         interlacePass++;
                         switch (interlacePass)
@@ -756,14 +775,31 @@ private void ReadFrameColors<TPixel>(
                         }
                     }
 
-                    writeY = Math.Min(interlaceY + descriptor.Top, image.Height);
+                    writeY = interlaceY + descriptorTop;
                     interlaceY += interlaceIncrement;
                 }
                 else
                 {
                     writeY = y;
                 }
 
+                // For a properly encoded gif the frame never extends below the logical screen. When it does
+                // the lines below the screen are dropped. Lines only move down within a pass, so once a line
+                // falls below the screen the rest of the pass is hidden too. A sequential frame is a single
+                // pass, so decoding can stop there. Before the last interlace pass, later passes still start
+                // at frame line 1, 2 or 4 and can reach the screen again unless it shows only frame line 0.
+                // The hidden lines in between must still be decoded and discarded to reach those lines.
+                if (writeY >= imageHeight)
+                {
+                    if (!descriptor.InterlaceFlag || interlacePass == 3 || imageHeight - descriptorTop < 2)
+                    {
+                        break;
+                    }
+
+                    lzwDecoder.SkipIndices(descriptor.Width);
+                    continue;
+                }
+
                 lzwDecoder.DecodePixelRow(indicesRow);
 
                 // #403 The left + width value can be larger than the image width
@@ -840,7 +876,15 @@ private bool ReadFrameMetadata(BufferedReadStream stream, List<ImageFrameMetadat
         if (LzwDecoder.IsValidMinCodeSize(minCodeSize))
         {
             using LzwDecoder lzwDecoder = new(this.configuration.MemoryAllocator, stream, minCodeSize);
-            lzwDecoder.SkipIndices(this.imageDescriptor.Width * this.imageDescriptor.Height);
+
+            try
+            {
+                lzwDecoder.SkipIndices(this.imageDescriptor.Width * this.imageDescriptor.Height);
+            }
+            catch (InvalidImageContentException) when (this.Options.SegmentIntegrityHandling is SegmentIntegrityHandling.IgnoreImageData)
+            {
+                // Identify still reads LZW codes. Use the same recovery policy and sub-block skipping as full decoding.
+            }
         }
 
         ImageFrameMetadata currentFrame = new();

src/ImageSharp/Formats/Gif/LzwDecoder.cs

@@ -131,6 +131,7 @@ public static bool IsValidMinCodeSize(int minCodeSize)
     /// Decodes and decompresses all pixel indices for a single row from the stream, assigning the pixel values to the buffer.
     /// </summary>
     /// <param name="indices">The pixel indices array to decode to.</param>
+    /// <exception cref="InvalidImageContentException">The first code addresses an entry outside the LZW code tables.</exception>
     public void DecodePixelRow(Span<byte> indices)
     {
         indices.Clear();
@@ -209,6 +210,14 @@ public void DecodePixelRow(Span<byte> indices)
 
                 if (oldCode == NullCode)
                 {
+                    // This first code has no previous string to extend. With a tolerated minimum size of 12,
+                    // the next available code is 4098, beyond the 4096-entry tables. Report the corrupt data
+                    // before indexing either table; the GIF decoder applies the image data integrity policy.
+                    if (code >= MaxStackSize)
+                    {
+                        GifThrowHelper.ThrowInvalidImageContentException("The GIF LZW code exceeds the code table bounds.");
+                    }
+
                     pixelStack[top++] = suffix[code];
                     oldCode = code;
                     first = code;
@@ -274,6 +283,7 @@ public void DecodePixelRow(Span<byte> indices)
     /// Decodes and decompresses all pixel indices from the stream allowing skipping of the data.
     /// </summary>
     /// <param name="length">The resulting index table length.</param>
+    /// <exception cref="InvalidImageContentException">The first code addresses an entry outside the LZW code tables.</exception>
     public void SkipIndices(int length)
     {
         // Get span values from the owners.
@@ -350,6 +360,14 @@ public void SkipIndices(int length)
 
                 if (oldCode == NullCode)
                 {
+                    // This first code has no previous string to extend. With a tolerated minimum size of 12,
+                    // the next available code is 4098, beyond the 4096-entry tables. Report the corrupt data
+                    // before indexing either table; the GIF decoder applies the image data integrity policy.
+                    if (code >= MaxStackSize)
+                    {
+                        GifThrowHelper.ThrowInvalidImageContentException("The GIF LZW code exceeds the code table bounds.");
+                    }
+
                     pixelStack[top++] = suffix[code];
                     oldCode = code;
                     first = code;

tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs

@@ -24,6 +24,14 @@ public class GifDecoderTests
         TestImages.Gif.Giphy, TestImages.Gif.Kumin
     ];
 
+    /// <summary>
+    /// The four palette colors in the stored frame geometry fixtures.
+    /// </summary>
+    private static readonly Rgba32[] GeometryTestColorTable =
+    [
+        new(255, 0, 0), new(0, 255, 0), new(0, 0, 255), new(255, 255, 255)
+    ];
+
     [Theory]
     [WithFileCollection(nameof(MultiFrameTestFiles), PixelTypes.Rgba32)]
     public void Decode_VerifyAllFrames<TPixel>(TestImageProvider<TPixel> provider)
@@ -442,6 +450,127 @@ public void Decode_MalformedApplicationExtension_ThrowsWithStrict()
         });
     }
 
+    /// <summary>
+    /// Invalid LZW table codes fail unless image data errors are explicitly ignored.
+    /// Recovery retains completed rows and continues to the following animation frame.
+    /// </summary>
+    /// <param name="path">The stored GIF with an invalid first code or an invalid code after a clear.</param>
+    /// <param name="completedRows">The number of valid rows before the invalid code.</param>
+    /// <param name="frameCount">The number of stored animation frames.</param>
+    [Theory]
+    [InlineData(TestImages.Gif.LzwCodeOutsideTable, 0, 1)]
+    [InlineData(TestImages.Gif.LzwCodeOutsideTableHiddenRow, 1, 2)]
+    public void Decode_LzwCodePastCodeTables_RespectsIntegrityHandling(string path, int completedRows, int frameCount)
+    {
+        byte[] data = TestFile.Create(path).Bytes;
+
+        foreach (SegmentIntegrityHandling integrity in Enum.GetValues<SegmentIntegrityHandling>())
+        {
+            DecoderOptions options = new() { SegmentIntegrityHandling = integrity };
+            if (integrity is not SegmentIntegrityHandling.IgnoreImageData)
+            {
+                Assert.Throws<InvalidImageContentException>(() =>
+                {
+                    using Image<Rgba32> image = Image.Load<Rgba32>(options, data);
+                });
+
+                continue;
+            }
+
+            using Image<Rgba32> actual = Image.Load<Rgba32>(options, data);
+            Assert.Equal(new Size(4, frameCount == 1 ? 4 : 2), actual.Size);
+            Assert.Equal(frameCount, actual.Frames.Count);
+
+            for (int y = 0; y < actual.Height; y++)
+            {
+                for (int x = 0; x < actual.Width; x++)
+                {
+                    // A completed row is red. An undecoded row keeps the frame's transparent background.
+                    Assert.Equal(y < completedRows ? new Rgba32(255, 0, 0) : default, actual[x, y]);
+
+                    if (frameCount == 2)
+                    {
+                        // The complete green frame after the corrupt frame proves that sub-block recovery resumed correctly.
+                        Assert.Equal(new Rgba32(0, 255, 0), actual.Frames[1][x, y]);
+                    }
+                }
+            }
+        }
+    }
+
+    /// <summary>
+    /// Identify uses LZW index skipping and must apply the same image data policy as full decoding.
+    /// </summary>
+    /// <param name="path">The stored GIF with an invalid table code.</param>
+    /// <param name="frameCount">The number of stored animation frames.</param>
+    [Theory]
+    [InlineData(TestImages.Gif.LzwCodeOutsideTable, 1)]
+    [InlineData(TestImages.Gif.LzwCodeOutsideTableHiddenRow, 2)]
+    public void Identify_LzwCodePastCodeTables_RespectsIntegrityHandling(string path, int frameCount)
+    {
+        byte[] data = TestFile.Create(path).Bytes;
+
+        foreach (SegmentIntegrityHandling integrity in Enum.GetValues<SegmentIntegrityHandling>())
+        {
+            DecoderOptions options = new() { SegmentIntegrityHandling = integrity };
+            if (integrity is not SegmentIntegrityHandling.IgnoreImageData)
+            {
+                Assert.Throws<InvalidImageContentException>(() => Image.Identify(options, data));
+                continue;
+            }
+
+            ImageInfo info = Image.Identify(options, data);
+            Assert.Equal(new Size(4, frameCount == 1 ? 4 : 2), info.Size);
+            Assert.Equal(frameCount, info.FrameMetadataCollection.Count);
+        }
+    }
+
+    /// <summary>
+    /// Clips stored frame rows to the logical screen without changing interlace order or later frame parsing.
+    /// </summary>
+    /// <param name="path">The stored GIF with a four-color row pattern and a second frame containing one white pixel.</param>
+    /// <param name="width">The first frame's stored width.</param>
+    /// <param name="height">The first frame's stored height.</param>
+    /// <param name="left">The first frame's left offset.</param>
+    /// <param name="top">The first frame's top offset.</param>
+    [Theory]
+    [InlineData(TestImages.Gif.Frame20x20At0x0Interlaced, 20, 20, 0, 0)]
+    [InlineData(TestImages.Gif.Frame20x20At0x0Sequential, 20, 20, 0, 0)]
+    [InlineData(TestImages.Gif.Frame10x10At0x5Interlaced, 10, 10, 0, 5)]
+    [InlineData(TestImages.Gif.Frame10x10At0x5Sequential, 10, 10, 0, 5)]
+    [InlineData(TestImages.Gif.Frame10x10At5x3Interlaced, 10, 10, 5, 3)]
+    [InlineData(TestImages.Gif.Frame10x10At0x10Interlaced, 10, 10, 0, 10)]
+    [InlineData(TestImages.Gif.Frame10x10At0x10Sequential, 10, 10, 0, 10)]
+    [InlineData(TestImages.Gif.Frame10x20At0x8Interlaced, 10, 20, 0, 8)]
+    [InlineData(TestImages.Gif.Frame10x20At0x9Interlaced, 10, 20, 0, 9)]
+    [InlineData(TestImages.Gif.Frame10x1At0x0Interlaced, 10, 1, 0, 0)]
+    [InlineData(TestImages.Gif.Frame10x2At0x0Interlaced, 10, 2, 0, 0)]
+    [InlineData(TestImages.Gif.Frame10x3At0x7Interlaced, 10, 3, 0, 7)]
+    [InlineData(TestImages.Gif.Frame10x4At0x0Interlaced, 10, 4, 0, 0)]
+    [InlineData(TestImages.Gif.Frame4x4At12x0Interlaced, 4, 4, 12, 0)]
+    [InlineData(TestImages.Gif.Frame4x4At12x0Sequential, 4, 4, 12, 0)]
+    public void Decode_FrameExceedingLogicalScreen_WritesOnlyRowsInsideCanvas(string path, int width, int height, int left, int top)
+    {
+        using Image<Rgba32> image = Image.Load<Rgba32>(TestFile.Create(path).Bytes);
+
+        Assert.Equal(new Size(10, 10), image.Size);
+        Assert.Equal(2, image.Frames.Count);
+
+        for (int y = 0; y < image.Height; y++)
+        {
+            for (int x = 0; x < image.Width; x++)
+            {
+                // Frame row n contains palette index n modulo four. Pixels outside its rectangle stay transparent.
+                bool inFrame = x >= left && x < left + width && y >= top && y < top + height;
+                Rgba32 expected = inFrame ? GeometryTestColorTable[(y - top) % 4] : default;
+                Assert.Equal(expected, image[x, y]);
+            }
+        }
+
+        // Even when the first frame is clipped or fully hidden, its data must not consume the next image descriptor.
+        Assert.Equal(new Rgba32(255, 255, 255), image.Frames[1][0, 0]);
+    }
+
     private static byte[] CreateGifWithMalformedApplicationExtension()
     {
         // The application extension declares a 5-byte application block (0x05), but GIF application

tests/ImageSharp.Tests/TestImages.cs

@@ -588,6 +588,23 @@ public static class Bmp
     public static class Gif
     {
         public const string Rings = "Gif/rings.gif";
+        public const string Frame20x20At0x0Interlaced = "Gif/frames/20x20-left0-top0-interlaced.gif";
+        public const string Frame20x20At0x0Sequential = "Gif/frames/20x20-left0-top0-sequential.gif";
+        public const string Frame10x10At0x5Interlaced = "Gif/frames/10x10-left0-top5-interlaced.gif";
+        public const string Frame10x10At0x5Sequential = "Gif/frames/10x10-left0-top5-sequential.gif";
+        public const string Frame10x10At5x3Interlaced = "Gif/frames/10x10-left5-top3-interlaced.gif";
+        public const string Frame10x10At0x10Interlaced = "Gif/frames/10x10-left0-top10-interlaced.gif";
+        public const string Frame10x10At0x10Sequential = "Gif/frames/10x10-left0-top10-sequential.gif";
+        public const string Frame10x20At0x8Interlaced = "Gif/frames/10x20-left0-top8-interlaced.gif";
+        public const string Frame10x20At0x9Interlaced = "Gif/frames/10x20-left0-top9-interlaced.gif";
+        public const string Frame10x1At0x0Interlaced = "Gif/frames/10x1-left0-top0-interlaced.gif";
+        public const string Frame10x2At0x0Interlaced = "Gif/frames/10x2-left0-top0-interlaced.gif";
+        public const string Frame10x3At0x7Interlaced = "Gif/frames/10x3-left0-top7-interlaced.gif";
+        public const string Frame10x4At0x0Interlaced = "Gif/frames/10x4-left0-top0-interlaced.gif";
+        public const string Frame4x4At12x0Interlaced = "Gif/frames/4x4-left12-top0-interlaced.gif";
+        public const string Frame4x4At12x0Sequential = "Gif/frames/4x4-left12-top0-sequential.gif";
+        public const string LzwCodeOutsideTable = "Gif/frames/lzw-code-outside-table.gif";
+        public const string LzwCodeOutsideTableHiddenRow = "Gif/frames/lzw-code-outside-table-hidden-row.gif";
         public const string Giphy = "Gif/giphy.gif";
         public const string Cheers = "Gif/cheers.gif";
         public const string Receipt = "Gif/receipt.gif";