-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Conversation
Tagging subscribers to this area: @roji, @ajcvickers |
08093c8
to
9a4e76c
Compare
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
9a4e76c
to
48c2b9e
Compare
Tagging subscribers to this area: @safern, @tannergooding, @jeffhandley |
One thing I'm not sure about - is it worthwhile to make One reason not to do it might be because the rest of the SafeHandle methods aren't really meaningful to |
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs
Show resolved
Hide resolved
cc: @stephentoub |
Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com>
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
On Windows, both of the following are legal
and
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.
Addresses #37838