Png- Do not attempt to read data for chunks of length 0.

[JimBobSquarePants]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

Png was attempting to read chunk data for chunks of length 0, for example EOF. This lead, to multiple attempts to read past the end of a stream.

[JimBobSquarePants]

We avoid the read here because Png still has to do a CRC check which if does not occur leads to failure during reading of chained Png images within a single stream.

[antonfirsov]

The behavior with these changes is still odd: we run through the stream with ~500 attempts to read a 0-byte chunk. It doesn't look like the correct/optimal thing to do for such an input.

This code doesn't look correct to me:

ImageSharp/src/ImageSharp/Formats/Png/PngDecoderCore.cs

Lines 1470 to 1479 in d93bc6c

	while (length < 0 || length > (this.currentStream.Length - this.currentStream.Position))
	{
	// Not a valid chunk so try again until we reach a known chunk.
	if (!this.TryReadChunkLength(buffer, out length))
	{
	chunk = default;
	return false;
	}
	}

If length > (this.currentStream.Length - this.currentStream.Position) because the file is truncated, we just start interpreting the following (type/data) bytes as length until length becomes 0. If we really don't want to throw/abort here, why not to process the chunk in the truncated form instead?

Edit: I guess there was some corrupt input this was logic was introduced for but is this really the best we can do?

[JimBobSquarePants]

The behavior with these changes is still odd: we run through the stream with ~500 attempts to read a 0-byte chunk. It doesn't look like the correct/optimal thing to do for such an input.

This code doesn't look correct to me:

ImageSharp/src/ImageSharp/Formats/Png/PngDecoderCore.cs

Lines 1470 to 1479 in d93bc6c

	while (length < 0 || length > (this.currentStream.Length - this.currentStream.Position))
	{
	// Not a valid chunk so try again until we reach a known chunk.
	if (!this.TryReadChunkLength(buffer, out length))
	{
	chunk = default;
	return false;
	}
	}

If length > (this.currentStream.Length - this.currentStream.Position) because the file is truncated, we just start interpreting the following (type/data) bytes as length until length becomes 0. If we really don't want to throw/abort here, why not to process the chunk in the truncated form instead?

Edit: I guess there was some corrupt input this was logic was introduced for but is this really the best we can do?

Is it the best we can do?

We differ from libpng here. We continue reading after parsing a length greater than 2147483647 while libpng will throw.

https://github.com/glennrp/libpng/blob/f2294569cfe43f87c9f72d35545e8de1126eb4b9/pngrutil.c#L22-L30

libpng doesn't seem to do the inner check.
https://github.com/glennrp/libpng/blob/libpng16/pngrutil.c#L156-L192

However, it seems to freely loop like we do. I'm not sure there's a better way to detect chunks while skipping corrupted sections.
https://github.com/glennrp/libpng/blob/f2294569cfe43f87c9f72d35545e8de1126eb4b9/pngread.c#L106-L262

[JimBobSquarePants]

I recked our suite of tests while disabling that section and we only had the one failing image. I don't think we'll hit that path often at all and it's the only way I think that we can be skip bad sections.