Read additional gifs and make transparency handling safer

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

@@ -165,6 +165,11 @@ internal sealed class GifDecoderCore : IImageDecoderInternals
             this.globalColorTable?.Dispose();
         }
 
+        if (image is null)
+        {
+            GifThrowHelper.ThrowNoData();
+        }
+
         return image;
     }
 
@@ -218,6 +223,11 @@ internal sealed class GifDecoderCore : IImageDecoderInternals
             this.globalColorTable?.Dispose();
         }
 
+        if (this.logicalScreenDescriptor.Width == 0 && this.logicalScreenDescriptor.Height == 0)
+        {
+            GifThrowHelper.ThrowNoHeader();
+        }
+
         return new ImageInfo(
             new PixelTypeInfo(this.logicalScreenDescriptor.BitsPerPixel),
             this.logicalScreenDescriptor.Width,
@@ -281,40 +291,44 @@ internal sealed class GifDecoderCore : IImageDecoderInternals
 
         // If the length is 11 then it's a valid extension and most likely
         // a NETSCAPE, XMP or ANIMEXTS extension. We want the loop count from this.
+        long position = this.stream.Position;
         if (appLength == GifConstants.ApplicationBlockSize)
         {
             this.stream.Read(this.buffer, 0, GifConstants.ApplicationBlockSize);
             bool isXmp = this.buffer.AsSpan().StartsWith(GifConstants.XmpApplicationIdentificationBytes);
-
             if (isXmp && !this.skipMetadata)
             {
-                var extension = GifXmpApplicationExtension.Read(this.stream, this.memoryAllocator);
+                GifXmpApplicationExtension extension = GifXmpApplicationExtension.Read(this.stream, this.memoryAllocator);
                 if (extension.Data.Length > 0)
                 {
                     this.metadata.XmpProfile = new XmpProfile(extension.Data);
                 }
+                else
+                {
+                    // Reset the stream position and continue.
+                    this.stream.Position = position;
+                    this.SkipBlock(appLength);
+                }
 
                 return;
             }
-            else
-            {
-                int subBlockSize = this.stream.ReadByte();
 
-                // TODO: There's also a NETSCAPE buffer extension.
-                // http://www.vurdalakov.net/misc/gif/netscape-buffering-application-extension
-                if (subBlockSize == GifConstants.NetscapeLoopingSubBlockSize)
-                {
-                    this.stream.Read(this.buffer, 0, GifConstants.NetscapeLoopingSubBlockSize);
-                    this.gifMetadata.RepeatCount = GifNetscapeLoopingApplicationExtension.Parse(this.buffer.AsSpan(1)).RepeatCount;
-                    this.stream.Skip(1); // Skip the terminator.
-                    return;
-                }
+            int subBlockSize = this.stream.ReadByte();
 
-                // Could be something else not supported yet.
-                // Skip the subblock and terminator.
-                this.SkipBlock(subBlockSize);
+            // TODO: There's also a NETSCAPE buffer extension.
+            // http://www.vurdalakov.net/misc/gif/netscape-buffering-application-extension
+            if (subBlockSize == GifConstants.NetscapeLoopingSubBlockSize)
+            {
+                this.stream.Read(this.buffer, 0, GifConstants.NetscapeLoopingSubBlockSize);
+                this.gifMetadata.RepeatCount = GifNetscapeLoopingApplicationExtension.Parse(this.buffer.AsSpan(1)).RepeatCount;
+                this.stream.Skip(1); // Skip the terminator.
+                return;
             }
 
+            // Could be something else not supported yet.
+            // Skip the subblock and terminator.
+            this.SkipBlock(subBlockSize);
+
             return;
         }
 
@@ -500,7 +514,7 @@ internal sealed class GifDecoderCore : IImageDecoderInternals
         int descriptorBottom = descriptorTop + descriptor.Height;
         int descriptorLeft = descriptor.Left;
         int descriptorRight = descriptorLeft + descriptor.Width;
-        byte transIndex = this.graphicsControlExtension.TransparencyIndex;
+        byte transparentIndex = this.graphicsControlExtension.TransparencyIndex;
         int colorTableMaxIdx = colorTable.Length - 1;
 
         for (int y = descriptorTop; y < descriptorBottom && y < imageHeight; y++)
@@ -558,8 +572,10 @@ internal sealed class GifDecoderCore : IImageDecoderInternals
             {
                 for (int x = descriptorLeft; x < descriptorRight && x < imageWidth; x++)
                 {
-                    int index = Numerics.Clamp(Unsafe.Add(ref indicesRowRef, x - descriptorLeft), 0, Math.Max(transIndex, colorTableMaxIdx));
-                    if (transIndex != index)
+                    int rawIndex = Unsafe.Add(ref indicesRowRef, x - descriptorLeft);
+                    int transIndex = Numerics.Clamp(rawIndex, 0, Math.Max(transparentIndex, colorTableMaxIdx));
+                    int index = Numerics.Clamp(rawIndex, 0, colorTableMaxIdx);
+                    if (transparentIndex != transIndex)
                     {
                         ref TPixel pixel = ref Unsafe.Add(ref rowRef, x);
                         Rgb24 rgb = colorTable[index];
@@ -649,7 +665,7 @@ internal sealed class GifDecoderCore : IImageDecoderInternals
         this.stream.Skip(6);
         this.ReadLogicalScreenDescriptor();
 
-        var meta = new ImageMetadata();
+        ImageMetadata meta = new();
 
         // The Pixel Aspect Ratio is defined to be the quotient of the pixel's
         // width over its height.  The value range in this field allows

src/ImageSharp/Formats/Gif/GifThrowHelper.cs

@@ -1,12 +1,19 @@
 // Copyright (c) Six Labors.
 // Licensed under the Six Labors Split License.
 
+using System.Diagnostics.CodeAnalysis;
+
 namespace SixLabors.ImageSharp.Formats.Gif;
 
 internal static class GifThrowHelper
 {
+    [DoesNotReturn]
     public static void ThrowInvalidImageContentException(string errorMessage)
         => throw new InvalidImageContentException(errorMessage);
 
-    public static void ThrowInvalidImageContentException(string errorMessage, Exception innerException) => throw new InvalidImageContentException(errorMessage, innerException);
+    [DoesNotReturn]
+    public static void ThrowNoHeader() => throw new InvalidImageContentException("Gif image does not contain a Logical Screen Descriptor.");
+
+    [DoesNotReturn]
+    public static void ThrowNoData() => throw new InvalidImageContentException("Unable to read Gif image data");
 }

src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs

@@ -28,7 +28,6 @@ internal readonly struct GifXmpApplicationExtension : IGifExtension
     /// <param name="stream">The stream to read from.</param>
     /// <param name="allocator">The memory allocator.</param>
     /// <returns>The XMP metadata</returns>
-    /// <exception cref="ImageFormatException">Thrown if the XMP block is not properly terminated.</exception>
     public static GifXmpApplicationExtension Read(Stream stream, MemoryAllocator allocator)
     {
         byte[] xmpBytes = ReadXmpData(stream, allocator);

src/ImageSharp/Formats/ImageDecoderUtilities.cs

@@ -68,6 +68,10 @@ internal static class ImageDecoderUtilities
         {
             throw new InvalidImageContentException(decoder.Dimensions, ex);
         }
+        catch (Exception)
+        {
+            throw;
+        }
     }
 
     internal static Image<TPixel> Decode<TPixel>(
@@ -96,6 +100,10 @@ internal static class ImageDecoderUtilities
         {
             throw largeImageExceptionFactory(ex, decoder.Dimensions);
         }
+        catch (Exception)
+        {
+            throw;
+        }
     }
 
     private static InvalidImageContentException DefaultLargeImageExceptionFactory(

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

@@ -203,8 +203,10 @@ public class GifEncoderTests
     }
 
     [Theory]
-    [WithFile(TestImages.Gif.Issues.Issue2288OptionalExtension, PixelTypes.Rgba32)]
-    [WithFile(TestImages.Gif.Issues.Issue2288OptionalExtension2, PixelTypes.Rgba32)]
+    [WithFile(TestImages.Gif.Issues.Issue2288_A, PixelTypes.Rgba32)]
+    [WithFile(TestImages.Gif.Issues.Issue2288_B, PixelTypes.Rgba32)]
+    [WithFile(TestImages.Gif.Issues.Issue2288_C, PixelTypes.Rgba32)]
+    [WithFile(TestImages.Gif.Issues.Issue2288_D, PixelTypes.Rgba32)]
     public void OptionalExtensionsShouldBeHandledProperly<TPixel>(TestImageProvider<TPixel> provider)
         where TPixel : unmanaged, IPixel<TPixel>
     {

tests/ImageSharp.Tests/TestImages.cs

@@ -476,8 +476,10 @@ public static class TestImages
             public const string Issue1962NoColorTable = "Gif/issues/issue1962_tiniest_gif_1st.gif";
             public const string Issue2012EmptyXmp = "Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif";
             public const string Issue2012BadMinCode = "Gif/issues/issue2012_drona1.gif";
-            public const string Issue2288OptionalExtension = "Gif/issues/issue_2288.gif";
-            public const string Issue2288OptionalExtension2 = "Gif/issues/issue_2288_2.gif";
+            public const string Issue2288_A = "Gif/issues/issue_2288.gif";
+            public const string Issue2288_B = "Gif/issues/issue_2288_2.gif";
+            public const string Issue2288_C = "Gif/issues/issue_2288_3.gif";
+            public const string Issue2288_D = "Gif/issues/issue_2288_4.gif";
         }
 
         public static readonly string[] All = { Rings, Giphy, Cheers, Trans, Kumin, Leo, Ratio4x1, Ratio1x4 };

tests/Images/Input/Gif/issues/issue_2288_3.gif

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:d3f0fd68a03e9c1c896e021828f470d9ceeb8f10e1aead230e42e55670520840
+size 958995

tests/Images/Input/Gif/issues/issue_2288_4.gif

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:0ac8bc8677a1eb4e26161e5255a5c651321ff063892f3caec3f95264aee38057
+size 1971226