CA1838 Avoid 'StringBuilder' parameters for P/Invokes (#8113)

* Initial work to remove string builders from PInvoke * Fix build errors * changes from review * more changes from review * fix usings * Changes from review * changes from review * Convert GetWindowText usage * changes from review to avoid char[] allocation * changes from review * more changes from review * more changes from review * remove and sort usings * remove vb changes * changes from review
dotnet · Nov 11, 2022 · 93dd384 · 93dd384
1 parent b2f6e00
commit 93dd384
Show file tree

Hide file tree

Showing 10 changed files with 110 additions and 73 deletions.
diff --git a/src/System.Windows.Forms.Primitives/src/NativeMethods.txt b/src/System.Windows.Forms.Primitives/src/NativeMethods.txt
@@ -183,6 +183,7 @@ GetMenu
 GetMenuItemCount
 GetMessagePos
 GetMessageTime
+GetModuleFileName
 GetModuleHandle
 GetNearestColor
 GetObjectType

diff --git a/...System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/UnsafeNativeMethods.cs b/...System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/UnsafeNativeMethods.cs
@@ -3,46 +3,15 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Runtime.InteropServices;
-using System.Text;
 using static Interop;
 
 namespace System.Windows.Forms
 {
     internal static class UnsafeNativeMethods
     {
-        [DllImport(Libraries.User32)]
-#pragma warning disable CA1838 // Avoid 'StringBuilder' parameters for P/Invokes
-        public static extern int GetClassName(HandleRef hwnd, StringBuilder? lpClassName, int nMaxCount);
-#pragma warning restore CA1838 // Avoid 'StringBuilder' parameters for P/Invokes
-
         [DllImport(Libraries.Comdlg32, SetLastError = true, CharSet = CharSet.Auto)]
         public static extern HRESULT PrintDlgEx([In, Out] NativeMethods.PRINTDLGEX lppdex);
 
-        [DllImport(Libraries.Kernel32, CharSet = CharSet.Auto, SetLastError = true)]
-#pragma warning disable CA1838 // Avoid 'StringBuilder' parameters for P/Invokes
-        public static extern int GetModuleFileName(HandleRef hModule, StringBuilder buffer, int length);
-#pragma warning restore CA1838 // Avoid 'StringBuilder' parameters for P/Invokes
-
-        public static StringBuilder GetModuleFileNameLongPath(HandleRef hModule)
-        {
-            StringBuilder buffer = new StringBuilder(PInvoke.MAX_PATH);
-            int noOfTimes = 1;
-            int length = 0;
-            // Iterating by allocating chunk of memory each time we find the length is not sufficient.
-            // Performance should not be an issue for current MAX_PATH length due to this change.
-            while (((length = GetModuleFileName(hModule, buffer, buffer.Capacity)) == buffer.Capacity)
-                && Marshal.GetLastWin32Error() == ERROR.INSUFFICIENT_BUFFER
-                && buffer.Capacity < PInvoke.MAX_UNICODESTRING_LEN)
-            {
-                noOfTimes += 2; // Increasing buffer size by 520 in each iteration.
-                int capacity = noOfTimes * PInvoke.MAX_PATH < PInvoke.MAX_UNICODESTRING_LEN ? noOfTimes * PInvoke.MAX_PATH : PInvoke.MAX_UNICODESTRING_LEN;
-                buffer.EnsureCapacity(capacity);
-            }
-
-            buffer.Length = length;
-            return buffer;
-        }
-
         [DllImport(Libraries.Oleacc, ExactSpelling = true, CharSet = CharSet.Auto)]
         public static extern int CreateStdAccessibleObject(HandleRef hWnd, int objID, ref Guid refiid, [In, Out, MarshalAs(UnmanagedType.Interface)] ref object? pAcc);
     }

diff --git a/src/System.Windows.Forms.Primitives/src/Windows/Win32/PInvoke.GetModuleFileNameLongPath.cs b/src/System.Windows.Forms.Primitives/src/Windows/Win32/PInvoke.GetModuleFileNameLongPath.cs
@@ -0,0 +1,65 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Buffers;
+using System.Diagnostics;
+
+namespace Windows.Win32
+{
+    internal static partial class PInvoke
+    {
+        public static unsafe string GetModuleFileNameLongPath(HINSTANCE hModule)
+        {
+            Span<char> stackBuffer = stackalloc char[MAX_PATH];
+            uint pathLength;
+            fixed (char* lpFilename = stackBuffer)
+            {
+                pathLength = GetModuleFileName(hModule, lpFilename, (uint)stackBuffer.Length);
+            }
+
+            if (pathLength == 0)
+            {
+                return string.Empty;
+            }
+
+            if (pathLength < stackBuffer.Length)
+            {
+                return new string(stackBuffer[..(int)pathLength]);
+            }
+
+            char[] buffer;
+            int bufferSize = 4096;
+            // Allocate increasingly larger portions of memory until successful or we hit short.maxvalue.
+            for (int i = 1; bufferSize <= short.MaxValue; i++, bufferSize = 4096 * i)
+            {
+                buffer = ArrayPool<char>.Shared.Rent(bufferSize);
+                fixed (char* lpFilename = buffer)
+                {
+                    pathLength = GetModuleFileName(hModule, lpFilename, (uint)buffer.Length);
+                }
+
+                if (pathLength == 0)
+                {
+                    ArrayPool<char>.Shared.Return(buffer);
+                    return string.Empty;
+                }
+
+                // If the length equals the buffer size we need to check to see if we were told the buffer was insufficient (it was trimmed)
+                if (pathLength < buffer.Length)
+                {
+                    // Get return value and return buffer to array pool.
+                    string returnValue = new string(buffer, 0, (int)pathLength);
+                    ArrayPool<char>.Shared.Return(buffer);
+                    return returnValue;
+                }
+
+                // buffer was too small, return to array pool.
+                ArrayPool<char>.Shared.Return(buffer);
+            }
+
+            Debug.Fail($"Module File Name is greater than {short.MaxValue}.");
+            return string.Empty;
+        }
+    }
+}
diff --git a/src/System.Windows.Forms.Primitives/src/Windows/Win32/PInvoke.MaxClassName.cs b/src/System.Windows.Forms.Primitives/src/Windows/Win32/PInvoke.MaxClassName.cs
@@ -0,0 +1,15 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+namespace Windows.Win32
+{
+    internal static partial class PInvoke
+    {
+        /// <summary>
+        /// <para>The maximum length for lpszClassName is 256. If lpszClassName is greater than the maximum length, the RegisterClassEx function will fail.</para>
+        /// <para><see href="https://learn.microsoft.com/windows/win32/api/winuser/ns-winuser-wndclassexw#members">Read more on docs.microsoft.com</see>.</para>
+        /// </summary>
+        public const int MaxClassName = 256;
+    }
+}
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Application.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Application.cs
@@ -288,19 +288,8 @@ internal static bool CustomThreadExceptionHandlerAttached
         /// <summary>
         ///  Gets the path for the executable file that started the application.
         /// </summary>
-        public static string ExecutablePath
-        {
-            get
-            {
-                if (s_executablePath is null)
-                {
-                    StringBuilder sb = UnsafeNativeMethods.GetModuleFileNameLongPath(NativeMethods.NullHandleRef);
-                    s_executablePath = Path.GetFullPath(sb.ToString());
-                }
-
-                return s_executablePath;
-            }
-        }
+        public static string ExecutablePath =>
+            s_executablePath ??= PInvoke.GetModuleFileNameLongPath(HINSTANCE.Null);
 
         /// <summary>
         ///  Gets the current <see cref="HighDpiMode"/> mode for the process.

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/ComboBox.AutoCompleteDropDownFinder.cs b/src/System.Windows.Forms/src/System/Windows/Forms/ComboBox.AutoCompleteDropDownFinder.cs
@@ -41,13 +41,12 @@ internal void FindDropDowns(bool subclass)
             private unsafe BOOL Callback(HWND hwnd)
             {
                 Span<char> buffer = stackalloc char[AutoCompleteClassName.Length + 2];
-
                 fixed (char* b = buffer)
                 {
-                    int count = PInvoke.GetClassName(hwnd, (PWSTR)b, buffer.Length);
+                    int length = PInvoke.GetClassName(hwnd, (PWSTR)b, buffer.Length);
 
                     // Check class name and see if it's visible
-                    if (count == AutoCompleteClassName.Length && buffer.StartsWith(AutoCompleteClassName))
+                    if (length == AutoCompleteClassName.Length && buffer.StartsWith(AutoCompleteClassName))
                     {
                         ACNativeWindow.RegisterACWindow(hwnd, _shouldSubClass);
                     }

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs
@@ -12,7 +12,6 @@
 using System.Runtime.ExceptionServices;
 using System.Runtime.InteropServices;
 using System.Runtime.InteropServices.ComTypes;
-using System.Text;
 using System.Windows.Forms.Automation;
 using System.Windows.Forms.Layout;
 using System.Windows.Forms.Primitives;
@@ -2545,7 +2544,7 @@ public int Height
             set => SetBounds(_x, _y, _width, value, BoundsSpecified.Height);
         }
 
-        internal bool HostedInWin32DialogManager
+        internal unsafe bool HostedInWin32DialogManager
         {
             get
             {
@@ -2560,22 +2559,21 @@ internal bool HostedInWin32DialogManager
                     {
                         HWND parentHandle = PInvoke.GetParent(this);
                         HWND lastParentHandle = parentHandle;
-
-                        StringBuilder sb = new StringBuilder(32);
-
                         SetState(States.HostedInDialog, false);
+                        Span<char> buffer = stackalloc char[PInvoke.MaxClassName];
 
                         while (!parentHandle.IsNull)
                         {
-                            int len = UnsafeNativeMethods.GetClassName(new HandleRef(null, lastParentHandle), null, 0);
-                            if (len > sb.Capacity)
+                            int length = 0;
+                            fixed (char* lpClassName = buffer)
                             {
-                                sb.Capacity = len + 5;
+                                length = PInvoke.GetClassName(lastParentHandle, lpClassName, buffer.Length);
                             }
 
-                            UnsafeNativeMethods.GetClassName(new HandleRef(null, lastParentHandle), sb, sb.Capacity);
-
-                            if (sb.ToString() == "#32770")
+                            // #32770 is the standard windows dialog class name
+                            // https://learn.microsoft.com/windows/win32/winmsg/about-window-classes#system-classes
+                            ReadOnlySpan<char> className = "#32770";
+                            if (className.Equals(buffer[..length], StringComparison.Ordinal))
                             {
                                 SetState(States.HostedInDialog, true);
                                 break;

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/RichTextBox.cs b/src/System.Windows.Forms/src/System/Windows/Forms/RichTextBox.cs
@@ -297,8 +297,7 @@ protected override CreateParams CreateParams
                         throw new Win32Exception(lastWin32Error, string.Format(SR.LoadDLLError, richEditControlDllVersion));
                     }
 
-                    StringBuilder pathBuilder = UnsafeNativeMethods.GetModuleFileNameLongPath(new HandleRef(null, moduleHandle));
-                    string path = pathBuilder.ToString();
+                    string path = PInvoke.GetModuleFileNameLongPath(new HINSTANCE(moduleHandle));
                     FileVersionInfo versionInfo = FileVersionInfo.GetVersionInfo(path);
 
                     Debug.Assert(versionInfo is not null && !string.IsNullOrEmpty(versionInfo.ProductVersion), "Couldn't get the version info for the richedit dll");

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/TaskDialog.cs b/src/System.Windows.Forms/src/System/Windows/Forms/TaskDialog.cs
@@ -777,9 +777,7 @@ internal void UpdateCaption(string? caption)
             // executable's name as title if the string is null or empty.
             if (TaskDialogPage.IsNativeStringNullOrEmpty(caption))
             {
-                caption = Path.GetFileName(
-                    UnsafeNativeMethods.GetModuleFileNameLongPath(NativeMethods.NullHandleRef)
-                                       .ToString());
+                caption = Path.GetFileName(PInvoke.GetModuleFileNameLongPath(HINSTANCE.Null));
             }
 
             User32.SetWindowTextW(_handle, caption);

diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs
@@ -10584,7 +10584,7 @@ public void RichTextBox_CheckDefaultNativeControlVersions()
             using var control = new RichTextBox();
             control.CreateControl();
 
-            Assert.Contains("RICHEDIT50W", GetClassName(control.Handle), StringComparison.InvariantCultureIgnoreCase);
+            Assert.Contains("RICHEDIT50W", GetClassName(control.HWND), StringComparison.Ordinal);
         }
 
         [WinFormsFact]
@@ -10593,13 +10593,13 @@ public void RichTextBox_CheckRichEditWithVersionCanCreateOldVersions()
             using (var riched32 = new RichEdit())
             {
                 riched32.CreateControl();
-                Assert.Contains(".RichEdit.", GetClassName(riched32.Handle), StringComparison.InvariantCultureIgnoreCase);
+                Assert.Contains(".RichEdit.", GetClassName(riched32.HWND), StringComparison.Ordinal);
             }
 
             using (var riched20 = new RichEdit20W())
             {
                 riched20.CreateControl();
-                Assert.Contains(".RichEdit20W.", GetClassName(riched20.Handle), StringComparison.InvariantCultureIgnoreCase);
+                Assert.Contains(".RichEdit20W.", GetClassName(riched20.HWND), StringComparison.Ordinal);
 
                 string rtfString = @"{\rtf1\ansi{" +
                     @"The next line\par " +
@@ -10615,8 +10615,8 @@ public void RichTextBox_CheckRichEditWithVersionCanCreateOldVersions()
                 Assert.Equal(riched20.Text, richTextBox.Text);
                 Assert.Equal(richTextBox.Text.Length, richTextBox.TextLength);
 
-                int startOfIs = riched20.Text.IndexOf("is");
-                int endOfHidden = riched20.Text.IndexOf("hidden") + "hidden".Length;
+                int startOfIs = riched20.Text.IndexOf("is", StringComparison.Ordinal);
+                int endOfHidden = riched20.Text.IndexOf("hidden", StringComparison.Ordinal) + "hidden".Length;
                 richTextBox.Select(startOfIs, endOfHidden - startOfIs);
                 Assert.Equal("is ###NOT### hidden", richTextBox.SelectedText);
             }
@@ -10791,12 +10791,16 @@ private class SubRichTextBox : RichTextBox
             public new void WndProc(ref Message m) => base.WndProc(ref m);
         }
 
-        private static string GetClassName(IntPtr hWnd)
+        private static unsafe string GetClassName(HWND hWnd)
         {
-            const int MaxClassName = 256;
-            StringBuilder sb = new StringBuilder(MaxClassName);
-            UnsafeNativeMethods.GetClassName(new HandleRef(null, hWnd), sb, MaxClassName);
-            return sb.ToString();
+            int length = 0;
+            Span<char> buffer = stackalloc char[PInvoke.MaxClassName];
+            fixed (char* lpClassName = buffer)
+            {
+                length = PInvoke.GetClassName(hWnd, lpClassName, buffer.Length);
+            }
+
+            return new string(buffer[..length]);
         }
 
         /// <summary>