Skip to content

PBM decoder robustness improvements and BufferedReadStream observability #2551

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 14, 2023
Merged

PBM decoder robustness improvements and BufferedReadStream observability #2551

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8241a5a
handle premature EOF in the PBM decoder
antonfirsov Oct 9, 2023
fecaa53
BufferedReadStreamExtensions: remove the 'Try' prefix
antonfirsov Oct 13, 2023
a1d7284
count EOF hits in BufferedReadStream
antonfirsov Oct 13, 2023
c74fbec
use EofHitCounter in pbm tests
antonfirsov Oct 14, 2023
c359d13
Merge branch 'main' into pbm-eof-02
antonfirsov Oct 14, 2023
e5ba24c
Merge branch 'main' into pbm-eof-02
JimBobSquarePants Oct 14, 2023
9b42de6
Naming convention tweaks
JimBobSquarePants Oct 14, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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.

#2552

{
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
38 changes: 29 additions & 9 deletions src/ImageSharp/Formats/Pbm/BufferedReadStreamExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,19 @@ 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)
/// <returns><see langword="false"/> if EOF has been reached while reading the stream; see langword="true"/> otherwise.</returns>
public static bool SkipWhitespaceAndComments(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 +32,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 +47,29 @@ 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)
/// <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 ReadDecimal(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 +78,6 @@ public static int ReadDecimal(this BufferedReadStream stream)
value = (value * 10) + current;
}

return value;
return true;
}
}
24 changes: 18 additions & 6 deletions src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs
View file
Open in desktop
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 @@ -144,14 +145,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.SkipWhitespaceAndComments() ||
Copy link
Member

Choose a reason for hiding this comment

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

I really like this!

Copy link
Member

Choose a reason for hiding this comment

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

Only thing I would suggest is to change the method names to use Try prefix to match convention.

Copy link
Member

Choose a reason for hiding this comment

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

I've pushed those changes.

Copy link
Member

Choose a reason for hiding this comment

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

Ha, didn't realize you'd explicitly removed them! I can revert at a later time if you feel strongly about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I really prefer it to be the other way.

By convention, TryDoSomething(...) == false should indicate that the operation failed as a whole. This is not the case with TryReadDecimal(out value) == false when it happens at the very las digit in the file: the decoding of the digit was succesful (we should threat the result as valid!) just the file reached an EOF.

I like following such conventions to the letter, because ambiguity can lead to a misunderstandings and programming mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

I’m not precious about it (and happy to revert) but there’s something a little clunky about the method there. We’re really returning two things, the decimal and the stream state. I’ve seen something in the runtime I’m sure that handles it better (Maybe inside guid parsing code)

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered returning a (bool, int) tuple, but that would lead to much more complicated code on the callsites, since we couldn't do !stream.SkipWhitespaceAndComments() || .... Defining the names without the Try prefix + documenting the behavior was the best idea I was able to come up with.

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

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

if (this.maxPixelValue > 255)
{
this.componentType = PbmComponentType.Short;
Expand All @@ -174,6 +183,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