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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tiff decoding robustness improvements #2550

Merged
merged 3 commits into from
Oct 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 20 additions & 25 deletions src/ImageSharp/Compression/Zlib/ZlibInflateStream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -161,29 +161,25 @@ public override int Read(byte[] buffer, int offset, int count)
bytesToRead = Math.Min(count - totalBytesRead, this.currentDataRemaining);
this.currentDataRemaining -= bytesToRead;
bytesRead = this.innerStream.Read(buffer, offset, bytesToRead);
if (bytesRead == 0)
{
return totalBytesRead;
}

totalBytesRead += bytesRead;
}

return totalBytesRead;
}
Copy link
Member

Choose a reason for hiding this comment

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

Since we are already touching this class: we should check the return value of this.innerStream.Read in InitializeInflateStream when it's skipping 4 bytes.

Copy link
Member

Choose a reason for hiding this comment

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

Pushed the change together with another check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good changes. Thanks!


/// <inheritdoc/>
public override long Seek(long offset, SeekOrigin origin)
{
throw new NotSupportedException();
}
public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException();

/// <inheritdoc/>
public override void SetLength(long value)
{
throw new NotSupportedException();
}
public override void SetLength(long value) => throw new NotSupportedException();

/// <inheritdoc/>
public override void Write(byte[] buffer, int offset, int count)
{
throw new NotSupportedException();
}
public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException();

/// <inheritdoc/>
protected override void Dispose(bool disposing)
Expand Down Expand Up @@ -246,22 +242,17 @@ private bool InitializeInflateStream(bool isCriticalChunk)
// CINFO is not defined in this specification for CM not equal to 8.
throw new ImageFormatException($"Invalid window size for ZLIB header: cinfo={cinfo}");
}
else
{
return false;
}

return false;
}
}
else if (isCriticalChunk)
{
throw new ImageFormatException($"Bad method for ZLIB header: cmf={cmf}");
}
else
{
if (isCriticalChunk)
{
throw new ImageFormatException($"Bad method for ZLIB header: cmf={cmf}");
}
else
{
return false;
}
return false;
}

// The preset dictionary.
Expand All @@ -270,7 +261,11 @@ private bool InitializeInflateStream(bool isCriticalChunk)
{
// We don't need this for inflate so simply skip by the next four bytes.
// https://tools.ietf.org/html/rfc1950#page-6
this.innerStream.Read(ChecksumBuffer, 0, 4);
if (this.innerStream.Read(ChecksumBuffer, 0, 4) != 4)
{
return false;
}

this.currentDataRemaining -= 4;
}

Expand Down
22 changes: 16 additions & 6 deletions src/ImageSharp/Formats/Tiff/Ifd/DirectoryReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public DirectoryReader(Stream stream, MemoryAllocator allocator)
public IList<ExifProfile> Read()
{
this.ByteOrder = ReadByteOrder(this.stream);
var headerReader = new HeaderReader(this.stream, this.ByteOrder);
HeaderReader headerReader = new(this.stream, this.ByteOrder);
headerReader.ReadFileHeader();

this.nextIfdOffset = headerReader.FirstIfdOffset;
Expand All @@ -52,7 +52,12 @@ public IList<ExifProfile> Read()
private static ByteOrder ReadByteOrder(Stream stream)
{
Span<byte> headerBytes = stackalloc byte[2];
stream.Read(headerBytes);

if (stream.Read(headerBytes) != 2)
{
throw TiffThrowHelper.ThrowInvalidHeader();
}

if (headerBytes[0] == TiffConstants.ByteOrderLittleEndian && headerBytes[1] == TiffConstants.ByteOrderLittleEndian)
{
return ByteOrder.LittleEndian;
Expand All @@ -68,10 +73,10 @@ private static ByteOrder ReadByteOrder(Stream stream)

private IList<ExifProfile> ReadIfds(bool isBigTiff)
{
var readers = new List<EntryReader>();
List<EntryReader> readers = new();
while (this.nextIfdOffset != 0 && this.nextIfdOffset < (ulong)this.stream.Length)
{
var reader = new EntryReader(this.stream, this.ByteOrder, this.allocator);
EntryReader reader = new(this.stream, this.ByteOrder, this.allocator);
reader.ReadTags(isBigTiff, this.nextIfdOffset);

if (reader.BigValues.Count > 0)
Expand All @@ -85,6 +90,11 @@ private IList<ExifProfile> ReadIfds(bool isBigTiff)
}
}

if (this.nextIfdOffset >= reader.NextIfdOffset && reader.NextIfdOffset != 0)
{
TiffThrowHelper.ThrowImageFormatException("TIFF image contains circular directory offsets");
}

this.nextIfdOffset = reader.NextIfdOffset;
readers.Add(reader);

Expand All @@ -94,11 +104,11 @@ private IList<ExifProfile> ReadIfds(bool isBigTiff)
}
}

var list = new List<ExifProfile>(readers.Count);
List<ExifProfile> list = new(readers.Count);
foreach (EntryReader reader in readers)
{
reader.ReadBigValues();
var profile = new ExifProfile(reader.Values, reader.InvalidTags);
ExifProfile profile = new(reader.Values, reader.InvalidTags);
list.Add(profile);
}

Expand Down
28 changes: 27 additions & 1 deletion tests/ImageSharp.Tests/Formats/Tiff/TiffDecoderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Six Labors Split License.

// ReSharper disable InconsistentNaming
using System.Runtime.InteropServices;
using System.Runtime.Intrinsics.X86;
using SixLabors.ImageSharp.Formats;
using SixLabors.ImageSharp.Formats.Tiff;
Expand Down Expand Up @@ -352,7 +351,7 @@
[Theory]
[WithFile(Rgba8BitAssociatedAlpha, PixelTypes.Rgba32)]
public void TiffDecoder_CanDecode_32Bit_WithAssociatedAlpha<TPixel>(TestImageProvider<TPixel> provider)
// Note: Using tolerant comparer here, because there is a small difference to the reference decoder probably due to floating point rounding issues.

Check warning on line 354 in tests/ImageSharp.Tests/Formats/Tiff/TiffDecoderTests.cs

View workflow job for this annotation

GitHub Actions / Build (false, windows-latest, net7.0, 7.0.x, true, -x64, false)

Check warning on line 354 in tests/ImageSharp.Tests/Formats/Tiff/TiffDecoderTests.cs

View workflow job for this annotation

GitHub Actions / Build (false, windows-latest, net6.0, 6.0.x, -x64, false)

where TPixel : unmanaged, IPixel<TPixel> => TestTiffDecoder(provider, useExactComparer: false, compareTolerance: 0.004F);

[Theory]
Expand Down Expand Up @@ -666,6 +665,33 @@
public void TiffDecoder_CanDecode_TiledWithNonEqualWidthAndHeight<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel> => TestTiffDecoder(provider);

[Theory]
[WithFile(JpegCompressedGray0000539558, PixelTypes.Rgba32)]
public void TiffDecoder_ThrowsException_WithCircular_IFD_Offsets<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
=> Assert.Throws<ImageFormatException>(
() =>
{
using (provider.GetImage(TiffDecoder.Instance))
{
}
});

[Theory]
[WithFile(Tiled0000023664, PixelTypes.Rgba32)]
public void TiffDecoder_CanDecode_TiledWithBadZlib<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
using Image<TPixel> image = provider.GetImage(TiffDecoder.Instance);

// ImageMagick cannot decode this image.
image.DebugSave(provider);
image.CompareToReferenceOutput(
ImageComparer.Exact,
antonfirsov marked this conversation as resolved.
Show resolved Hide resolved
provider,
appendPixelTypeToFileName: false);
}

[Theory]
[WithFileCollection(nameof(MultiframeTestImages), PixelTypes.Rgba32)]
public void DecodeMultiframe<TPixel>(TestImageProvider<TPixel> provider)
Expand Down
2 changes: 2 additions & 0 deletions tests/ImageSharp.Tests/TestImages.cs
Original file line number Diff line number Diff line change
Expand Up @@ -980,6 +980,8 @@ public static class Tiff
public const string Issues2149 = "Tiff/Issues/Group4CompressionWithStrips.tiff";
public const string Issues2255 = "Tiff/Issues/Issue2255.png";
public const string Issues2435 = "Tiff/Issues/Issue2435.tiff";
public const string JpegCompressedGray0000539558 = "Tiff/Issues/JpegCompressedGray-0000539558.tiff";
public const string Tiled0000023664 = "Tiff/Issues/tiled-0000023664.tiff";

public const string SmallRgbDeflate = "Tiff/rgb_small_deflate.tiff";
public const string SmallRgbLzw = "Tiff/rgb_small_lzw.tiff";
Expand Down
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Git LFS file not shown
3 changes: 3 additions & 0 deletions tests/Images/Input/Tiff/Issues/tiled-0000023664.tiff
Git LFS file not shown