Skip to content

Commit fe8a2d0

Browse files
safernAnirudh Agnihotry
safern
authored and
Anirudh Agnihotry
committed
Merged PR 12207: Port graphics metafile refcount fix to avoid disposing 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.
2 parents 44db96a + 9b86174 commit fe8a2d0

File tree

7 files changed

+132
-2
lines changed

7 files changed

+132
-2
lines changed

‎NuGet.config‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
<add key="dotnet-eng" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json" />
1717
<add key="dotnet5" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5/nuget/v3/index.json" />
1818
<add key="dotnet5-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5-transport/nuget/v3/index.json" />
19+
<!-- Harvesting feed from 2.1 -->
20+
<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" />
1921
</packageSources>
2022
<disabledPackageSources>
2123
<clear />

‎eng/restore/harvestPackages.targets‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

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

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,8 @@
44
<StrongNameKeyId>Open</StrongNameKeyId>
55
<IncludePlatformAttributes>true</IncludePlatformAttributes>
66
<UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>
7+
<PackageVersion>5.0.1</PackageVersion>
8+
<AssemblyVersion>5.0.0.1</AssemblyVersion>
9+
<HarvestVersion>4.5.2</HarvestVersion>
710
</PropertyGroup>
811
</Project>

‎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
@@ -49,9 +49,18 @@ public sealed partial class Graphics : MarshalByRefObject, IDisposable, IDeviceC
4949
private bool disposed;
5050
private static float defDpiX;
5151
private static float defDpiY;
52+
private Metafile.MetafileHolder? _metafileHolder;
5253

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

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

@@ -487,7 +504,7 @@ public static Graphics FromImage(Image image)
487504

488505
int status = Gdip.GdipGetImageGraphicsContext(image.nativeImage, out graphics);
489506
Gdip.CheckStatus(status);
490-
Graphics result = new Graphics(graphics);
507+
Graphics result = new Graphics(graphics, image);
491508

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

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

147250
public IntPtr GetHenhmetafile()

‎src/libraries/libraries-packages.proj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
<ProjectReference Include="$(LibrariesProjectRoot)\System.Speech\pkg\System.Speech.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
2323
<ProjectReference Include="$(LibrariesProjectRoot)\System.Diagnostics.PerformanceCounter\pkg\System.Diagnostics.PerformanceCounter.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
2424
<ProjectReference Include="$(LibrariesProjectRoot)\System.Diagnostics.EventLog\pkg\System.Diagnostics.EventLog.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
25+
<ProjectReference Include="$(LibrariesProjectRoot)\System.Drawing.Common\pkg\System.Drawing.Common.pkgproj" Condition="'$(BuildAllConfigurations)' == 'true'" />
2526
<ProjectReference Include="$(PkgDir)\Microsoft.NETCore.Platforms\Microsoft.NETCore.Platforms.proj" />
2627
</ItemGroup>
2728

‎src/libraries/pkg/baseline/packageIndex.json‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2862,6 +2862,7 @@
28622862
"StableVersions": [
28632863
"4.5.0",
28642864
"4.5.1",
2865+
"4.5.2",
28652866
"4.6.0",
28662867
"4.6.1",
28672868
"4.6.2",
@@ -2881,9 +2882,11 @@
28812882
"AssemblyVersionInPackageVersion": {
28822883
"4.0.0.0": "4.5.0",
28832884
"4.0.0.1": "4.5.1",
2885+
"4.0.0.2": "4.5.2",
28842886
"4.0.1.0": "4.6.0",
28852887
"4.0.1.1": "4.6.1",
2886-
"5.0.0.0": "5.0.0"
2888+
"5.0.0.0": "5.0.0",
2889+
"5.0.0.1": "5.0.1"
28872890
}
28882891
},
28892892
"System.Drawing.Design": {

0 commit comments

Comments
 (0)