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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,18 @@ public sealed partial class Graphics : MarshalByRefObject, IDisposable, IDeviceC
private bool disposed;
private static float defDpiX;
private static float defDpiY;
private Metafile.MetafileHolder? _metafileHolder;

internal Graphics(IntPtr nativeGraphics) => NativeGraphics = nativeGraphics;

internal Graphics(IntPtr nativeGraphics, Image image) : this(nativeGraphics)
{
if (image is Metafile mf)
{
_metafileHolder = mf.AddMetafileHolder();
}
}

~Graphics()
{
Dispose();
Expand Down Expand Up @@ -226,6 +235,14 @@ public void Dispose()
status = Gdip.GdipDeleteGraphics(new HandleRef(this, NativeGraphics));
NativeGraphics = IntPtr.Zero;
Gdip.CheckStatus(status);

if (_metafileHolder != null)
{
var mh = _metafileHolder;
_metafileHolder = null;
mh.GraphicsDisposed();
}

disposed = true;
}

Expand Down Expand Up @@ -488,7 +505,7 @@ public static Graphics FromImage(Image image)

int status = Gdip.GdipGetImageGraphicsContext(image.nativeImage, out graphics);
Gdip.CheckStatus(status);
Graphics result = new Graphics(graphics);
Graphics result = new Graphics(graphics, image);

Rectangle rect = new Rectangle(0, 0, image.Width, image.Height);
Gdip.GdipSetVisibleClip_linux(result.NativeGraphics, ref rect);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
using System.IO;
using System.Reflection;
using System.ComponentModel;
using System.Diagnostics;
using System.Runtime.InteropServices;
using Gdip = System.Drawing.SafeNativeMethods.Gdip;
using System.Runtime.Serialization;
Expand All @@ -43,6 +44,93 @@ namespace System.Drawing.Imaging
{
public sealed partial class Metafile : Image
{
// Non-null if a graphics instance was created using
// Graphics.FromImage(this) The metadata holder is responsible for
// freeing the nativeImage if the Metadata instance is disposed before
// the Graphics instance.
private MetafileHolder? _metafileHolder;

// A class responsible for disposing of the native Metafile instance
// if it needs to outlive the managed Metafile instance.
//
// The following are both legal with win32 GDI+:
// Metafile mf = ...; // get a metafile instance
// Graphics g = Graphics.FromImage(mf); // get a graphics instance
// g.Dispose(); mf.Dispose(); // dispose of the graphics instance first
// OR
// mf.Dispose(); g.Dispose(); // dispose of the metafile instance first
//
// ligbgdiplus has a bug where disposing of the metafile instance first will
// trigger a use of freed memory when the graphics instance is disposed, which
// could lead to crashes when the native memory is reused.
//
// The metafile holder is designed to take ownership of the native metafile image
// when the managed Metafile instance is disposed while a Graphics instance is still
// not disposed (ie the second code pattern above) and to keep the native image alive until the graphics
// instance is disposed.
//
// Note that the following throws, so we only ever need to keep track of one Graphics
// instance at a time:
// 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.

{
private bool _disposed;
private IntPtr _nativeImage;


internal bool Disposed { get => _disposed; }
internal MetafileHolder()
{
_disposed = false;
_nativeImage = IntPtr.Zero;
}

~MetafileHolder() => Dispose(false);

public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}

internal void Dispose(bool disposing)
{
if (!_disposed)
{
IntPtr nativeImage = _nativeImage;
_nativeImage = IntPtr.Zero;
_disposed = true;
if (nativeImage != IntPtr.Zero)
{
int status = Gdip.GdipDisposeImage(nativeImage);
Gdip.CheckStatus(status);
}
}
}

internal void MetafileDisposed(IntPtr nativeImage)
{
_nativeImage = nativeImage;
}

internal void GraphicsDisposed()
{
Dispose();
}
}

internal MetafileHolder? AddMetafileHolder()
{
// If _metafileHolder is not null and hasn't been disposed yet, there's already a graphics instance associated with
// this metafile, the native code will return an error status.
if (_metafileHolder != null && !_metafileHolder.Disposed)
return null;
_metafileHolder = new MetafileHolder();
return _metafileHolder;
}

// Usually called when cloning images that need to have
// not only the handle saved, but also the underlying stream
// (when using MS GDI+ and IStream we must ensure the stream stays alive for all the life of the Image)
Expand Down Expand Up @@ -143,6 +231,21 @@ public Metafile(string fileName, IntPtr referenceHdc, Rectangle frameRect, Metaf
Gdip.CheckStatus(status);
}

protected override void Dispose(bool disposing)
{
if (_metafileHolder != null && !_metafileHolder.Disposed)
{
// There's a graphics instance created from this Metafile,
// transfer responsibility for disposing the nativeImage to the
// MetafileHolder
_metafileHolder.MetafileDisposed(nativeImage);
_metafileHolder = null;
nativeImage = IntPtr.Zero;
}
lambdageek marked this conversation as resolved.
Show resolved Hide resolved

base.Dispose(disposing);
}

// methods

public IntPtr GetHenhmetafile()
Expand Down