From 715757301fca88e06e8406a4bd56f71030f1c804 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Mon, 8 Feb 2021 18:46:33 -0500 Subject: [2020-02][System.Drawing] Work around libgdiplus Metafile dispose ordering (#20828) * [System.Drawing] Work around libgdiplus Metafile dispose ordering * Bump API snapshot submodule Co-authored-by: monojenkins --- external/api-snapshot | 2 +- .../System.Drawing.Imaging/Metafile.cs | 102 +++++++++++++++++++++ .../System.Drawing/System.Drawing/Graphics.cs | 19 +++- 3 files changed, 120 insertions(+), 3 deletions(-) diff --git a/external/api-snapshot b/external/api-snapshot index e6b00a47728..808e8a9e4be 160000 --- a/external/api-snapshot +++ b/external/api-snapshot @@ -1 +1 @@ -Subproject commit e6b00a477284e92f37df1ab942a289253a495022 +Subproject commit 808e8a9e4be8d38e097d2b0919cac37bc195844a diff --git a/mcs/class/System.Drawing/System.Drawing.Imaging/Metafile.cs b/mcs/class/System.Drawing/System.Drawing.Imaging/Metafile.cs index b28654bae37..a35359cf01d 100644 --- a/mcs/class/System.Drawing/System.Drawing.Imaging/Metafile.cs +++ b/mcs/class/System.Drawing/System.Drawing.Imaging/Metafile.cs @@ -41,6 +41,93 @@ namespace System.Drawing.Imaging { [Editor ("System.Drawing.Design.MetafileEditor, " + Consts.AssemblySystem_Drawing_Design, typeof (System.Drawing.Design.UITypeEditor))] public sealed 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) + { + IntPtr nativeImage = _nativeImage; + _nativeImage = IntPtr.Zero; + _disposed = true; + if (nativeImage != IntPtr.Zero) + { + Status status = GDIPlus.GdipDisposeImage(nativeImage); + GDIPlus.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; + } + // constructors internal Metafile (IntPtr ptr) @@ -324,6 +411,21 @@ namespace System.Drawing.Imaging { GDIPlus.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(nativeObject); + _metafileHolder = null; + nativeObject = IntPtr.Zero; + } + + base.Dispose(disposing); + } + // methods public IntPtr GetHenhmetafile () diff --git a/mcs/class/System.Drawing/System.Drawing/Graphics.cs b/mcs/class/System.Drawing/System.Drawing/Graphics.cs index 5e244d47d60..967fb33715a 100644 --- a/mcs/class/System.Drawing/System.Drawing/Graphics.cs +++ b/mcs/class/System.Drawing/System.Drawing/Graphics.cs @@ -48,6 +48,7 @@ namespace System.Drawing private static float defDpiX = 0; private static float defDpiY = 0; private IntPtr deviceContextHdc; + private Metafile.MetafileHolder _metafileHolder; public delegate bool EnumerateMetafileProc (EmfPlusRecordType recordType, int flags, @@ -62,6 +63,13 @@ namespace System.Drawing nativeObject = nativeGraphics; } + internal Graphics(IntPtr nativeGraphics, Image image) : this(nativeGraphics) + { + if (image is Metafile mf) { + _metafileHolder = mf.AddMetafileHolder(); + } + } + ~Graphics () { Dispose (); @@ -297,6 +305,14 @@ namespace System.Drawing status = GDIPlus.GdipDeleteGraphics (nativeObject); nativeObject = IntPtr.Zero; GDIPlus.CheckStatus (status); + + if (_metafileHolder != null) + { + var mh = _metafileHolder; + _metafileHolder = null; + mh.GraphicsDisposed(); + } + disposed = true; } @@ -1773,8 +1789,7 @@ namespace System.Drawing Status status = GDIPlus.GdipGetImageGraphicsContext (image.nativeObject, out graphics); GDIPlus.CheckStatus (status); - Graphics result = new Graphics (graphics); - + Graphics result = new Graphics (graphics, image); if (GDIPlus.RunningOnUnix ()) { Rectangle rect = new Rectangle (0,0, image.Width, image.Height); GDIPlus.GdipSetVisibleClip_linux (result.NativeObject, ref rect); -- cgit v1.2.3