From 061436f7656828f4f07586b87e118479b6b3f836 Mon Sep 17 00:00:00 2001 From: Curtis Wensley Date: Thu, 20 Sep 2018 11:08:49 -0700 Subject: [PATCH 1/2] Fix issue on mono <= 5.14 when reading multiple png data chunks. Fixes #598 --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 139 +++++++++++------- .../Formats/Png/Zlib/ZlibInflateStream.cs | 16 +- 2 files changed, 98 insertions(+), 57 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 928192701..76162eacb 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -187,6 +187,11 @@ namespace SixLabors.ImageSharp.Formats.Png /// private bool hasTrans; + /// + /// The next chunk of data to return + /// + private PngChunk? nextChunk; + /// /// Initializes a new instance of the class. /// @@ -223,67 +228,66 @@ namespace SixLabors.ImageSharp.Formats.Png Image image = null; try { - using (var deframeStream = new ZlibInflateStream(this.currentStream)) + while (!this.isEndChunkReached && this.TryReadChunk(out PngChunk chunk)) { - while (!this.isEndChunkReached && this.TryReadChunk(out PngChunk chunk)) + try { - try + switch (chunk.Type) { - switch (chunk.Type) - { - case PngChunkType.Header: - this.ReadHeaderChunk(pngMetaData, chunk.Data.Array); - this.ValidateHeader(); - break; - case PngChunkType.Physical: - this.ReadPhysicalChunk(metaData, chunk.Data.GetSpan()); - break; - case PngChunkType.Gamma: - this.ReadGammaChunk(pngMetaData, chunk.Data.GetSpan()); - break; - case PngChunkType.Data: - if (image is null) - { - this.InitializeImage(metaData, out image); - } - - deframeStream.AllocateNewBytes(chunk.Length); + case PngChunkType.Header: + this.ReadHeaderChunk(pngMetaData, chunk.Data.Array); + this.ValidateHeader(); + break; + case PngChunkType.Physical: + this.ReadPhysicalChunk(metaData, chunk.Data.GetSpan()); + break; + case PngChunkType.Gamma: + this.ReadGammaChunk(pngMetaData, chunk.Data.GetSpan()); + break; + case PngChunkType.Data: + if (image is null) + { + this.InitializeImage(metaData, out image); + } + + using (var deframeStream = new ZlibInflateStream(this.currentStream, this.ReadNextDataChunk)) + { + deframeStream.AllocateNewBytes(chunk.Length); this.ReadScanlines(deframeStream.CompressedStream, image.Frames.RootFrame); - this.currentStream.Read(this.crcBuffer, 0, 4); - break; - case PngChunkType.Palette: - byte[] pal = new byte[chunk.Length]; - Buffer.BlockCopy(chunk.Data.Array, 0, pal, 0, chunk.Length); - this.palette = pal; - break; - case PngChunkType.PaletteAlpha: - byte[] alpha = new byte[chunk.Length]; - Buffer.BlockCopy(chunk.Data.Array, 0, alpha, 0, chunk.Length); - this.paletteAlpha = alpha; - this.AssignTransparentMarkers(alpha); - break; - case PngChunkType.Text: - this.ReadTextChunk(metaData, chunk.Data.Array, chunk.Length); - break; - case PngChunkType.Exif: - if (!this.ignoreMetadata) - { - byte[] exifData = new byte[chunk.Length]; - Buffer.BlockCopy(chunk.Data.Array, 0, exifData, 0, chunk.Length); - metaData.ExifProfile = new ExifProfile(exifData); - } - - break; - case PngChunkType.End: - this.isEndChunkReached = true; - break; - } - } - finally - { - chunk.Data?.Dispose(); // Data is rented in ReadChunkData() + } + break; + case PngChunkType.Palette: + byte[] pal = new byte[chunk.Length]; + Buffer.BlockCopy(chunk.Data.Array, 0, pal, 0, chunk.Length); + this.palette = pal; + break; + case PngChunkType.PaletteAlpha: + byte[] alpha = new byte[chunk.Length]; + Buffer.BlockCopy(chunk.Data.Array, 0, alpha, 0, chunk.Length); + this.paletteAlpha = alpha; + this.AssignTransparentMarkers(alpha); + break; + case PngChunkType.Text: + this.ReadTextChunk(metaData, chunk.Data.Array, chunk.Length); + break; + case PngChunkType.Exif: + if (!this.ignoreMetadata) + { + byte[] exifData = new byte[chunk.Length]; + Buffer.BlockCopy(chunk.Data.Array, 0, exifData, 0, chunk.Length); + metaData.ExifProfile = new ExifProfile(exifData); + } + + break; + case PngChunkType.End: + this.isEndChunkReached = true; + break; } } + finally + { + chunk.Data?.Dispose(); // Data is rented in ReadChunkData() + } } if (image is null) @@ -1366,6 +1370,25 @@ namespace SixLabors.ImageSharp.Formats.Png metadata.Properties.Add(new ImageProperty(name, value)); } + /// + /// Reads the next data chunk. + /// + /// Count of bytes in the next data chunk, or 0 if there are no more data chunks left. + private int ReadNextDataChunk() + { + this.currentStream.Read(this.crcBuffer, 0, 4); + + this.TryReadChunk(out PngChunk chunk); + + if (chunk.Type == PngChunkType.Data) + { + return chunk.Length; + } + + this.nextChunk = chunk; + return 0; + } + /// /// Reads a chunk from the stream. /// @@ -1375,6 +1398,12 @@ namespace SixLabors.ImageSharp.Formats.Png /// private bool TryReadChunk(out PngChunk chunk) { + if (this.nextChunk != null) + { + chunk = this.nextChunk.Value; + return true; + } + int length = this.ReadChunkLength(); if (length == -1) diff --git a/src/ImageSharp/Formats/Png/Zlib/ZlibInflateStream.cs b/src/ImageSharp/Formats/Png/Zlib/ZlibInflateStream.cs index 55432d60b..c2f6d6d88 100644 --- a/src/ImageSharp/Formats/Png/Zlib/ZlibInflateStream.cs +++ b/src/ImageSharp/Formats/Png/Zlib/ZlibInflateStream.cs @@ -52,13 +52,20 @@ namespace SixLabors.ImageSharp.Formats.Png.Zlib /// private int currentDataRemaining; + /// + /// Delegate to get more data once we've exhausted the current data remaining + /// + private Func getData; + /// /// Initializes a new instance of the class. /// /// The inner raw stream - public ZlibInflateStream(Stream innerStream) + /// A delegate to get more data from the inner stream + public ZlibInflateStream(Stream innerStream, Func getData) { this.innerStream = innerStream; + this.getData = getData; } /// @@ -112,7 +119,12 @@ namespace SixLabors.ImageSharp.Formats.Png.Zlib { if (this.currentDataRemaining == 0) { - return 0; + this.currentDataRemaining = this.getData(); + + if (this.currentDataRemaining == 0) + { + return 0; + } } int bytesToRead = Math.Min(count, this.currentDataRemaining); From 4979e1971f5fa16fb440ee1bdee15a51d0287c56 Mon Sep 17 00:00:00 2001 From: Curtis Wensley Date: Thu, 20 Sep 2018 13:44:33 -0700 Subject: [PATCH 2/2] Fixes to make all unit tests pass - Also fix some formatting warnings --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 23 +++++++--- .../Formats/Png/Zlib/ZlibInflateStream.cs | 44 +++++++++++-------- 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 76162eacb..ef2f22655 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -252,9 +252,10 @@ namespace SixLabors.ImageSharp.Formats.Png using (var deframeStream = new ZlibInflateStream(this.currentStream, this.ReadNextDataChunk)) { - deframeStream.AllocateNewBytes(chunk.Length); + deframeStream.AllocateNewBytes(chunk.Length); this.ReadScanlines(deframeStream.CompressedStream, image.Frames.RootFrame); } + break; case PngChunkType.Palette: byte[] pal = new byte[chunk.Length]; @@ -1376,16 +1377,23 @@ namespace SixLabors.ImageSharp.Formats.Png /// Count of bytes in the next data chunk, or 0 if there are no more data chunks left. private int ReadNextDataChunk() { - this.currentStream.Read(this.crcBuffer, 0, 4); + if (this.nextChunk != null) + { + return 0; + } - this.TryReadChunk(out PngChunk chunk); + this.currentStream.Read(this.crcBuffer, 0, 4); - if (chunk.Type == PngChunkType.Data) + if (this.TryReadChunk(out PngChunk chunk)) { - return chunk.Length; + if (chunk.Type == PngChunkType.Data) + { + return chunk.Length; + } + + this.nextChunk = chunk; } - this.nextChunk = chunk; return 0; } @@ -1401,6 +1409,9 @@ namespace SixLabors.ImageSharp.Formats.Png if (this.nextChunk != null) { chunk = this.nextChunk.Value; + + this.nextChunk = null; + return true; } diff --git a/src/ImageSharp/Formats/Png/Zlib/ZlibInflateStream.cs b/src/ImageSharp/Formats/Png/Zlib/ZlibInflateStream.cs index c2f6d6d88..a92220a59 100644 --- a/src/ImageSharp/Formats/Png/Zlib/ZlibInflateStream.cs +++ b/src/ImageSharp/Formats/Png/Zlib/ZlibInflateStream.cs @@ -42,11 +42,6 @@ namespace SixLabors.ImageSharp.Formats.Png.Zlib /// private bool isDisposed; - /// - /// Whether the crc value has been read. - /// - private bool crcRead; - /// /// The current data remaining to be read /// @@ -119,17 +114,36 @@ namespace SixLabors.ImageSharp.Formats.Png.Zlib { if (this.currentDataRemaining == 0) { - this.currentDataRemaining = this.getData(); + // last buffer was read in its entirety, let's make sure we don't actually have more + this.currentDataRemaining = this.getData(); - if (this.currentDataRemaining == 0) - { - return 0; - } + if (this.currentDataRemaining == 0) + { + return 0; + } } int bytesToRead = Math.Min(count, this.currentDataRemaining); this.currentDataRemaining -= bytesToRead; - return this.innerStream.Read(buffer, offset, bytesToRead); + int bytesRead = this.innerStream.Read(buffer, offset, bytesToRead); + + // keep reading data until we've reached the end of the stream or filled the buffer + while (this.currentDataRemaining == 0 && bytesRead < count) + { + this.currentDataRemaining = this.getData(); + + if (this.currentDataRemaining == 0) + { + return bytesRead; + } + + offset += bytesRead; + bytesToRead = Math.Min(count - bytesRead, this.currentDataRemaining); + this.currentDataRemaining -= bytesToRead; + bytesRead += this.innerStream.Read(buffer, offset, bytesToRead); + } + + return bytesRead; } /// @@ -165,14 +179,6 @@ namespace SixLabors.ImageSharp.Formats.Png.Zlib { this.compressedStream.Dispose(); this.compressedStream = null; - - if (!this.crcRead) - { - // Consume the trailing 4 bytes - this.innerStream.Read(ChecksumBuffer, 0, 4); - this.currentDataRemaining -= 4; - this.crcRead = true; - } } }