From b8db68c52d817b2cb0bff30c0f43eb47901c5d2f Mon Sep 17 00:00:00 2001 From: Santiago Fernandez Madero Date: Mon, 8 Feb 2021 12:44:58 -0800 Subject: [PATCH] Fix GDI handle leak in Icon.DrawImage (#47836) * Fix GDI handle leak in Icon.DrawImage * Fix build * Add test * Fix test in Mono * Add missing Usings --- .../Interop/Windows/Gdi32/Interop.SelectClipRgn.cs | 7 --- .../Common/tests/System/Drawing/Helpers.cs | 3 ++ .../src/System/Drawing/Icon.Windows.cs | 14 ++--- .../tests/GdiPlusHandlesTests.cs | 59 ++++++++++++++++++++++ .../tests/System.Drawing.Common.Tests.csproj | 1 + 5 files changed, 71 insertions(+), 13 deletions(-) create mode 100644 src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs diff --git a/src/libraries/Common/src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs b/src/libraries/Common/src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs index 38d84e9..adf12cd 100644 --- a/src/libraries/Common/src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs +++ b/src/libraries/Common/src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs @@ -11,13 +11,6 @@ internal static partial class Interop [DllImport(Libraries.Gdi32, SetLastError = true, ExactSpelling = true)] public static extern RegionType SelectClipRgn(IntPtr hdc, IntPtr hrgn); - public static RegionType SelectClipRgn(HandleRef hdc, IntPtr hrgn) - { - RegionType result = SelectClipRgn(hdc.Handle, hrgn); - GC.KeepAlive(hdc.Wrapper); - return result; - } - public static RegionType SelectClipRgn(HandleRef hdc, HandleRef hrgn) { RegionType result = SelectClipRgn(hdc.Handle, hrgn.Handle); diff --git a/src/libraries/Common/tests/System/Drawing/Helpers.cs b/src/libraries/Common/tests/System/Drawing/Helpers.cs index bf15413..162cf6a 100644 --- a/src/libraries/Common/tests/System/Drawing/Helpers.cs +++ b/src/libraries/Common/tests/System/Drawing/Helpers.cs @@ -182,6 +182,9 @@ namespace System.Drawing [DllImport("user32.dll", SetLastError = true)] public static extern IntPtr GetForegroundWindow(); + [DllImport("user32.dll", ExactSpelling = true, SetLastError = true)] + public static extern int GetGuiResources(IntPtr hProcess, uint flags); + [DllImport("user32.dll", SetLastError = true)] public static extern IntPtr GetDC(IntPtr hWnd); diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs index 68689ff..c6d2b84 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs @@ -379,7 +379,9 @@ namespace System.Drawing } finally { - RestoreClipRgn(dc, hSaveRgn); + Interop.Gdi32.SelectClipRgn(dc, hSaveRgn); + // We need to delete the region handle after restoring the region as GDI+ uses a copy of the handle. + Interop.Gdi32.DeleteObject(hSaveRgn); } } @@ -394,15 +396,15 @@ namespace System.Drawing hSaveRgn = hTempRgn; hTempRgn = IntPtr.Zero; } + else + { + // if we fail to get the clip region delete the handle. + Interop.Gdi32.DeleteObject(hTempRgn); + } return hSaveRgn; } - private static void RestoreClipRgn(IntPtr hDC, IntPtr hRgn) - { - Interop.Gdi32.SelectClipRgn(new HandleRef(null, hDC), new HandleRef(null, hRgn)); - } - internal void Draw(Graphics graphics, int x, int y) { Size size = Size; diff --git a/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs b/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs new file mode 100644 index 0000000..7fdc52d --- /dev/null +++ b/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs @@ -0,0 +1,59 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Runtime.InteropServices; +using Microsoft.DotNet.RemoteExecutor; +using Xunit; +using Xunit.Sdk; + +namespace System.Drawing.Tests +{ + [PlatformSpecific(TestPlatforms.Windows)] + public static class GdiPlusHandlesTests + { + public static bool IsDrawingAndRemoteExecutorSupported => Helpers.GetIsDrawingSupported() && RemoteExecutor.IsSupported; + + [ConditionalFact(nameof(IsDrawingAndRemoteExecutorSupported))] + public static void GraphicsDrawIconDoesNotLeakHandles() + { + RemoteExecutor.Invoke(() => + { + const int handleTreshold = 1; + using Bitmap bmp = new(100, 100); + using Icon ico = new(Helpers.GetTestBitmapPath("16x16_one_entry_4bit.ico")); + + IntPtr hdc = Helpers.GetDC(Helpers.GetForegroundWindow()); + using Graphics graphicsFromHdc = Graphics.FromHdc(hdc); + + using Process currentProcess = Process.GetCurrentProcess(); + IntPtr processHandle = currentProcess.Handle; + + int initialHandles = Helpers.GetGuiResources(processHandle, 0); + ValidateNoWin32Error(initialHandles); + + for (int i = 0; i < 5000; i++) + { + graphicsFromHdc.DrawIcon(ico, 100, 100); + } + + int finalHandles = Helpers.GetGuiResources(processHandle, 0); + ValidateNoWin32Error(finalHandles); + + Assert.InRange(finalHandles, initialHandles, initialHandles + handleTreshold); + }).Dispose(); + } + + private static void ValidateNoWin32Error(int handleCount) + { + if (handleCount == 0) + { + int error = Marshal.GetLastWin32Error(); + + if (error != 0) + throw new XunitException($"GetGuiResorces failed with win32 error: {error}"); + } + } + + } +} diff --git a/src/libraries/System.Drawing.Common/tests/System.Drawing.Common.Tests.csproj b/src/libraries/System.Drawing.Common/tests/System.Drawing.Common.Tests.csproj index e84c4ed..7e66b06 100644 --- a/src/libraries/System.Drawing.Common/tests/System.Drawing.Common.Tests.csproj +++ b/src/libraries/System.Drawing.Common/tests/System.Drawing.Common.Tests.csproj @@ -22,6 +22,7 @@ + -- 2.7.4