Merge pull request #770 from carbon/gif-bug

Fix infinite loop when a GIF prematurely terminates

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.
         }
 
         /// <summary>
-        /// Skips the designated number of bytes in the stream.
+        /// Skips over a block or reads its terminator.
+        /// <param name="blockSize">The length of the block to skip.</param>
         /// </summary>
-        /// <param name="length">The number of bytes to skip.</param>
-        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
             {

tests/ImageSharp.Sandbox46/ImageSharp.Sandbox46.csproj

@@ -11,7 +11,7 @@
     <Authors>James Jackson-South and contributors</Authors>
     <Company>James Jackson-South</Company>
     <RootNamespace>SixLabors.ImageSharp.Sandbox46</RootNamespace>
-    <LangVersion>7.2</LangVersion>
+    <LangVersion>7.3</LangVersion>
   </PropertyGroup>
   <ItemGroup>
     <ProjectReference Include="..\..\src\ImageSharp.Drawing\ImageSharp.Drawing.csproj" />

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<Rgba32> image = decoder.Decode<Rgba32>(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)