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

PBM decoder robustness improvements and BufferedReadStream observability #2551

Merged
merged 7 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/ImageSharp/Formats/ImageDecoderUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ internal static Image<TPixel> Decode<TPixel>(
CancellationToken cancellationToken)
where TPixel : unmanaged, IPixel<TPixel>
{
using BufferedReadStream bufferedReadStream = new(configuration, stream, cancellationToken);
// Test may pass a BufferedReadStream in order to monitor EOF hits, if so, use the existing instance.
BufferedReadStream bufferedReadStream = stream as BufferedReadStream ?? new BufferedReadStream(configuration, stream, cancellationToken);

try
{
Expand All @@ -64,6 +65,13 @@ internal static Image<TPixel> Decode<TPixel>(
{
throw;
}
finally
{
if (bufferedReadStream != stream)
{
bufferedReadStream.Dispose();
}
}
}

private static InvalidImageContentException DefaultLargeImageExceptionFactory(
Expand Down
29 changes: 25 additions & 4 deletions src/ImageSharp/Formats/Pbm/BinaryDecoder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,11 @@ private static void ProcessGrayscale<TPixel>(Configuration configuration, Buffer

for (int y = 0; y < height; y++)
{
stream.Read(rowSpan);
if (stream.Read(rowSpan) == 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is lame actually. I should have sliced down rowSpan with the result of stream.Read(), for the pixel conversion done later, or used the condition stream.Read(rowSpan) < rowSpan.Length (the latter isstricter, but simpler).

This has little practical relevance (BuffferedReadStream is hitting EOF twice & FromL8Bytes is converting some memory garbage for broken files), so no need to fix it now, but the code looks stupid :)

Copy link
Member

Choose a reason for hiding this comment

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

We can improve it with follow up.

Copy link
Member Author

Choose a reason for hiding this comment

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

{
return;
}

Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
PixelOperations<TPixel>.Instance.FromL8Bytes(
configuration,
Expand All @@ -93,7 +97,11 @@ private static void ProcessWideGrayscale<TPixel>(Configuration configuration, Bu

for (int y = 0; y < height; y++)
{
stream.Read(rowSpan);
if (stream.Read(rowSpan) == 0)
{
return;
}

Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
PixelOperations<TPixel>.Instance.FromL16Bytes(
configuration,
Expand All @@ -115,7 +123,11 @@ private static void ProcessRgb<TPixel>(Configuration configuration, Buffer2D<TPi

for (int y = 0; y < height; y++)
{
stream.Read(rowSpan);
if (stream.Read(rowSpan) == 0)
{
return;
}

Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
PixelOperations<TPixel>.Instance.FromRgb24Bytes(
configuration,
Expand All @@ -137,7 +149,11 @@ private static void ProcessWideRgb<TPixel>(Configuration configuration, Buffer2D

for (int y = 0; y < height; y++)
{
stream.Read(rowSpan);
if (stream.Read(rowSpan) == 0)
{
return;
}

Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
PixelOperations<TPixel>.Instance.FromRgb48Bytes(
configuration,
Expand All @@ -161,6 +177,11 @@ private static void ProcessBlackAndWhite<TPixel>(Configuration configuration, Bu
for (int x = 0; x < width;)
{
int raw = stream.ReadByte();
if (raw < 0)
{
return;
}

int stopBit = Math.Min(8, width - x);
for (int bit = 0; bit < stopBit; bit++)
{
Expand Down
41 changes: 32 additions & 9 deletions src/ImageSharp/Formats/Pbm/BufferedReadStreamExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,20 @@ namespace SixLabors.ImageSharp.Formats.Pbm;
internal static class BufferedReadStreamExtensions
{
/// <summary>
/// Skip over any whitespace or any comments.
/// Skip over any whitespace or any comments and signal if EOF has been reached.
/// </summary>
public static void SkipWhitespaceAndComments(this BufferedReadStream stream)
/// <param name="stream">The buffered read stream.</param>
/// <returns><see langword="false"/> if EOF has been reached while reading the stream; see langword="true"/> otherwise.</returns>
public static bool TrySkipWhitespaceAndComments(this BufferedReadStream stream)
{
bool isWhitespace;
do
{
int val = stream.ReadByte();
if (val < 0)
{
return false;
}

// Comments start with '#' and end at the next new-line.
if (val == 0x23)
Expand All @@ -27,8 +33,12 @@ public static void SkipWhitespaceAndComments(this BufferedReadStream stream)
do
{
innerValue = stream.ReadByte();
if (innerValue < 0)
{
return false;
}
}
while (innerValue is not 0x0a and not -0x1);
while (innerValue is not 0x0a);

// Continue searching for whitespace.
val = innerValue;
Expand All @@ -38,18 +48,31 @@ public static void SkipWhitespaceAndComments(this BufferedReadStream stream)
}
while (isWhitespace);
stream.Seek(-1, SeekOrigin.Current);
return true;
}

/// <summary>
/// Read a decimal text value.
/// Read a decimal text value and signal if EOF has been reached.
/// </summary>
/// <returns>The integer value of the decimal.</returns>
public static int ReadDecimal(this BufferedReadStream stream)
/// <param name="stream">The buffered read stream.</param>
/// <param name="value">The read value.</param>
/// <returns><see langword="false"/> if EOF has been reached while reading the stream; <see langword="true"/> otherwise.</returns>
/// <remarks>
/// A 'false' return value doesn't mean that the parsing has been failed, since it's possible to reach EOF while reading the last decimal in the file.
/// It's up to the call site to handle such a situation.
/// </remarks>
public static bool TryReadDecimal(this BufferedReadStream stream, out int value)
{
int value = 0;
value = 0;
while (true)
{
int current = stream.ReadByte() - 0x30;
int current = stream.ReadByte();
if (current < 0)
{
return false;
}

current -= 0x30;
if ((uint)current > 9)
{
break;
Expand All @@ -58,6 +81,6 @@ public static int ReadDecimal(this BufferedReadStream stream)
value = (value * 10) + current;
}

return value;
return true;
}
}
27 changes: 20 additions & 7 deletions src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Diagnostics.CodeAnalysis;
using SixLabors.ImageSharp.IO;
using SixLabors.ImageSharp.Memory;
using SixLabors.ImageSharp.Metadata;
Expand Down Expand Up @@ -95,6 +96,7 @@ public ImageInfo Identify(BufferedReadStream stream, CancellationToken cancellat
/// Processes the ppm header.
/// </summary>
/// <param name="stream">The input stream.</param>
/// <exception cref="InvalidImageContentException">An EOF marker has been read before the image has been decoded.</exception>
private void ProcessHeader(BufferedReadStream stream)
{
Span<byte> buffer = stackalloc byte[2];
Expand Down Expand Up @@ -144,14 +146,22 @@ private void ProcessHeader(BufferedReadStream stream)
throw new InvalidImageContentException("Unknown of not implemented image type encountered.");
}

stream.SkipWhitespaceAndComments();
int width = stream.ReadDecimal();
stream.SkipWhitespaceAndComments();
int height = stream.ReadDecimal();
stream.SkipWhitespaceAndComments();
if (!stream.TrySkipWhitespaceAndComments() ||
!stream.TryReadDecimal(out int width) ||
!stream.TrySkipWhitespaceAndComments() ||
!stream.TryReadDecimal(out int height) ||
!stream.TrySkipWhitespaceAndComments())
{
ThrowPrematureEof();
}

if (this.colorType != PbmColorType.BlackAndWhite)
{
this.maxPixelValue = stream.ReadDecimal();
if (!stream.TryReadDecimal(out this.maxPixelValue))
{
ThrowPrematureEof();
}

if (this.maxPixelValue > 255)
{
this.componentType = PbmComponentType.Short;
Expand All @@ -161,7 +171,7 @@ private void ProcessHeader(BufferedReadStream stream)
this.componentType = PbmComponentType.Byte;
}

stream.SkipWhitespaceAndComments();
stream.TrySkipWhitespaceAndComments();
}
else
{
Expand All @@ -174,6 +184,9 @@ private void ProcessHeader(BufferedReadStream stream)
meta.Encoding = this.encoding;
meta.ColorType = this.colorType;
meta.ComponentType = this.componentType;

[DoesNotReturn]
static void ThrowPrematureEof() => throw new InvalidImageContentException("Reached EOF while reading the header.");
}

private void ProcessPixels<TPixel>(BufferedReadStream stream, Buffer2D<TPixel> pixels)
Expand Down
Loading
Loading