From 4ec64c8bed0a4dea07e759b61e194b1766d75565 Mon Sep 17 00:00:00 2001 From: Santiago Fernandez Madero Date: Wed, 3 Feb 2021 19:09:14 -0800 Subject: [PATCH 1/5] Fix GDI handle leak in Icon.DrawImage --- .../Windows/Gdi32/Interop.SelectClipRgn.cs | 15 --------------- .../src/System/Drawing/Icon.Windows.cs | 14 ++++++++------ 2 files changed, 8 insertions(+), 21 deletions(-) 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 38d84e967185af..527091b2e783d4 100644 --- a/src/libraries/Common/src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs +++ b/src/libraries/Common/src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs @@ -10,20 +10,5 @@ internal static partial class Gdi32 { [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); - GC.KeepAlive(hdc.Wrapper); - GC.KeepAlive(hrgn.Wrapper); - return result; - } } } 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 68689fff1bebb9..02c1fdb6f9d4a4 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 @@ private void DrawIcon(IntPtr dc, Rectangle imageRect, Rectangle targetRect, bool } finally { - RestoreClipRgn(dc, hSaveRgn); + Interop.Gdi32.SelectClipRgn(hDC, hRgn); + // 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 @@ private static IntPtr SaveClipRgn(IntPtr hDC) 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; From 49b815b112ef996a84c44f19179dc35bbd03e1ce Mon Sep 17 00:00:00 2001 From: Santiago Fernandez Madero Date: Thu, 4 Feb 2021 09:53:21 -0800 Subject: [PATCH 2/5] Fix build --- .../src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs | 8 ++++++++ .../src/System/Drawing/Icon.Windows.cs | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) 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 527091b2e783d4..adf12cde9407aa 100644 --- a/src/libraries/Common/src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs +++ b/src/libraries/Common/src/Interop/Windows/Gdi32/Interop.SelectClipRgn.cs @@ -10,5 +10,13 @@ internal static partial class Gdi32 { [DllImport(Libraries.Gdi32, SetLastError = true, ExactSpelling = true)] public static extern RegionType SelectClipRgn(IntPtr hdc, IntPtr hrgn); + + public static RegionType SelectClipRgn(HandleRef hdc, HandleRef hrgn) + { + RegionType result = SelectClipRgn(hdc.Handle, hrgn.Handle); + GC.KeepAlive(hdc.Wrapper); + GC.KeepAlive(hrgn.Wrapper); + return result; + } } } 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 02c1fdb6f9d4a4..c6d2b845ae9b54 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,7 @@ private void DrawIcon(IntPtr dc, Rectangle imageRect, Rectangle targetRect, bool } finally { - Interop.Gdi32.SelectClipRgn(hDC, hRgn); + 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); } From d568908b4e5b8b99d3535dabd335a64af8d1608f Mon Sep 17 00:00:00 2001 From: Santiago Fernandez Madero Date: Thu, 4 Feb 2021 20:17:20 -0800 Subject: [PATCH 3/5] Add test --- .../Common/tests/System/Drawing/Helpers.cs | 3 ++ .../tests/GdiPlusHandlesTests.cs | 43 +++++++++++++++++++ .../tests/System.Drawing.Common.Tests.csproj | 1 + 3 files changed, 47 insertions(+) create mode 100644 src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs diff --git a/src/libraries/Common/tests/System/Drawing/Helpers.cs b/src/libraries/Common/tests/System/Drawing/Helpers.cs index bf154135a8c0cb..162cf6abcb0727 100644 --- a/src/libraries/Common/tests/System/Drawing/Helpers.cs +++ b/src/libraries/Common/tests/System/Drawing/Helpers.cs @@ -182,6 +182,9 @@ private static Rectangle GetRectangle(RECT rect) [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/tests/GdiPlusHandlesTests.cs b/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs new file mode 100644 index 00000000000000..68492fec89590e --- /dev/null +++ b/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs @@ -0,0 +1,43 @@ +// 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; + +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; + Bitmap bmp = new(100, 100); + Icon ico = new(Helpers.GetTestBitmapPath("16x16_one_entry_4bit.ico")); + IntPtr currentProcessHandle = Process.GetCurrentProcess().Handle; + IntPtr hdc = Helpers.GetDC(Helpers.GetForegroundWindow()); + using Graphics graphicsFromHdc = Graphics.FromHdc(hdc); + + int initialHandles = Helpers.GetGuiResources(currentProcessHandle, 0); + + for (int i = 0; i < 5000; i++) + { + graphicsFromHdc.DrawIcon(ico, 100, 100); + } + + int finalHandles = Helpers.GetGuiResources(currentProcessHandle, 0); + + Assert.InRange(finalHandles, initialHandles, initialHandles + handleTreshold); + }).Dispose(); + + } + + } +} 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 e84c4ed3c71b05..7e66b067e8d28e 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 @@ + From b726bf5479efa5c528719d123fc96f0b567a158d Mon Sep 17 00:00:00 2001 From: Santiago Fernandez Madero Date: Fri, 5 Feb 2021 17:49:11 -0800 Subject: [PATCH 4/5] Fix test in Mono --- .../tests/GdiPlusHandlesTests.cs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs b/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs index 68492fec89590e..93f67d3742537d 100644 --- a/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs +++ b/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs @@ -5,6 +5,7 @@ using System.Runtime.InteropServices; using Microsoft.DotNet.RemoteExecutor; using Xunit; +using Xunit.Sdk; namespace System.Drawing.Tests { @@ -21,22 +22,33 @@ public static void GraphicsDrawIconDoesNotLeakHandles() const int handleTreshold = 1; Bitmap bmp = new(100, 100); Icon ico = new(Helpers.GetTestBitmapPath("16x16_one_entry_4bit.ico")); - IntPtr currentProcessHandle = Process.GetCurrentProcess().Handle; IntPtr hdc = Helpers.GetDC(Helpers.GetForegroundWindow()); using Graphics graphicsFromHdc = Graphics.FromHdc(hdc); - int initialHandles = Helpers.GetGuiResources(currentProcessHandle, 0); + int initialHandles = Helpers.GetGuiResources(Process.GetCurrentProcess().Handle, 0); + ValidateNoWin32Error(initialHandles); for (int i = 0; i < 5000; i++) { graphicsFromHdc.DrawIcon(ico, 100, 100); } - int finalHandles = Helpers.GetGuiResources(currentProcessHandle, 0); + int finalHandles = Helpers.GetGuiResources(Process.GetCurrentProcess().Handle, 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}"); + } } } From f297ea49ccf0b84eb87367e8b03ef95ef78d5fcf Mon Sep 17 00:00:00 2001 From: Santiago Fernandez Madero Date: Mon, 8 Feb 2021 10:08:08 -0800 Subject: [PATCH 5/5] Add missing Usings --- .../tests/GdiPlusHandlesTests.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs b/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs index 93f67d3742537d..7fdc52d52ccc01 100644 --- a/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs +++ b/src/libraries/System.Drawing.Common/tests/GdiPlusHandlesTests.cs @@ -20,12 +20,16 @@ public static void GraphicsDrawIconDoesNotLeakHandles() RemoteExecutor.Invoke(() => { const int handleTreshold = 1; - Bitmap bmp = new(100, 100); - Icon ico = new(Helpers.GetTestBitmapPath("16x16_one_entry_4bit.ico")); + 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); - int initialHandles = Helpers.GetGuiResources(Process.GetCurrentProcess().Handle, 0); + using Process currentProcess = Process.GetCurrentProcess(); + IntPtr processHandle = currentProcess.Handle; + + int initialHandles = Helpers.GetGuiResources(processHandle, 0); ValidateNoWin32Error(initialHandles); for (int i = 0; i < 5000; i++) @@ -33,7 +37,7 @@ public static void GraphicsDrawIconDoesNotLeakHandles() graphicsFromHdc.DrawIcon(ico, 100, 100); } - int finalHandles = Helpers.GetGuiResources(Process.GetCurrentProcess().Handle, 0); + int finalHandles = Helpers.GetGuiResources(processHandle, 0); ValidateNoWin32Error(finalHandles); Assert.InRange(finalHandles, initialHandles, initialHandles + handleTreshold);