Gif decoder. Fix memory access violations.

src/ImageSharp/Common/Extensions/StreamExtensions.cs

@@ -13,7 +13,7 @@ namespace SixLabors.ImageSharp
     internal static class StreamExtensions
     {
         /// <summary>
-        /// Writes data from a stream into the provided buffer.
+        /// Writes data from a stream from the provided buffer.
         /// </summary>
         /// <param name="stream">The stream.</param>
         /// <param name="buffer">The buffer.</param>

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

@@ -265,10 +265,14 @@ private void ReadApplicationExtension()
                 this.stream.Read(this.buffer, 0, GifConstants.ApplicationBlockSize);
                 bool isXmp = this.buffer.AsSpan().StartsWith(GifConstants.XmpApplicationIdentificationBytes);
 
-                if (isXmp)
+                if (isXmp && !this.IgnoreMetadata)
                 {
-                    var extension = GifXmpApplicationExtension.Read(this.stream);
-                    this.metadata.XmpProfile = new XmpProfile(extension.Data);
+                    var extension = GifXmpApplicationExtension.Read(this.stream, this.MemoryAllocator);
+                    if (extension.Data.Length > 0)
+                    {
+                        this.metadata.XmpProfile = new XmpProfile(extension.Data);
+                    }
+
                     return;
                 }
                 else

src/ImageSharp/Formats/Gif/GifEncoderCore.cs

@@ -123,7 +123,8 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
             this.WriteComments(gifMetadata, stream);
 
             // Write application extensions.
-            this.WriteApplicationExtensions(stream, image.Frames.Count, gifMetadata.RepeatCount, metadata.XmpProfile);
+            XmpProfile xmpProfile = image.Metadata.XmpProfile ?? image.Frames.RootFrame.Metadata.XmpProfile;
+            this.WriteApplicationExtensions(stream, image.Frames.Count, gifMetadata.RepeatCount, xmpProfile);
 
             if (useGlobalTable)
             {
@@ -137,7 +138,6 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
             // Clean up.
             quantized.Dispose();
 
-            // TODO: Write extension etc
             stream.WriteByte(GifConstants.EndIntroducer);
         }
 

src/ImageSharp/Formats/Gif/LzwDecoder.cs

@@ -68,7 +68,7 @@ public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream)
         /// <param name="pixels">The pixel array to decode to.</param>
         public void DecodePixels(int dataSize, Buffer2D<byte> pixels)
         {
-            Guard.MustBeLessThan(dataSize, int.MaxValue, nameof(dataSize));
+            Guard.MustBeLessThanOrEqualTo(1 << dataSize, MaxStackSize, nameof(dataSize));
 
             // The resulting index table length.
             int width = pixels.Width;

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

@@ -2,9 +2,9 @@
 // Licensed under the Apache License, Version 2.0.
 
 using System;
-using System.Collections.Generic;
 using System.IO;
-using SixLabors.ImageSharp.Metadata.Profiles.Xmp;
+using SixLabors.ImageSharp.IO;
+using SixLabors.ImageSharp.Memory;
 
 namespace SixLabors.ImageSharp.Formats.Gif
 {
@@ -14,7 +14,10 @@ namespace SixLabors.ImageSharp.Formats.Gif
 
         public byte Label => GifConstants.ApplicationExtensionLabel;
 
-        public int ContentLength => this.Data.Length + 269; // 12 + Data Length + 1 + 256
+        // size          : 1
+        // identifier    : 11
+        // magic trailer : 257
+        public int ContentLength => this.Data.Length + 269;
 
         /// <summary>
         /// Gets the raw Data.
@@ -25,40 +28,23 @@ namespace SixLabors.ImageSharp.Formats.Gif
         /// Reads the XMP metadata from the specified stream.
         /// </summary>
         /// <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)
+        public static GifXmpApplicationExtension Read(Stream stream, MemoryAllocator allocator)
         {
-            // Read data in blocks, until an \0 character is encountered.
-            // We overshoot, indicated by the terminatorIndex variable.
-            const int bufferSize = 256;
-            var list = new List<byte[]>();
-            int terminationIndex = -1;
-            while (terminationIndex < 0)
-            {
-                byte[] temp = new byte[bufferSize];
-                int bytesRead = stream.Read(temp);
-                list.Add(temp);
-                terminationIndex = Array.IndexOf(temp, (byte)1);
-            }
+            byte[] xmpBytes = ReadXmpData(stream, allocator);
 
-            // Pack all the blocks (except magic trailer) into one single array again.
-            int dataSize = ((list.Count - 1) * bufferSize) + terminationIndex;
-            byte[] buffer = new byte[dataSize];
-            Span<byte> bufferSpan = buffer;
-            int pos = 0;
-            for (int j = 0; j < list.Count - 1; j++)
+            // Exclude the "magic trailer", see XMP Specification Part 3, 1.1.2 GIF
+            int xmpLength = xmpBytes.Length - 256; // 257 - unread 0x0
+            byte[] buffer = Array.Empty<byte>();
+            if (xmpLength > 0)
             {
-                list[j].CopyTo(bufferSpan.Slice(pos));
-                pos += bufferSize;
+                buffer = new byte[xmpLength];
+                xmpBytes.AsSpan(0, xmpLength).CopyTo(buffer);
+                stream.Skip(1); // Skip the terminator.
             }
 
-            // Last one only needs the portion until terminationIndex copied over.
-            Span<byte> lastBytes = list[list.Count - 1];
-            lastBytes.Slice(0, terminationIndex).CopyTo(bufferSpan.Slice(pos));
-
-            // Skip the remainder of the magic trailer.
-            stream.Skip(258 - (bufferSize - terminationIndex));
             return new GifXmpApplicationExtension(buffer);
         }
 
@@ -67,7 +53,7 @@ public int WriteTo(Span<byte> buffer)
             int totalSize = this.ContentLength;
             if (buffer.Length < totalSize)
             {
-                throw new InsufficientMemoryException("Unable to write XMP metadata to GIF image");
+                ThrowInsufficientMemory();
             }
 
             int bytesWritten = 0;
@@ -93,5 +79,29 @@ public int WriteTo(Span<byte> buffer)
 
             return totalSize;
         }
+
+        private static byte[] ReadXmpData(Stream stream, MemoryAllocator allocator)
+        {
+            using ChunkedMemoryStream bytes = new(allocator);
+
+            // XMP data doesn't have a fixed length nor is there an indicator of the length.
+            // So we simply read one byte at a time until we hit the 0x0 value at the end
+            // of the magic trailer or the end of the stream.
+            // Using ChunkedMemoryStream reduces the array resize allocation normally associated
+            // with writing from a non fixed-size buffer.
+            while (true)
+            {
+                int b = stream.ReadByte();
+                if (b <= 0)
+                {
+                    return bytes.ToArray();
+                }
+
+                bytes.WriteByte((byte)b);
+            }
+        }
+
+        private static void ThrowInsufficientMemory()
+            => throw new InsufficientMemoryException("Unable to write XMP metadata to GIF image");
     }
 }

src/ImageSharp/Metadata/ImageMetadata.cs

@@ -68,6 +68,7 @@ private ImageMetadata(ImageMetadata other)
             this.ExifProfile = other.ExifProfile?.DeepClone();
             this.IccProfile = other.IccProfile?.DeepClone();
             this.IptcProfile = other.IptcProfile?.DeepClone();
+            this.XmpProfile = other.XmpProfile?.DeepClone();
         }
 
         /// <summary>
@@ -175,7 +176,7 @@ public TFormatMetadata GetFormatMetadata<TFormatMetadata>(IImageFormat<TFormatMe
         }
 
         /// <inheritdoc/>
-        public ImageMetadata DeepClone() => new ImageMetadata(this);
+        public ImageMetadata DeepClone() => new(this);
 
         /// <summary>
         /// Synchronizes the profiles with the current metadata.

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

@@ -259,5 +259,17 @@ public void Issue1962<TPixel>(TestImageProvider<TPixel> provider)
 
             image.CompareFirstFrameToReferenceOutput(ImageComparer.Exact, provider);
         }
+
+        // https://github.com/SixLabors/ImageSharp/issues/2012
+        [Theory]
+        [WithFile(TestImages.Gif.Issues.Issue2012EmptyXmp, PixelTypes.Rgba32)]
+        public void Issue2012EmptyXmp<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : unmanaged, IPixel<TPixel>
+        {
+            using Image<TPixel> image = provider.GetImage();
+
+            image.DebugSave(provider);
+            image.CompareFirstFrameToReferenceOutput(ImageComparer.Exact, provider);
+        }
     }
 }

tests/ImageSharp.Tests/Formats/WebP/WebpMetaDataTests.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0.
 
 using System.IO;
+using System.Threading.Tasks;
 using SixLabors.ImageSharp.Formats.Webp;
 using SixLabors.ImageSharp.Metadata.Profiles.Exif;
 using SixLabors.ImageSharp.PixelFormats;
@@ -66,7 +67,7 @@ public void IgnoreMetadata_ControlsWhetherIccpIsParsed<TPixel>(TestImageProvider
         [Theory]
         [WithFile(TestImages.Webp.Lossy.WithXmp, PixelTypes.Rgba32, false)]
         [WithFile(TestImages.Webp.Lossy.WithXmp, PixelTypes.Rgba32, true)]
-        public async void IgnoreMetadata_ControlsWhetherXmpIsParsed<TPixel>(TestImageProvider<TPixel> provider, bool ignoreMetadata)
+        public async Task IgnoreMetadata_ControlsWhetherXmpIsParsed<TPixel>(TestImageProvider<TPixel> provider, bool ignoreMetadata)
             where TPixel : unmanaged, IPixel<TPixel>
         {
             var decoder = new WebpDecoder { IgnoreMetadata = ignoreMetadata };

tests/ImageSharp.Tests/Metadata/Profiles/XMP/XmpProfileTests.cs

@@ -4,6 +4,7 @@
 using System;
 using System.IO;
 using System.Text;
+using System.Threading.Tasks;
 using System.Xml.Linq;
 using SixLabors.ImageSharp.Formats;
 using SixLabors.ImageSharp.Formats.Gif;
@@ -31,7 +32,7 @@ public class XmpProfileTests
 
         [Theory]
         [WithFile(TestImages.Gif.Receipt, PixelTypes.Rgba32)]
-        public async void ReadXmpMetadata_FromGif_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public async Task ReadXmpMetadata_FromGif_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel>
         {
             using (Image<TPixel> image = await provider.GetImageAsync(GifDecoder))
@@ -45,7 +46,7 @@ public async void ReadXmpMetadata_FromGif_Works<TPixel>(TestImageProvider<TPixel
         [WithFile(TestImages.Jpeg.Baseline.Lake, PixelTypes.Rgba32)]
         [WithFile(TestImages.Jpeg.Baseline.Metadata, PixelTypes.Rgba32)]
         [WithFile(TestImages.Jpeg.Baseline.ExtendedXmp, PixelTypes.Rgba32)]
-        public async void ReadXmpMetadata_FromJpg_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public async Task ReadXmpMetadata_FromJpg_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel>
         {
             using (Image<TPixel> image = await provider.GetImageAsync(JpegDecoder))
@@ -57,7 +58,7 @@ public async void ReadXmpMetadata_FromJpg_Works<TPixel>(TestImageProvider<TPixel
 
         [Theory]
         [WithFile(TestImages.Png.XmpColorPalette, PixelTypes.Rgba32)]
-        public async void ReadXmpMetadata_FromPng_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public async Task ReadXmpMetadata_FromPng_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel>
         {
             using (Image<TPixel> image = await provider.GetImageAsync(PngDecoder))
@@ -69,7 +70,7 @@ public async void ReadXmpMetadata_FromPng_Works<TPixel>(TestImageProvider<TPixel
 
         [Theory]
         [WithFile(TestImages.Tiff.SampleMetadata, PixelTypes.Rgba32)]
-        public async void ReadXmpMetadata_FromTiff_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public async Task ReadXmpMetadata_FromTiff_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel>
         {
             using (Image<TPixel> image = await provider.GetImageAsync(TiffDecoder))
@@ -81,7 +82,7 @@ public async void ReadXmpMetadata_FromTiff_Works<TPixel>(TestImageProvider<TPixe
 
         [Theory]
         [WithFile(TestImages.Webp.Lossy.WithXmp, PixelTypes.Rgba32)]
-        public async void ReadXmpMetadata_FromWebp_Works<TPixel>(TestImageProvider<TPixel> provider)
+        public async Task ReadXmpMetadata_FromWebp_Works<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel>
         {
             using (Image<TPixel> image = await provider.GetImageAsync(WebpDecoder))
@@ -157,7 +158,7 @@ public void WritingJpeg_PreservesXmpProfile()
         }
 
         [Fact]
-        public async void WritingJpeg_PreservesExtendedXmpProfile()
+        public async Task WritingJpeg_PreservesExtendedXmpProfile()
         {
             // arrange
             var provider = TestImageProvider<Rgba32>.File(TestImages.Jpeg.Baseline.ExtendedXmp);

tests/ImageSharp.Tests/TestImages.cs

@@ -452,6 +452,7 @@ public static class Issues
                 public const string Issue1530 = "Gif/issues/issue1530.gif";
                 public const string InvalidColorIndex = "Gif/issues/issue1668_invalidcolorindex.gif";
                 public const string Issue1962NoColorTable = "Gif/issues/issue1962_tiniest_gif_1st.gif";
+                public const string Issue2012EmptyXmp = "Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif";
             }
 
             public static readonly string[] All = { Rings, Giphy, Cheers, Trans, Kumin, Leo, Ratio4x1, Ratio1x4 };