[System.Drawing.Common] In test, dispose of Metafile after Graphics

[lambdageek]

Fixes #37838

Verified using libgdiplus built with
mono/libgdiplus#671

[ghost]

Tagging subscribers to this area: @safern, @tannergooding, @jeffhandley
See info in area-owners.md if you want to be subscribed.

[lambdageek]

The changes to libgdiplus aren't necessary for the fix - they just make the crash due to incorrect use more visible and deterministic

[safern]

Thanks, @lambdageek

[stephentoub]

I'm a little confused here. It sounds like the test is doing the wrong thing, but even if the test is buggy, it shouldn't be able to crash the runtime like this. Doesn't this suggest there's also a bug in either System.Drawing.Common, libgdiplus, or mono?

[stephentoub]

Oh, I see, the fix for the issue is this for the test + mono/libgdiplus#671 for the actual bug?

[stephentoub]

The changes to libgdiplus aren't necessary for the fix - they just make the crash due to incorrect use more visible and deterministic

Now I'm back to being confused ;-)

[lambdageek]

Is it a bug in libgdiplus if it's being used incorrectly? I'm not sure.

The change in libgdiplus is effectively an assertion so that you definitely crash if you use the API incorrectly. The current behavior is that you overwrite some freed memory - so you get a crash if you get unlucky.

[lambdageek]

even if the test is buggy, it shouldn't be able to crash the runtime like this

it's P/Invoking a native library that is writing over freed memory. why would we expect something other than a crash in that situation?

[stephentoub]

The test is P/Invoking? Where?

[lambdageek]

Now I'm confused.

Are there two problems here?

1. Do we agree that the test is definitely wrong? it shouldn't dispose of the Metafile instance in the scope of a Graphics that's using it? If so then this is the PR that fixes that.
2. The next question is - if it's possible for libgdiplus to detect that it's being used incorrectly, should we assert or return some error code? (I see that Image.Dispose is prepared to deal with error statuses, so
   https://github.com/dotnet/runtime/blob/master/src/libraries/System.Drawing.Common/src/System/Drawing/Image.Unix.cs#L448 maybe the right thing is to return an error code from libgdiplus)

If there's a bug to be fixed in libgdiplus, I think that's a separate issue, no?

[stephentoub]

Do we agree that the test is definitely wrong?

Not sure. There are plenty of situations where one disposable type wrapping another disposable type doesn't do anything in its Dispose except call Dispose on the wrapped instance, and Dispose is idempotent, so it's acceptable use. There are also cases where objects are ref counted, e.g. with SafeHandle, and the wrapper would have taken a ref on the wrapped thing such that disposing the wrapped thing wouldn't have actually closed the wrapped thing until after everything was disposed and released ref counts. I don't know what category these fall into nor what guarantees we've made in docs... but it seems like this works on Windows?

if it's possible for libgdiplus to detect that it's being used incorrectly

My confusion is that the APIs being used here are managed. Misuse of a managed API shouldn't cause a seg fault / crash. If it's actually misuse, worst case it should throw an exception. e.g. if I misuse FileStream, it shouldn't crash the process (even though FileStream P/Invokes as an implementation detail), worst case a use-after-dispose would produce an ObjectDisposedException. So I'm trying to understand what makes this special; it's possible there some aspect of this I'm just missing that makes it different :)

[lambdageek]

Thanks @stephentoub. Going to look for another approach (leaning toward refcounting)

[stephentoub]

Thanks, @lambdageek. Just to be clear, I wasn't trying to demand a different direction, simply trying to understand and highlight differences I see in other APIs, ask questions to better understand, etc.

[stephentoub]

(Also, if this is causing problems in CI or for devs running tests locally, we can of course disable the test until it's resolved. We can also change the test as you suggested if we believe that's more represented of typical usage; I just want to make sure we don't then lose sight of the underlying issue being worked around.)