Skip to content

Commit 7939172

Browse files
lambdageeksafern
lambdageek
and
safern
authored
[System.Drawing.Common] Work around libgdiplus use after free (#43074)
* [System.Drawing.Common] Work around libgdiplus use after free 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 * Formatting fixes Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com> * Address review feedback * Inilne unhelpful helper * formatting Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com>
1 parent 789845f commit 7939172

File tree

2 files changed

+121
-1
lines changed

2 files changed

+121
-1
lines changed

‎src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,18 @@ public sealed partial class Graphics : MarshalByRefObject, IDisposable, IDeviceC
5050
private bool disposed;
5151
private static float defDpiX;
5252
private static float defDpiY;
53+
private Metafile.MetafileHolder? _metafileHolder;
5354

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

57+
internal Graphics(IntPtr nativeGraphics, Image image) : this(nativeGraphics)
58+
{
59+
if (image is Metafile mf)
60+
{
61+
_metafileHolder = mf.AddMetafileHolder();
62+
}
63+
}
64+
5665
~Graphics()
5766
{
5867
Dispose();
@@ -226,6 +235,14 @@ public void Dispose()
226235
status = Gdip.GdipDeleteGraphics(new HandleRef(this, NativeGraphics));
227236
NativeGraphics = IntPtr.Zero;
228237
Gdip.CheckStatus(status);
238+
239+
if (_metafileHolder != null)
240+
{
241+
var mh = _metafileHolder;
242+
_metafileHolder = null;
243+
mh.GraphicsDisposed();
244+
}
245+
229246
disposed = true;
230247
}
231248

@@ -488,7 +505,7 @@ public static Graphics FromImage(Image image)
488505

489506
int status = Gdip.GdipGetImageGraphicsContext(image.nativeImage, out graphics);
490507
Gdip.CheckStatus(status);
491-
Graphics result = new Graphics(graphics);
508+
Graphics result = new Graphics(graphics, image);
492509

493510
Rectangle rect = new Rectangle(0, 0, image.Width, image.Height);
494511
Gdip.GdipSetVisibleClip_linux(result.NativeGraphics, ref rect);

‎src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
using System.IO;
3636
using System.Reflection;
3737
using System.ComponentModel;
38+
using System.Diagnostics;
3839
using System.Runtime.InteropServices;
3940
using Gdip = System.Drawing.SafeNativeMethods.Gdip;
4041
using System.Runtime.Serialization;
@@ -43,6 +44,93 @@ namespace System.Drawing.Imaging
4344
{
4445
public sealed partial class Metafile : Image
4546
{
47+
// Non-null if a graphics instance was created using
48+
// Graphics.FromImage(this) The metadata holder is responsible for
49+
// freeing the nativeImage if the Metadata instance is disposed before
50+
// the Graphics instance.
51+
private MetafileHolder? _metafileHolder;
52+
53+
// A class responsible for disposing of the native Metafile instance
54+
// if it needs to outlive the managed Metafile instance.
55+
//
56+
// The following are both legal with win32 GDI+:
57+
// Metafile mf = ...; // get a metafile instance
58+
// Graphics g = Graphics.FromImage(mf); // get a graphics instance
59+
// g.Dispose(); mf.Dispose(); // dispose of the graphics instance first
60+
// OR
61+
// mf.Dispose(); g.Dispose(); // dispose of the metafile instance first
62+
//
63+
// ligbgdiplus has a bug where disposing of the metafile instance first will
64+
// trigger a use of freed memory when the graphics instance is disposed, which
65+
// could lead to crashes when the native memory is reused.
66+
//
67+
// The metafile holder is designed to take ownership of the native metafile image
68+
// when the managed Metafile instance is disposed while a Graphics instance is still
69+
// not disposed (ie the second code pattern above) and to keep the native image alive until the graphics
70+
// instance is disposed.
71+
//
72+
// Note that the following throws, so we only ever need to keep track of one Graphics
73+
// instance at a time:
74+
// Metafile mf = ...; // get a metafile instance
75+
// Graphics g = Graphics.FromImage(mf);
76+
// Graphics g2 = Graphics.FromImage(mf); // throws OutOfMemoryException on GDI+ on Win32
77+
internal sealed class MetafileHolder : IDisposable
78+
{
79+
private bool _disposed;
80+
private IntPtr _nativeImage;
81+
82+
83+
internal bool Disposed { get => _disposed; }
84+
internal MetafileHolder()
85+
{
86+
_disposed = false;
87+
_nativeImage = IntPtr.Zero;
88+
}
89+
90+
~MetafileHolder() => Dispose(false);
91+
92+
public void Dispose()
93+
{
94+
Dispose(true);
95+
GC.SuppressFinalize(this);
96+
}
97+
98+
internal void Dispose(bool disposing)
99+
{
100+
if (!_disposed)
101+
{
102+
IntPtr nativeImage = _nativeImage;
103+
_nativeImage = IntPtr.Zero;
104+
_disposed = true;
105+
if (nativeImage != IntPtr.Zero)
106+
{
107+
int status = Gdip.GdipDisposeImage(nativeImage);
108+
Gdip.CheckStatus(status);
109+
}
110+
}
111+
}
112+
113+
internal void MetafileDisposed(IntPtr nativeImage)
114+
{
115+
_nativeImage = nativeImage;
116+
}
117+
118+
internal void GraphicsDisposed()
119+
{
120+
Dispose();
121+
}
122+
}
123+
124+
internal MetafileHolder? AddMetafileHolder()
125+
{
126+
// If _metafileHolder is not null and hasn't been disposed yet, there's already a graphics instance associated with
127+
// this metafile, the native code will return an error status.
128+
if (_metafileHolder != null && !_metafileHolder.Disposed)
129+
return null;
130+
_metafileHolder = new MetafileHolder();
131+
return _metafileHolder;
132+
}
133+
46134
// Usually called when cloning images that need to have
47135
// not only the handle saved, but also the underlying stream
48136
// (when using MS GDI+ and IStream we must ensure the stream stays alive for all the life of the Image)
@@ -143,6 +231,21 @@ public Metafile(string fileName, IntPtr referenceHdc, Rectangle frameRect, Metaf
143231
Gdip.CheckStatus(status);
144232
}
145233

234+
protected override void Dispose(bool disposing)
235+
{
236+
if (_metafileHolder != null && !_metafileHolder.Disposed)
237+
{
238+
// There's a graphics instance created from this Metafile,
239+
// transfer responsibility for disposing the nativeImage to the
240+
// MetafileHolder
241+
_metafileHolder.MetafileDisposed(nativeImage);
242+
_metafileHolder = null;
243+
nativeImage = IntPtr.Zero;
244+
}
245+
246+
base.Dispose(disposing);
247+
}
248+
146249
// methods
147250

148251
public IntPtr GetHenhmetafile()

0 commit comments

Comments
 (0)