Jpeg unhandled System.ArgumentException with malformed image

[br3aker]

Prerequisites

• I have written a descriptive issue title
• I have verified that I am running the latest version of ImageSharp
• I have verified if the problem exist in both DEBUG and RELEASE mode
• I have searched open and closed issues to ensure it has not already been reported

Description

I've modified valid image from repo image base with actually itu spec complient numbers. I've changed component count from SOF segment from 3 to 255 and modified marker 'length' accordingly. Current master throws System.ArgumentException:

Unhandled exception. System.ArgumentException: Offset and length were out of bounds for the array or count is greater than the number of elements from index to the end of the source collection.
   at System.Buffer.BlockCopy(Array src, Int32 srcOffset, Array dst, Int32 dstOffset, Int32 count)
   at SixLabors.ImageSharp.Formats.Jpeg.JpegDecoderCore.ProcessStartOfFrameMarker(BufferedReadStream stream, Int32 remaining, JpegFileMarker& frameMarker, Boolean metadataOnly) in D:\Programming\C#\ImageSharp\src\ImageSharp\Formats\Jpeg\JpegDecoderCore.cs:line 1013
   at SixLabors.ImageSharp.Formats.Jpeg.JpegDecoderCore.ParseStream(BufferedReadStream stream, HuffmanScanDecoder scanDecoder, CancellationToken cancellationToken) in D:\Programming\C#\ImageSharp\src\ImageSharp\Formats\Jpeg\JpegDecoderCore.cs:line 312
   at SixLabors.ImageSharp.Formats.Jpeg.JpegDecoderCore.Decode[TPixel](BufferedReadStream stream, CancellationToken cancellationToken) in D:\Programming\C#\ImageSharp\src\ImageSharp\Formats\Jpeg\JpegDecoderCore.cs:line 183
   at SixLabors.ImageSharp.Formats.ImageDecoderUtilities.Decode[TPixel](IImageDecoderInternals decoder, Configuration configuration, Stream stream, Func`3 largeImageExceptionFactory) in D:\Programming\C#\ImageSharp\src\ImageSharp\Formats\ImageDecoderUtilities.cs:line 162
   at SixLabors.ImageSharp.Formats.ImageDecoderUtilities.Decode[TPixel](IImageDecoderInternals decoder, Configuration configuration, Stream stream) in D:\Programming\C#\ImageSharp\src\ImageSharp\Formats\ImageDecoderUtilities.cs:line 149
   at SixLabors.ImageSharp.Formats.Jpeg.JpegDecoder.Decode[TPixel](Configuration configuration, Stream stream) in D:\Programming\C#\ImageSharp\src\ImageSharp\Formats\Jpeg\JpegDecoder.cs:line 26

// Image.Load call site

libjpeg-turbo output:

Corrupt JPEG data: 3016 extraneous bytes before marker 0xd9
Invalid JPEG file structure: missing SOS marker

Steps to Reproduce

Image.Load(...) on this image:

jpeg_components255.zip

How to fix

Source of the problem is that 255 component SOF marker would occupy 8 + 255 * 3 == 8 + 765 bytes, 2 of them are 0xFF SOFn marker bytes, 6 of them are 'constant sized' and are read as 6 bytes package. Remaining 765 bytes are variable and depend on component count which is 255 in this case, they are read into intermediate buffer:

ImageSharp/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

Line 39 in ecd3db2

private readonly byte[] temp = new byte[2 * 16 * 4];

in one go:

ImageSharp/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

Line 1013 in ecd3db2

stream.Read(this.temp, 0, remaining);

So decoder writes 765 bytes into 128 byte-sized buffer. We can enlarge buffer to handle up to 255 components but do we need to? I'm yet to see a JPEG color space with 255 dimensions. And even if such color space exists - right now decoder can handle up to 4 components color spaces (CMYK) so should we bother?

[JimBobSquarePants]

I've modified valid image from repo image base with actually itu spec complient numbers

I'm a little confused @br3aker Does this mean that the values you modified within the sample image are still ITU compliant but ourselves and libjpeg-turbo both throw?

[antonfirsov]

If I get it right, the bug is that we throw ArgumentException instead of InvalidImageContentException. @br3aker is this correct?

[br3aker]

Technically, specification doesn't say anything about color spaces:

The interchange format does not specify a complete coded image representation. Application-dependent information,
e.g. colour space, is outside the scope of this Specification,

They've decided to save color channel count into single byte without any constraints. Here's the SOF header segment structure, Nf variable is the number of channels in the image:

Maybe back in the day jpeg decided to give some flexibility allowing image to contain up to 255 channels. But we, libjpeg/libjpeg-turbo and most likely any other jpeg decoder on the planet only have 1/3/4 color channels converters. In fact, we even throw if image have 2 or >4 channels:

ImageSharp/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

Line 475 in e20410c

JpegThrowHelper.ThrowInvalidImageContentException($"Unsupported color mode. Supported component counts 1, 3, and 4; found {componentCount}");

The only problem right now as we read possibly more bytes (255 * 3) than our buffer can handle (128) thus System.ArgumentException from some .net internal calls. @antonfirsov we should throw but not 'InvalidImageContent' imo, it's valid according to the spec, it's just not supported by ImageSharp. Maybe one day we'll get 5 channel color, in fact, we already have 2 channel color space. Yes, nobody uses it but it's still possible and jpeg format allows that.

My take is that we should throw NotSupportedException either manually from SOF marker parsing method before reading the stream here:

ImageSharp/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

Lines 1012 to 1013 in e20410c

	// components*3 bytes: component data
	stream.Read(this.temp, 0, remaining);

Or move this code from end of the SOF parser method right before stream read from above because it checks for 1/3/4 color channels:

ImageSharp/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

Lines 1072 to 1073 in e20410c

	this.ColorSpace = this.DeduceJpegColorSpace(componentCount);
	this.Metadata.GetJpegMetadata().ColorType = this.DeduceJpegColorType();

[antonfirsov]

NotSupportedException is good enough for me, since seems to more or less match our current documentation:

ImageSharp/src/ImageSharp/Image.FromStream.cs

Lines 272 to 280 in e20410c

	/// <param name="stream">The stream containing image information.</param>
	/// <param name="decoder">The decoder.</param>
	/// <exception cref="ArgumentNullException">The stream is null.</exception>
	/// <exception cref="ArgumentNullException">The decoder is null.</exception>
	/// <exception cref="NotSupportedException">The stream is not readable or the image format is not supported.</exception>
	/// <exception cref="UnknownImageFormatException">Image format not recognised.</exception>
	/// <exception cref="InvalidImageContentException">Image contains invalid content.</exception>
	/// <returns>The <see cref="Image"/>.</returns>
	public static Image Load(Stream stream, IImageDecoder decoder)

Though I think it would be better if all exceptions thrown based on the stream content would derive from the same base type. I wonder which are other places throwing NotSupportedException?

[br3aker]

Didn't thought about actual exception handling before writing reply. Yep, one base exception is better in terms of quality of life for the end users.

Jpeg throws NotSupportedExceptions for any non-supported but valid headers:

ImageSharp/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

Lines 315 to 335 in 39f37d0

	case JpegConstants.Markers.SOF5:
	JpegThrowHelper.ThrowNotSupportedException("Decoding jpeg files with differential sequential DCT is not supported.");
	break;
	case JpegConstants.Markers.SOF6:
	JpegThrowHelper.ThrowNotSupportedException("Decoding jpeg files with differential progressive DCT is not supported.");
	break;
	case JpegConstants.Markers.SOF3:
	case JpegConstants.Markers.SOF7:
	JpegThrowHelper.ThrowNotSupportedException("Decoding lossless jpeg files is not supported.");
	break;
	case JpegConstants.Markers.SOF9:
	case JpegConstants.Markers.SOF10:
	case JpegConstants.Markers.SOF11:
	case JpegConstants.Markers.SOF13:
	case JpegConstants.Markers.SOF14:
	case JpegConstants.Markers.SOF15:
	JpegThrowHelper.ThrowNotSupportedException("Decoding jpeg files with arithmetic coding is not supported.");
	break;

We can alter and revise all of the InvalidImageContent occurances but it's too much work for actually no gain. I believe most of the users don't care if we throw either Invalid or NotSupported exceptions, all they care is the actual exception occurance:

try { /* Load image */ }
// okay, we have unparsable image - log something or even do some UI stuff
catch(InvalidImageContentException ex) {...}
// yet another type of unparsable image, same semantics as above so is it worth it?
catch(NotSupportedException ex) {...} 

[JimBobSquarePants]

InvalidImageContentException has a very specific use case (malformed or non spec) so NotSupportedException is definitely the best approach here. I'd leave everything else untouched for now.