Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relaxed handle of corrupt png files #2589

Merged
14 changes: 9 additions & 5 deletions src/ImageSharp/Formats/Png/PngChunk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,13 @@ public PngChunk(int length, PngChunkType type, IMemoryOwner<byte> data = null)
/// <summary>
/// Gets a value indicating whether the given chunk is critical to decoding
/// </summary>
public bool IsCritical =>
this.Type is PngChunkType.Header or
PngChunkType.Palette or
PngChunkType.Data or
PngChunkType.FrameData;
/// <param name="handling">The chunk CRC handling behavior.</param>
public bool IsCritical(PngCrcChunkHandling handling)
=> handling switch
{
PngCrcChunkHandling.IgnoreNone => true,
PngCrcChunkHandling.IgnoreNonCritical => this.Type is PngChunkType.Header or PngChunkType.Palette or PngChunkType.Data or PngChunkType.FrameData,
PngCrcChunkHandling.IgnoreData => this.Type is PngChunkType.Header or PngChunkType.Palette,
_ => false,
};
}
30 changes: 30 additions & 0 deletions src/ImageSharp/Formats/Png/PngCrcChunkHandling.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

namespace SixLabors.ImageSharp.Formats.Png;

/// <summary>
/// Specifies how to handle validation of any CRC (Cyclic Redundancy Check) data within the encoded PNG.
/// </summary>
public enum PngCrcChunkHandling
{
/// <summary>
/// Do not ignore any CRC chunk errors.
/// </summary>
IgnoreNone,

/// <summary>
/// Ignore CRC errors in non critical chunks.
/// </summary>
IgnoreNonCritical,

/// <summary>
/// Ignore CRC errors in data chunks.
/// </summary>
IgnoreData,

/// <summary>
/// Ignore CRC errors in all chunks.
/// </summary>
IgnoreAll
}
17 changes: 10 additions & 7 deletions src/ImageSharp/Formats/Png/PngDecoder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace SixLabors.ImageSharp.Formats.Png;
/// <summary>
/// Decoder for generating an image out of a png encoded stream.
/// </summary>
public sealed class PngDecoder : ImageDecoder
public sealed class PngDecoder : SpecializedImageDecoder<PngDecoderOptions>
{
private PngDecoder()
{
Expand All @@ -25,31 +25,31 @@ protected override ImageInfo Identify(DecoderOptions options, Stream stream, Can
Guard.NotNull(options, nameof(options));
Guard.NotNull(stream, nameof(stream));

return new PngDecoderCore(options).Identify(options.Configuration, stream, cancellationToken);
return new PngDecoderCore(new PngDecoderOptions() { GeneralOptions = options }).Identify(options.Configuration, stream, cancellationToken);
}

/// <inheritdoc/>
protected override Image<TPixel> Decode<TPixel>(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
protected override Image<TPixel> Decode<TPixel>(PngDecoderOptions options, Stream stream, CancellationToken cancellationToken)
{
Guard.NotNull(options, nameof(options));
Guard.NotNull(stream, nameof(stream));

PngDecoderCore decoder = new(options);
Image<TPixel> image = decoder.Decode<TPixel>(options.Configuration, stream, cancellationToken);
Image<TPixel> image = decoder.Decode<TPixel>(options.GeneralOptions.Configuration, stream, cancellationToken);

ScaleToTargetSize(options, image);
ScaleToTargetSize(options.GeneralOptions, image);

return image;
}

/// <inheritdoc/>
protected override Image Decode(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
protected override Image Decode(PngDecoderOptions options, Stream stream, CancellationToken cancellationToken)
{
Guard.NotNull(options, nameof(options));
Guard.NotNull(stream, nameof(stream));

PngDecoderCore decoder = new(options, true);
ImageInfo info = decoder.Identify(options.Configuration, stream, cancellationToken);
ImageInfo info = decoder.Identify(options.GeneralOptions.Configuration, stream, cancellationToken);
stream.Position = 0;

PngMetadata meta = info.Metadata.GetPngMetadata();
Expand Down Expand Up @@ -99,4 +99,7 @@ protected override Image Decode(DecoderOptions options, Stream stream, Cancellat
return this.Decode<Rgba32>(options, stream, cancellationToken);
}
}

/// <inheritdoc/>
protected override PngDecoderOptions CreateDefaultSpecializedOptions(DecoderOptions options) => new PngDecoderOptions() { GeneralOptions = options };
}
109 changes: 77 additions & 32 deletions src/ImageSharp/Formats/Png/PngDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -114,27 +114,34 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
/// </summary>
private PngChunk? nextChunk;

/// <summary>
/// How to handle CRC errors.
/// </summary>
private readonly PngCrcChunkHandling pngCrcChunkHandling;

/// <summary>
/// Initializes a new instance of the <see cref="PngDecoderCore"/> class.
/// </summary>
/// <param name="options">The decoder options.</param>
public PngDecoderCore(DecoderOptions options)
public PngDecoderCore(PngDecoderOptions options)
{
this.Options = options;
this.configuration = options.Configuration;
this.maxFrames = options.MaxFrames;
this.skipMetadata = options.SkipMetadata;
this.Options = options.GeneralOptions;
this.configuration = options.GeneralOptions.Configuration;
this.maxFrames = options.GeneralOptions.MaxFrames;
this.skipMetadata = options.GeneralOptions.SkipMetadata;
this.memoryAllocator = this.configuration.MemoryAllocator;
this.pngCrcChunkHandling = options.PngCrcChunkHandling;
}

internal PngDecoderCore(DecoderOptions options, bool colorMetadataOnly)
internal PngDecoderCore(PngDecoderOptions options, bool colorMetadataOnly)
{
this.Options = options;
this.Options = options.GeneralOptions;
this.colorMetadataOnly = colorMetadataOnly;
this.maxFrames = options.MaxFrames;
this.maxFrames = options.GeneralOptions.MaxFrames;
this.skipMetadata = true;
this.configuration = options.Configuration;
this.configuration = options.GeneralOptions.Configuration;
this.memoryAllocator = this.configuration.MemoryAllocator;
this.pngCrcChunkHandling = options.PngCrcChunkHandling;
}

/// <inheritdoc/>
Expand Down Expand Up @@ -576,11 +583,23 @@ private static void ReadGammaChunk(PngMetadata pngMetadata, ReadOnlySpan<byte> d
private void InitializeImage<TPixel>(ImageMetadata metadata, FrameControl frameControl, out Image<TPixel> image)
where TPixel : unmanaged, IPixel<TPixel>
{
image = Image.CreateUninitialized<TPixel>(
this.configuration,
this.header.Width,
this.header.Height,
metadata);
// When ignoring data CRCs, we can't use the image constructor that leaves the buffer uncleared.
if (this.pngCrcChunkHandling is PngCrcChunkHandling.IgnoreData or PngCrcChunkHandling.IgnoreAll)
{
image = new Image<TPixel>(
this.configuration,
this.header.Width,
this.header.Height,
metadata);
}
else
{
image = Image.CreateUninitialized<TPixel>(
this.configuration,
this.header.Width,
this.header.Height,
metadata);
}

PngFrameMetadata frameMetadata = image.Frames.RootFrame.Metadata.GetPngMetadata();
frameMetadata.FromChunk(in frameControl);
Expand Down Expand Up @@ -803,6 +822,11 @@ private int CalculateScanlineLength(int width)
break;

default:
if (this.pngCrcChunkHandling is PngCrcChunkHandling.IgnoreData or PngCrcChunkHandling.IgnoreAll)
{
goto EXIT;
}

PngThrowHelper.ThrowUnknownFilter();
break;
}
Expand All @@ -812,6 +836,7 @@ private int CalculateScanlineLength(int width)
currentRow++;
}

EXIT:
blendMemory?.Dispose();
}

Expand Down Expand Up @@ -903,6 +928,11 @@ private int CalculateScanlineLength(int width)
break;

default:
if (this.pngCrcChunkHandling is PngCrcChunkHandling.IgnoreData or PngCrcChunkHandling.IgnoreAll)
{
goto EXIT;
}

PngThrowHelper.ThrowUnknownFilter();
break;
}
Expand Down Expand Up @@ -937,6 +967,7 @@ private int CalculateScanlineLength(int width)
}
}

EXIT:
blendMemory?.Dispose();
}

Expand Down Expand Up @@ -1364,7 +1395,7 @@ private void ReadCompressedTextChunk(ImageMetadata baseMetadata, PngMetadata met

ReadOnlySpan<byte> compressedData = data[(zeroIndex + 2)..];

if (this.TryUncompressTextData(compressedData, PngConstants.Encoding, out string? uncompressed)
if (this.TryDecompressTextData(compressedData, PngConstants.Encoding, out string? uncompressed)
&& !TryReadTextChunkMetadata(baseMetadata, name, uncompressed))
{
metadata.TextData.Add(new PngTextData(name, uncompressed, string.Empty, string.Empty));
Expand Down Expand Up @@ -1508,19 +1539,19 @@ private void ReadColorProfileChunk(ImageMetadata metadata, ReadOnlySpan<byte> da

ReadOnlySpan<byte> compressedData = data[(zeroIndex + 2)..];

if (this.TryUncompressZlibData(compressedData, out byte[] iccpProfileBytes))
if (this.TryDecompressZlibData(compressedData, out byte[] iccpProfileBytes))
{
metadata.IccProfile = new IccProfile(iccpProfileBytes);
}
}

/// <summary>
/// Tries to un-compress zlib compressed data.
/// Tries to decompress zlib compressed data.
/// </summary>
/// <param name="compressedData">The compressed data.</param>
/// <param name="uncompressedBytesArray">The uncompressed bytes array.</param>
/// <returns>True, if de-compressing was successful.</returns>
private unsafe bool TryUncompressZlibData(ReadOnlySpan<byte> compressedData, out byte[] uncompressedBytesArray)
private unsafe bool TryDecompressZlibData(ReadOnlySpan<byte> compressedData, out byte[] uncompressedBytesArray)
{
fixed (byte* compressedDataBase = compressedData)
{
Expand Down Expand Up @@ -1657,7 +1688,7 @@ private void ReadInternationalTextChunk(ImageMetadata metadata, ReadOnlySpan<byt
{
ReadOnlySpan<byte> compressedData = data[dataStartIdx..];

if (this.TryUncompressTextData(compressedData, PngConstants.TranslatedEncoding, out string? uncompressed))
if (this.TryDecompressTextData(compressedData, PngConstants.TranslatedEncoding, out string? uncompressed))
{
pngMetadata.TextData.Add(new PngTextData(keyword, uncompressed, language, translatedKeyword));
}
Expand All @@ -1680,9 +1711,9 @@ private void ReadInternationalTextChunk(ImageMetadata metadata, ReadOnlySpan<byt
/// <param name="encoding">The string encoding to use.</param>
/// <param name="value">The uncompressed value.</param>
/// <returns>The <see cref="bool"/>.</returns>
private bool TryUncompressTextData(ReadOnlySpan<byte> compressedData, Encoding encoding, [NotNullWhen(true)] out string? value)
private bool TryDecompressTextData(ReadOnlySpan<byte> compressedData, Encoding encoding, [NotNullWhen(true)] out string? value)
{
if (this.TryUncompressZlibData(compressedData, out byte[] uncompressedData))
if (this.TryDecompressZlibData(compressedData, out byte[] uncompressedData))
{
value = encoding.GetString(uncompressedData);
return true;
Expand All @@ -1705,7 +1736,11 @@ private int ReadNextDataChunk()

Span<byte> buffer = stackalloc byte[20];

_ = this.currentStream.Read(buffer, 0, 4);
int length = this.currentStream.Read(buffer, 0, 4);
if (length == 0)
{
return 0;
}

if (this.TryReadChunk(buffer, out PngChunk chunk))
{
Expand Down Expand Up @@ -1734,7 +1769,11 @@ private int ReadNextFrameDataChunk()

Span<byte> buffer = stackalloc byte[20];

_ = this.currentStream.Read(buffer, 0, 4);
int length = this.currentStream.Read(buffer, 0, 4);
if (length == 0)
{
return 0;
}

if (this.TryReadChunk(buffer, out PngChunk chunk))
{
Expand Down Expand Up @@ -1771,21 +1810,27 @@ private bool TryReadChunk(Span<byte> buffer, out PngChunk chunk)
return true;
}

if (!this.TryReadChunkLength(buffer, out int length))
if (this.currentStream.Position >= this.currentStream.Length - 1)
{
// IEND
chunk = default;
return false;
}

if (!this.TryReadChunkLength(buffer, out int length))
{
// IEND
chunk = default;
return false;
}

while (length < 0 || length > (this.currentStream.Length - this.currentStream.Position))
while (length < 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonfirsov I did find a way to improve this!

{
// Not a valid chunk so try again until we reach a known chunk.
if (!this.TryReadChunkLength(buffer, out length))
{
// IEND
chunk = default;

return false;
}
}
Expand All @@ -1797,13 +1842,14 @@ private bool TryReadChunk(Span<byte> buffer, out PngChunk chunk)
if (this.colorMetadataOnly && type != PngChunkType.Header && type != PngChunkType.Transparency && type != PngChunkType.Palette)
{
chunk = new PngChunk(length, type);

return true;
}

long pos = this.currentStream.Position;
// A chunk might report a length that exceeds the length of the stream.
// Take the minimum of the two values to ensure we don't read past the end of the stream.
long position = this.currentStream.Position;
chunk = new PngChunk(
length: length,
length: (int)Math.Min(length, this.currentStream.Length - position),
type: type,
data: this.ReadChunkData(length));

Expand All @@ -1813,7 +1859,7 @@ private bool TryReadChunk(Span<byte> buffer, out PngChunk chunk)
// was only read to verifying the CRC is correct.
if (type is PngChunkType.Data or PngChunkType.FrameData)
{
this.currentStream.Position = pos;
this.currentStream.Position = position;
}

return true;
Expand All @@ -1827,8 +1873,7 @@ private bool TryReadChunk(Span<byte> buffer, out PngChunk chunk)
private void ValidateChunk(in PngChunk chunk, Span<byte> buffer)
{
uint inputCrc = this.ReadChunkCrc(buffer);

if (chunk.IsCritical)
if (chunk.IsCritical(this.pngCrcChunkHandling))
{
Span<byte> chunkType = stackalloc byte[4];
BinaryPrimitives.WriteUInt32BigEndian(chunkType, (uint)chunk.Type);
Expand Down
18 changes: 18 additions & 0 deletions src/ImageSharp/Formats/Png/PngDecoderOptions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

namespace SixLabors.ImageSharp.Formats.Png;

/// <summary>
/// Configuration options for decoding png images.
/// </summary>
public sealed class PngDecoderOptions : ISpecializedDecoderOptions
{
/// <inheritdoc/>
public DecoderOptions GeneralOptions { get; init; } = new DecoderOptions();

/// <summary>
/// Gets a value indicating how to handle validation of any CRC (Cyclic Redundancy Check) data within the encoded PNG.
/// </summary>
public PngCrcChunkHandling PngCrcChunkHandling { get; init; } = PngCrcChunkHandling.IgnoreNonCritical;
}
2 changes: 1 addition & 1 deletion tests/ImageSharp.Tests/Formats/ImageFormatManagerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public void IfAutoLoadWellKnownFormatsIsTrueAllFormatsAreLoaded()
Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<PngDecoder>().Count());
Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<BmpDecoder>().Count());
Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<JpegDecoder>().Count());
Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<BmpDecoder>().Count());
Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<GifDecoder>().Count());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good spot!!

Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<TgaDecoder>().Count());
Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<TiffDecoder>().Count());
Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<WebpDecoder>().Count());
Expand Down