System.Drawing.Common throws System.NotSupportedException: Stream does not support reading

[soyang2828]

It looks like version from netcoreapp2.0 works but one from netcoreapp3.0 doesn't.
I was told to use one from netcoreapp3.1, but there isn't such folder in the nuget package.
The callstack from our repro:

System.NotSupportedException: Stream does not support reading.
at System.IO.StreamHelpers.ValidateCopyToArgs(Stream source, Stream destination, Int32 bufferSize)
at System.IO.Stream.CopyTo(Stream destination, Int32 bufferSize)
at System.IO.Stream.CopyTo(Stream destination)
at System.Drawing.Internal.GPStream..ctor(Stream stream, Boolean makeSeekable)
at System.Drawing.Image.Save(Stream stream, ImageCodecInfo encoder, EncoderParameters encoderParams)
at System.Drawing.Image.Save(Stream stream, ImageFormat format)
at Microsoft.Exchange.Data.Storage.OleAttachment.ConvertToImage(Stream outStream, ImageFormat format) in H:\subrepo1\src\sources\dev\data\src\storage\Attachments\OleAttachment.cs:line 263
at Microsoft.Exchange.Data.Storage.OleAttachment.TryConvertToImage(Stream outStream, ImageFormat format) in H:\subrepo1\src\sources\dev\data\src\storage\Attachments\OleAttachment.cs:line 70

[safern]

@soyang2828 could please provide more details on the repro?
I.e:
A sample code and input of the image?
What OS are you running on?
What version of System.Drawing.Common are you using?

cc: @JeremyKuhne

[safern]

I was told to use one from netcoreapp3.1

There is no netcoreapp3.1 asset because the implementation for netcoreapp3.1 is the same as for netcoreapp3.0 so it uses the one in netcoreapp3.0.

We would need to know the details asked above to be able to diagnose and understand the problem.

[soyang2828]

The ask is why it throws when I use assembly under netcoreapp3.0? (callstack in the first post)

[safern]

I'm still not understanding the issue. Could you provide with more details? If you can't share the project, code sample, etc, we can take this offline and share all that info there 😄.

I don't know why it throws, it might be a bug we introduced, but to understand more we need more info. Like the package version, a simple repro, etc.

[danmoseley]

@soyang2828 were you able to share this info (eg in email)? We would like to help -- but we need more info.

[soyang2828]

Sorry for the delay. The OS is windows 10
This repros on: system.drawing.common\4.7.0\runtimes\win\lib\netcoreapp3.0

NOT repro on: system.drawing.common\4.7.0\runtimes\win\lib\netcoreapp2.0

[safern]

Thanks, @soyang2828 -- we still need more info on the repro, a sample code and sample stream (file used to create the stream) would really help.

[soyang2828]

Hmm... This was caught in the production and it doesn't repro all the time. I vaguely remembered it crashed when tried to parse a png image file. Would you find anything suspcious from the callstacks?

[danmoseley]

Is this a Drawing bug? It seems that Microsoft.Exchange.Data.Storage.OleAttachment.ConvertToImage has called System.Drawing.Image.Save(Stream stream, ImageFormat format) passing in a stream that returns false from CanRead property -- ie., it does not support reading (only writing, presumably).

The callstack is very simple -- we are trying to read from the stream passed in, and it's not readable. @soyang2828 can you inspect the calling code and see where the stream comes from?

[ericstj]

dotnet/corefx@dee4344 likely introduced the difference.

Here's the relevant diff:
1365275#diff-110b09b5c69ec04d04ae3f8e569c757bL329-R333

So this previously just exposed the managed stream as an IStream by wrapping with ComStreamFromDataStream, which would have let GDIPlus directly call Read/Write as needed (and maybe it never called read, or it did but handled it not working). By using GPStream here and not specifying makeSeekable = false it ends up trying to copy the user's stream into a MemoryStream.
1365275#diff-928ae7e9a8b8058b941ca631131edf7dL17-L45

I think the fix here may be to change this to pass makeSeekable=false so long as that doesn't invalidate the intent of the original fix. /cc @JeremyKuhne

As @danmosemsft mentioned this is happening as a result of the Stream being passed into System.Drawing.Image.Save(Stream stream, ImageFormat format) returning CanRead as false. So if you have control of that stream you can workaround this bug by passing in a readable stream (eg: a temporary MemoryStream) and doing the copy to the backing stream yourself after calling Save.

[danmoseley]

Thanks @ericstj -- your links into hte diff above don't seem to navigate to me. Which files in this diff?

[ericstj]

Interesting, not sure what's wrong with Github's range navigation in diffs but I'm also seeing the links don't work. I'll add some snippets here.

Image.Windows.cs - line 329-333

Before change:

runtime/src/libraries/System.Drawing.Common/src/System/Drawing/Image.Windows.cs

Lines 327 to 334 in ddb5b5e

	if (!saved)
	{
	Gdip.CheckStatus(Gdip.GdipSaveImageToStream(
	new HandleRef(this, nativeImage),
	new UnsafeNativeMethods.ComStreamFromDataStream(stream),
	ref g,
	new HandleRef(encoderParams, encoderParamsMemory)));
	}

After change:

runtime/src/libraries/System.Drawing.Common/src/System/Drawing/Image.Windows.cs

Lines 242 to 249 in cf1390c

	if (!saved)
	{
	Gdip.CheckStatus(Gdip.GdipSaveImageToStream(
	new HandleRef(this, nativeImage),
	new GPStream(stream),
	ref g,
	new HandleRef(encoderParams, encoderParamsMemory)));
	}

GPStream.cs (after change)

runtime/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs

Lines 18 to 31 in cf1390c

	internal GPStream(Stream stream, bool makeSeekable = true)
	{
	if (makeSeekable && !stream.CanSeek)
	{
	// Copy to a memory stream so we can seek
	MemoryStream memoryStream = new MemoryStream();
	stream.CopyTo(memoryStream);
	_dataStream = memoryStream;
	}
	else
	{
	_dataStream = stream;
	}
	}

[ericstj]

I'll pick this up since I pretty much already understand it and the fix.

[safern]

Thanks @ericstj — nice find!

[JeremyKuhne]

@ericstj the bigger part of the change was making the interop more performant and trying to avoid unnecessary allocations (in addition to fixing bugs). This call path looks to be an oversight on my part. 🙉 Sort of silly to make a copy of something you're trying to copy into.

[ericstj]

Reopening to track servicing fix.

[danmoseley]

@ericstj this can be closed now the port merged, right?

[safern]

Should we port this to release/5.0 as well? It seems odd that the 4.7.* package has the fix, but the 5.0.* package doesn't

[ericstj]

@ericstj this can be closed now the port merged, right?

Yep!

Should we port this to release/5.0

This was a backport of a fix already in 5.0: #36805

[safern]

This was a backport of a fix already in 5.0: #36805

Ah, I missed that this was before we branched off for release/5.0