Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Nov 11, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ GetMenu
GetMenuItemCount
GetMessagePos
GetMessageTime
GetModuleFileName
GetModuleHandle
GetNearestColor
GetObjectType
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
elachlan marked this conversation as resolved.
Show resolved Hide resolved
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;
elachlan marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
20 changes: 9 additions & 11 deletions src/System.Windows.Forms/src/System/Windows/Forms/Control.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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 Microsoft.Win32;
Expand Down Expand Up @@ -2541,7 +2540,7 @@ public int Height
set => SetBounds(_x, _y, _width, value, BoundsSpecified.Height);
}

internal bool HostedInWin32DialogManager
internal unsafe bool HostedInWin32DialogManager
{
get
{
Expand All @@ -2556,22 +2555,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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
elachlan marked this conversation as resolved.
Show resolved Hide resolved
}

User32.SetWindowTextW(_handle, caption);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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 " +
Expand All @@ -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);
}
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this move to the PInvoke namespace?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is another place where we call PInvoke.GetClassName in our codebase. If this is the case, we should move this to PInvoke namespace and call to this method in the other places we are using PInvoke.GetClassName

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't move this because its a wrapper and isn't used in the other calls. I am happy to still move it. The other uses avoid string allocations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think this is a dup, feel free to remove this. I can't recall exact reasons why I copied it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since its a wrapper of the pinvoke and we don't reuse it, I think it can stay here.

{
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>
Expand Down