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 @@ -10,6 +10,8 @@ Imports System.Windows.Forms
Imports Microsoft.VisualBasic.CompilerServices
Imports Microsoft.VisualBasic.CompilerServices.ExceptionUtils
Imports Microsoft.VisualBasic.CompilerServices.Utils
Imports Windows.Win32
Imports Windows.Win32.Foundation

Namespace Microsoft.VisualBasic

Expand Down Expand Up @@ -136,25 +138,21 @@ Namespace Microsoft.VisualBasic
Public Sub AppActivateByTitle(Title As String)
'As an optimization, we will only check the UI permission once we actually know we found the app to activate - we'll do that in AppActivateHelper
Dim WindowHandle As IntPtr = NativeMethods.FindWindow(Nothing, Title) 'see if we can find the window using an exact match on the title
Const MAX_TITLE_LENGTH As Integer = 511

' if no match, search through all parent windows
If IntPtr.op_Equality(WindowHandle, IntPtr.Zero) Then
Dim AppTitle As String
' Old implementation uses MAX_TITLE_LENGTH characters, INCLUDING NULL character.
' Interop code will extend string builder to handle NULL character.
Dim AppTitleBuilder As New StringBuilder(MAX_TITLE_LENGTH)
Dim AppTitleLength As Integer
Dim TitleLength As Integer = Len(Title)
Dim TitleLength As Integer = Title.Length

'Loop through all children of the desktop
WindowHandle = NativeMethods.GetWindow(NativeMethods.GetDesktopWindow(), NativeTypes.GW_CHILD)
Do While IntPtr.op_Inequality(WindowHandle, IntPtr.Zero)
' get the window caption and test for a left-aligned substring
AppTitleLength = NativeMethods.GetWindowText(WindowHandle, AppTitleBuilder, AppTitleBuilder.Capacity)
AppTitle = AppTitleBuilder.ToString()
AppTitle = PInvoke.GetWindowText(CType(WindowHandle, HWND))

If AppTitleLength >= TitleLength Then
If AppTitle.Length >= TitleLength Then
If String.Compare(AppTitle, 0, Title, 0, TitleLength, StringComparison.OrdinalIgnoreCase) = 0 Then
Exit Do 'found one
End If
Expand All @@ -170,10 +168,9 @@ Namespace Microsoft.VisualBasic

Do While IntPtr.op_Inequality(WindowHandle, IntPtr.Zero)
' get the window caption and test for a right-aligned substring
AppTitleLength = NativeMethods.GetWindowText(WindowHandle, AppTitleBuilder, AppTitleBuilder.Capacity)
AppTitle = AppTitleBuilder.ToString()
AppTitle = PInvoke.GetWindowText(CType(WindowHandle, HWND))

If AppTitleLength >= TitleLength Then
If AppTitle.Length >= TitleLength Then
If String.Compare(Right(AppTitle, TitleLength), 0, Title, 0, TitleLength, StringComparison.OrdinalIgnoreCase) = 0 Then
Exit Do 'found a match
End If
Expand Down
3 changes: 3 additions & 0 deletions src/System.Windows.Forms.Primitives/src/NativeMethods.txt
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 Expand Up @@ -240,6 +241,8 @@ GetWindow
GetWindowDpiAwarenessContext
GetWindowPlacement
GetWindowRect
GetWindowText
GetWindowTextLength
GetWindowThreadProcessId
GetWorldTransform
GlobalAlloc
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

[assembly: System.Runtime.InteropServices.ComVisible(false)]

[assembly: InternalsVisibleTo("Microsoft.VisualBasic.Forms, PublicKey=00000000000000000400000000000000")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should have worked but we are still getting build errors. I don't know why.

Copy link
Member

Choose a reason for hiding this comment

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

The build is failing for other reasons:

##[error]src\System.Windows.Forms.Primitives\src\Windows\Win32\PInvoke.GetModuleFileNameLongPath.cs(6,1): error IDE0005: (NETCORE_ENGINEERING_TELEMETRY=Build) Using directive is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it has the failures:

error BC30389: (NETCORE_ENGINEERING_TELEMETRY=Build) 'Windows.Win32.PInvoke' is not accessible in this context because it is 'Friend'.

Copy link
Member

Choose a reason for hiding this comment

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

Microsoft.VisualBasic.Forms doesn't reference System.Windows.Forms.Primitives project directly.

Copy link
Member

Choose a reason for hiding this comment

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

...and I personally don't think it should.

Copy link
Contributor

Choose a reason for hiding this comment

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

IVT is not transitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RussKie Did you want me to remove the Microsoft.VisualBasic.Forms changes and we can work out what to do for it in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

I defer the decision to @lonitra and @JeremyKuhne.

Copy link
Member

Choose a reason for hiding this comment

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

@Elachan it's probably better to do the VB stuff separately for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

[assembly: InternalsVisibleTo("System.Windows.Forms, PublicKey=00000000000000000400000000000000")]
[assembly: InternalsVisibleTo("System.Windows.Forms.Design, PublicKey=00000000000000000400000000000000")]
[assembly: InternalsVisibleTo("System.Windows.Forms.Design.Editors, PublicKey=00000000000000000400000000000000")]
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,67 @@
// 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.Runtime.InteropServices;

namespace Windows.Win32
{
internal static partial class PInvoke
{
public static unsafe string GetModuleFileNameLongPath(HINSTANCE hModule)
{
Span<char> buffer = stackalloc char[MAX_PATH];
elachlan marked this conversation as resolved.
Show resolved Hide resolved
uint pathLength;
bool isBufferTooSmall = false;
fixed (char* lpFilename = buffer)
{
pathLength = GetModuleFileName(hModule, lpFilename, (uint)buffer.Length);
isBufferTooSmall = (WIN32_ERROR)Marshal.GetLastWin32Error() == WIN32_ERROR.ERROR_INSUFFICIENT_BUFFER;
elachlan marked this conversation as resolved.
Show resolved Hide resolved
}

if (pathLength == 0)
{
return string.Empty;
}

if (pathLength < buffer.Length - 1 && !isBufferTooSmall)
elachlan marked this conversation as resolved.
Show resolved Hide resolved
{
return new string(buffer[..(int)pathLength]);
}

char[] lbuffer;
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)
{
lbuffer = ArrayPool<char>.Shared.Rent(bufferSize);
fixed (char* lpFilename = lbuffer)
{
pathLength = GetModuleFileName(hModule, lpFilename, (uint)lbuffer.Length);
isBufferTooSmall = (WIN32_ERROR)Marshal.GetLastWin32Error() == WIN32_ERROR.ERROR_INSUFFICIENT_BUFFER;
elachlan marked this conversation as resolved.
Show resolved Hide resolved
}

if (pathLength == 0)
{
ArrayPool<char>.Shared.Return(lbuffer);
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 < lbuffer.Length - 1 && !isBufferTooSmall)
elachlan marked this conversation as resolved.
Show resolved Hide resolved
{
// Get return value and return buffer to array pool.
string returnValue = new string(lbuffer, 0, (int)pathLength);
ArrayPool<char>.Shared.Return(lbuffer);
return returnValue;
}

// buffer was too small, return to array pool.
ArrayPool<char>.Shared.Return(lbuffer);
}

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,62 @@
// 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;

namespace Windows.Win32
{
internal static partial class PInvoke
{
//const int MAX_TITLE_LENGTH = 511;

public static unsafe string GetWindowText(HWND hWnd)
{
int length = GetWindowTextLength(hWnd);

// If the window has no text, the return value is zero.
if (length == 0)
{
return string.Empty;
}

// Stackalloc for smaller values
if (length <= 1024)
{
Span<char> buffer = stackalloc char[length + 1];
fixed (char* lpString = buffer)
{
length = GetWindowText(hWnd, (PWSTR)lpString, buffer.Length);
}

// If the window has no title bar or text, if the title bar is empty,
// or if the window or control handle is invalid, the return value is zero
if (length == 0)
{
return string.Empty;
}
}

// Use arraypool for larger than 1024 characters.
char[] lBuffer;
lBuffer = ArrayPool<char>.Shared.Rent(length + 1);
elachlan marked this conversation as resolved.
Show resolved Hide resolved
int pathLength;
fixed (char* lpString = lBuffer)
{
pathLength = GetWindowText(hWnd, (PWSTR)lpString, lBuffer.Length);
elachlan marked this conversation as resolved.
Show resolved Hide resolved
}

// If the window has no title bar or text, if the title bar is empty,
// or if the window or control handle is invalid, the return value is zero
if (pathLength == 0)
{
ArrayPool<char>.Shared.Return(lBuffer);
return string.Empty;
}

// Get return value and return buffer to array pool.
string returnValue = new string(lBuffer, 0, pathLength);
ArrayPool<char>.Shared.Return(lBuffer);
return returnValue;
}
}
}
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
Loading