Access Violation in 2.1, works fine in 1.04

[alex-jitbit]

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

ImageSharp version

2.1.0

Other ImageSharp packages and versions

none

Environment (Operating system, version and so on)

Windows 2016

.NET Framework version

6.0

Description

We have an ASP.NET 6 app that uses ImageSharp 2.1 to resize images and create thumbnails (and that's it) for UGC. Several times a day, during high-load, our app crashes with this error:

Application: w3wp.exe
CoreCLR Version: 6.0.322.12309
.NET Version: 6.0.3
Description: The process was terminated due to an internal error in the .NET Runtime at IP 00007FF932A2A5A9 (00007FF932940000) with exit code 80131506.

It's very simple code. But this uncatchable error crashes our entire process.

Reverting back to 1.0.4 seems to help.

Steps to Reproduce

Unfortunately I'm unable to reliably tell which images caused this, since it's a busy app with millions of HTTP requests. Also I had no time to create process dumps or anything, But I did narrow this down to ImageSharp - reverting nuget from 2.1 to 1.0.4 fixed the issue immediately. So I thought you should know.

PS. Also, posting this is to help someone who's going to google for a solution desperately

Images

No response

[JimBobSquarePants]

Thanks @alex-jitbit for raising this.

However, without either a stack trace, dump or image we’re absolutely powerless to do anything about it. You’ll need to provide means to replicate the issue or submit a patch.

[alex-jitbit]

@JimBobSquarePants 100% understand. I will try to set up automated crash-dump saving, and then let it run on 2.1 for a while. If I won't be able to provide more details feel free to close.

[JimBobSquarePants]

Thanks. We definitely won’t be closing this issue. It’s top priority to avoid memory access violations.

[JimBobSquarePants]

This may or may not be related but I have noticed we're missing a sanitation check here.

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

Lines 723 to 734 in 136cc14

	if (ProfileResolver.IsProfile(this.temp, ProfileResolver.XmpMarker.Slice(0, ExifMarkerLength)))
	{
	int remainingXmpMarkerBytes = XmpMarkerLength - ExifMarkerLength;
	stream.Read(this.temp, ExifMarkerLength, remainingXmpMarkerBytes);
	remaining -= remainingXmpMarkerBytes;
	if (ProfileResolver.IsProfile(this.temp, ProfileResolver.XmpMarker))
	{
	this.hasXmp = true;
	byte[] profile = new byte[remaining];
	stream.Read(profile, 0, remaining);
	if (this.xmpData is null)

We really should be checking for a valid length before reading despite how rare this should be.

if (ProfileResolver.IsProfile(this.temp, ProfileResolver.XmpMarker.Slice(0, ExifMarkerLength)))
{
    const int remainingXmpMarkerBytes = XmpMarkerLength - ExifMarkerLength;
    if (remaining < remainingXmpMarkerBytes || this.IgnoreMetadata)
    {
        // Skip the application header length.
        stream.Skip(remaining);
        return;
    }

    stream.Read(this.temp, ExifMarkerLength, remainingXmpMarkerBytes);

[alex-jitbit]

@JimBobSquarePants thanks! Will try my best to dig some more debug info. I completely understand.

JFYI, this is the code we use, like I said it's very basic

using (Image srcBmp = Image.Load(imgData)) //"imgData" is a MemoryStream
{
	//omitted some custom code that does not use ImageSharp (calculating target dimensions)
	//...
	
	srcBmp.Mutate(x => x.Resize(newWidth, newHeight));
	
	//sometimes we use this instead of the above
	srcBmp.Mutate(x => x.Resize(new ResizeOptions { Mode = ResizeMode.Pad, Position = AnchorPositionMode.Center, Size = new Size(newWidth, newHeight), Sampler = KnownResamplers.NearestNeighbor }));

	using (var resultMs = new MemoryStream())
	{
		srcBmp.Save(resultMs, IsJpeg(imgData) ? new JpegEncoder() : new PngEncoder());
		//"IsJpeg" is our custom function that uses "magic numbers" to tell JPG from everything else
	}
}

[tocsoft]

@alex-jitbit unrelated to the bug reported but thought you would like to know but there is an overload of Image.Load(imgData, out IImageFormat imageFormat) that returns the format of the image loaded, saves you having to process the imgData stream twice as we have already done exactly that during load. Also there is an overload/extension method on Image for saving that takes in an IImageFormat so you can call srcBmp.Save(resultMs, imageFormat); to save in the same format as the source image and avoid create a new encoder instance on each call. (at the very least you want to make them singletons, there is no need to create a new one every image).

[JimBobSquarePants]

@alex-jitbit any update on means of replicating? We’re blocked from any further development of the libraries until we get this sorted.

[alex-jitbit]

@JimBobSquarePants no, sorry, I was prohibited from setting up the automated crash dumps on production and can't replicate this on staging or local dev machine. I completely understand this issue is useless without STR, so feel free to close

[JimBobSquarePants]

Not even a try… catch around the relevant code to identify a problematic image? You don’t need a process dump.

[alex-jitbit]

@JimBobSquarePants we do have a try-catch around it, but no exception is thrown.

[JimBobSquarePants]

Damn.... That's nasty! And you definitely have the same version deployed on both staging and production with the same images?

[br3aker]

@alex-jitbit corrupted state exceptions are not caught by try-catch by default, 'access violation' is one of them. Unfortunately, dotnet6 dropped support of the CSE exception handling but you can use this event: AppDomain.UnhandledException.

Problem is that process crash can be caused by various exceptions, so it's better to confirm exact exception type before watching for potential access violations.