Merged PR 12207: Port graphics metafile refcount fix to avoid disposi…

…ng the native image multiple times

Ports: 7939172

## Summary

Due to libgdiplus behavior we were disposing metafiles twice causing the app to crash. This adds a ref count to avoid disposing a metafile when a graphics instance still has a reference to it.

## Customer Impact

Detected via tests.

## Regression?

No.

## Testing

Unit tests.

## Risk

Low-medium.  The fix just adds a ref count to the metafile and avoids disposing it if it is still referenced by a graphics object.

NuGet.config

@@ -16,6 +16,8 @@
     <add key="dotnet-eng" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json" />
     <add key="dotnet5" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5/nuget/v3/index.json" />
     <add key="dotnet5-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5-transport/nuget/v3/index.json" />
+    <!-- Harvesting feed from 2.1 -->
+    <add key="darc-int-dotnet-corefx-43e382ec" value="https://pkgs.dev.azure.com/dnceng/internal/_packaging/darc-int-dotnet-corefx-43e382ec/nuget/v3/index.json" />
   </packageSources>
   <disabledPackageSources>
     <clear />

eng/restore/harvestPackages.targets

@@ -23,6 +23,7 @@
 
     <!-- Allow to override package download and versions in case there is already a PackageDownload set -->
     <ItemGroup>
+      <PackageDownload Include="System.Drawing.Common" Version="4.5.2" />
       <_OverridenPackageDownloads Include="@(_PackageDownload)" Condition="'@(PackageDownload)' == '@(_PackageDownload)' and %(Identity) != ''" />
       <_PackageDownload Remove="@(_OverridenPackageDownloads)" />
       <_PackageDownload Include="@(PackageDownload)" />

src/libraries/System.Drawing.Common/Directory.Build.props

@@ -4,5 +4,8 @@
     <StrongNameKeyId>Open</StrongNameKeyId>
     <IncludePlatformAttributes>true</IncludePlatformAttributes>
     <UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>
+    <PackageVersion>5.0.1</PackageVersion>
+    <AssemblyVersion>5.0.0.1</AssemblyVersion>
+    <HarvestVersion>4.5.2</HarvestVersion>
   </PropertyGroup>
 </Project>

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

@@ -49,9 +49,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();
@@ -225,6 +234,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;
             }
 
@@ -487,7 +504,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);

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

@@ -34,6 +34,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;
@@ -42,6 +43,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
+        {
+            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)
@@ -142,6 +230,21 @@ public Metafile(IntPtr hmetafile, WmfPlaceableFileHeader wmfHeader)
             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()

src/libraries/libraries-packages.proj

@@ -22,6 +22,7 @@
     <ProjectReference Include="$(LibrariesProjectRoot)\System.Speech\pkg\System.Speech.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
     <ProjectReference Include="$(LibrariesProjectRoot)\System.Diagnostics.PerformanceCounter\pkg\System.Diagnostics.PerformanceCounter.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
     <ProjectReference Include="$(LibrariesProjectRoot)\System.Diagnostics.EventLog\pkg\System.Diagnostics.EventLog.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
+    <ProjectReference Include="$(LibrariesProjectRoot)\System.Drawing.Common\pkg\System.Drawing.Common.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
     <ProjectReference Include="$(PkgDir)\Microsoft.NETCore.Platforms\Microsoft.NETCore.Platforms.proj" />
   </ItemGroup>
 

src/libraries/pkg/baseline/packageIndex.json

@@ -2862,6 +2862,7 @@
       "StableVersions": [
         "4.5.0",
         "4.5.1",
+        "4.5.2",
         "4.6.0",
         "4.6.1",
         "4.6.2",
@@ -2881,9 +2882,11 @@
       "AssemblyVersionInPackageVersion": {
         "4.0.0.0": "4.5.0",
         "4.0.0.1": "4.5.1",
+        "4.0.0.2": "4.5.2",
         "4.0.1.0": "4.6.0",
         "4.0.1.1": "4.6.1",
-        "5.0.0.0": "5.0.0"
+        "5.0.0.0": "5.0.0",
+        "5.0.0.1": "5.0.1"
       }
     },
     "System.Drawing.Design": {