From 48c2b9e18c6a54766e851f595696c1b5799b0c26 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 5 Oct 2020 22:51:59 -0400 Subject: [PATCH 1/5] [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 https://github.com/dotnet/runtime/issues/37838 --- .../src/System/Drawing/Graphics.Unix.cs | 19 +++- .../System/Drawing/Imaging/Metafile.Unix.cs | 104 ++++++++++++++++++ 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs index 271da443ce082..b70802cd144af 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs @@ -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(this); + } + } + ~Graphics() { Dispose(); @@ -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; } @@ -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); diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs index 53aaca69fd4be..e5bb802b765e5 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs @@ -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; @@ -43,6 +44,95 @@ 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 + { + 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) + return; + IntPtr nativeImage = _nativeImage; + _nativeImage = IntPtr.Zero; + disposed = true; + if (nativeImage != IntPtr.Zero) + Metafile.DisposeNativeInstance (nativeImage); + } + + internal void MetafileDisposed (IntPtr nativeImage) + { + _nativeImage = nativeImage; + } + + internal void GraphicsDisposed () + { + Dispose(); + } + } + + internal MetafileHolder? AddMetafileHolder(Graphics g) + { + // 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; + } + + internal static void DisposeNativeInstance(IntPtr nativeImage) + { + int status = Gdip.GdipDisposeImage(nativeImage); + Gdip.CheckStatus(status); + } + // 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) @@ -143,6 +233,20 @@ 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; + } + base.Dispose (disposing); + } + // methods public IntPtr GetHenhmetafile() From 1e11fbf103daec3de1295fe165eb66008c89dcef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Tue, 6 Oct 2020 14:35:18 -0400 Subject: [PATCH 2/5] Formatting fixes Co-authored-by: Santiago Fernandez Madero --- .../System/Drawing/Imaging/Metafile.Unix.cs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs index e5bb802b765e5..4dfd8347eb30f 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs @@ -81,13 +81,13 @@ internal sealed class MetafileHolder : IDisposable internal bool Disposed { get => disposed; } - internal MetafileHolder () + internal MetafileHolder() { disposed = false; _nativeImage = IntPtr.Zero; } - ~MetafileHolder () => Dispose(false); + ~MetafileHolder() => Dispose(false); public void Dispose() { @@ -97,21 +97,22 @@ public void Dispose() internal void Dispose(bool disposing) { - if (disposed) - return; - IntPtr nativeImage = _nativeImage; - _nativeImage = IntPtr.Zero; - disposed = true; - if (nativeImage != IntPtr.Zero) - Metafile.DisposeNativeInstance (nativeImage); + if (!disposed) + { + IntPtr nativeImage = _nativeImage; + _nativeImage = IntPtr.Zero; + disposed = true; + if (nativeImage != IntPtr.Zero) + Metafile.DisposeNativeInstance(nativeImage); + } } - internal void MetafileDisposed (IntPtr nativeImage) + internal void MetafileDisposed(IntPtr nativeImage) { _nativeImage = nativeImage; } - internal void GraphicsDisposed () + internal void GraphicsDisposed() { Dispose(); } @@ -240,11 +241,11 @@ protected override void Dispose(bool disposing) // There's a graphics instance created from this Metafile, // transfer responsibility for disposing the nativeImage to the // MetafileHolder - _metafileHolder.MetafileDisposed (nativeImage); + _metafileHolder.MetafileDisposed(nativeImage); _metafileHolder = null; nativeImage = IntPtr.Zero; } - base.Dispose (disposing); + base.Dispose(disposing); } // methods From 3c97aeda10c8d026b9098e4ec7353552476eb287 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 6 Oct 2020 14:40:14 -0400 Subject: [PATCH 3/5] Address review feedback --- .../src/System/Drawing/Graphics.Unix.cs | 2 +- .../src/System/Drawing/Imaging/Metafile.Unix.cs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs index b70802cd144af..a8492d7ffcab7 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Unix.cs @@ -58,7 +58,7 @@ internal Graphics(IntPtr nativeGraphics, Image image) : this(nativeGraphics) { if (image is Metafile mf) { - _metafileHolder = mf.AddMetafileHolder(this); + _metafileHolder = mf.AddMetafileHolder(); } } diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs index 4dfd8347eb30f..8e15509ccc5a0 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs @@ -76,14 +76,14 @@ public sealed partial class Metafile : Image // Graphics g2 = Graphics.FromImage(mf); // throws OutOfMemoryException on GDI+ on Win32 internal sealed class MetafileHolder : IDisposable { - private bool disposed; + private bool _disposed; private IntPtr _nativeImage; - internal bool Disposed { get => disposed; } + internal bool Disposed { get => _disposed; } internal MetafileHolder() { - disposed = false; + _disposed = false; _nativeImage = IntPtr.Zero; } @@ -97,11 +97,11 @@ public void Dispose() internal void Dispose(bool disposing) { - if (!disposed) + if (!_disposed) { IntPtr nativeImage = _nativeImage; _nativeImage = IntPtr.Zero; - disposed = true; + _disposed = true; if (nativeImage != IntPtr.Zero) Metafile.DisposeNativeInstance(nativeImage); } @@ -118,7 +118,7 @@ internal void GraphicsDisposed() } } - internal MetafileHolder? AddMetafileHolder(Graphics g) + 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. From d68f10ca46e3d8cb62b3f1e39e1263819e4d87ab Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 6 Oct 2020 14:47:50 -0400 Subject: [PATCH 4/5] Inilne unhelpful helper --- .../src/System/Drawing/Imaging/Metafile.Unix.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs index 8e15509ccc5a0..2b1059afd52da 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs @@ -103,7 +103,10 @@ internal void Dispose(bool disposing) _nativeImage = IntPtr.Zero; _disposed = true; if (nativeImage != IntPtr.Zero) - Metafile.DisposeNativeInstance(nativeImage); + { + int status = Gdip.GdipDisposeImage(nativeImage); + Gdip.CheckStatus(status); + } } } @@ -128,12 +131,6 @@ internal void GraphicsDisposed() return _metafileHolder; } - internal static void DisposeNativeInstance(IntPtr nativeImage) - { - int status = Gdip.GdipDisposeImage(nativeImage); - Gdip.CheckStatus(status); - } - // 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) From d8bee9aca9fa24f6b0bc72a4487ddb479fa0c83c Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 6 Oct 2020 14:50:22 -0400 Subject: [PATCH 5/5] formatting --- .../src/System/Drawing/Imaging/Metafile.Unix.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs index 2b1059afd52da..361565d3f1664 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Metafile.Unix.cs @@ -107,7 +107,7 @@ internal void Dispose(bool disposing) int status = Gdip.GdipDisposeImage(nativeImage); Gdip.CheckStatus(status); } - } + } } internal void MetafileDisposed(IntPtr nativeImage) @@ -242,6 +242,7 @@ protected override void Dispose(bool disposing) _metafileHolder = null; nativeImage = IntPtr.Zero; } + base.Dispose(disposing); }