Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[System.Drawing.Common] Work around libgdiplus use after free #43074

Merged
merged 5 commits into from
Oct 8, 2020

Conversation

lambdageek
Copy link
Member

On Windows, both of the following are legal

Metafile mf = ... ; // get a metafile instance
Graphics g = Graphics.FromImage(mf);
g.Dispose(); mf.Dispose();

and

Metafile mf = ... ; // get a metafile instance
Graphics g = Graphics.FromImage(mf);
mf.Dispose(); g.Dispose();

On Unix, libgdiplus has a use after free bug for the second form - the metafile
native image is disposed, but the graphics instance still has a pointer to the
memory that it will use during cleanup. If the memory is reused, the graphics
instance will see garbage values and crash.

The workaround is to add a MetadataHolder class and to transfer responsibility
for disposing of the native image instance to it if the Metafile is disposed
before the Graphics.

Note that the following is not allowed (throws OutOfMemoryException on GDI+ on
Windows), so there's only ever one instance of Graphics associated with a
Metafile at a time.

Graphics g = Graphics.FromImage(mf);
Graphics g2 = Graphics.FromImage(mf); // throws

Addresses #37838

@ghost
Copy link

ghost commented Oct 6, 2020

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

On Windows, both of the following are legal

    Metafile mf = ... ; // get a metafile instance
    Graphics g = Graphics.FromImage(mf);
    g.Dispose(); mf.Dispose();

and

    Metafile mf = ... ; // get a metafile instance
    Graphics g = Graphics.FromImage(mf);
    mf.Dispose(); g.Dispose();

On Unix, libgdiplus has a use after free bug for the second form - the metafile
native image is disposed, but the graphics instance still has a pointer to the
memory that it will use during cleanup.  If the memory is reused, the graphics
instance will see garbage values and crash.

The workaround is to add a MetadataHolder class and to transfer responsibility
for disposing of the native image instance to it if the Metafile is disposed
before the Graphics.

Note that the following is not allowed (throws OutOfMemoryException on GDI+ on
Windows), so there's only ever one instance of Graphics associated with a
Metafile at a time.

    Graphics g = Graphics.FromImage(mf);
    Graphics g2 = Graphics.FromImage(mf); // throws

Addresses dotnet#37838
@ghost
Copy link

ghost commented Oct 6, 2020

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

@lambdageek
Copy link
Member Author

One thing I'm not sure about - is it worthwhile to make Metafile.MetafileHolder a subclass of SafeHandle. On construction I'd bump the refcount to 2 but leave the handle as invalid, then MetafileDispose would call SetHandle and SafeHandle.Dispose, and GraphicsDispose would also call SafeHandle.Dispose.

One reason not to do it might be because the rest of the SafeHandle methods aren't really meaningful to MetafileHolder - it has quite a constrained API surface. I wish SafeHandle was a concrete class with a delegate for the release method, then I could just use it as a field in MetafileHolder.

@safern
Copy link
Member

safern commented Oct 6, 2020

cc: @stephentoub

@lambdageek lambdageek marked this pull request as ready for review October 6, 2020 18:48
@lambdageek lambdageek merged commit 7939172 into dotnet:master Oct 8, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
// Metafile mf = ...; // get a metafile instance
// Graphics g = Graphics.FromImage(mf);
// Graphics g2 = Graphics.FromImage(mf); // throws OutOfMemoryException on GDI+ on Win32
internal sealed class MetafileHolder : IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own curiosity, any reason not to make it a struct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it has a finalizer. Structs can't have finalizers.

@lambdageek lambdageek deleted the fix-gh-37838-holder branch March 19, 2022 16:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants