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 #7186

Merged
merged 25 commits into from
Feb 15, 2022
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
994b696
CA1838 Avoid 'StringBuilder' parameters for P/Invokes
elachlan Dec 30, 2021
4b519a9
revert ruleset change
elachlan Jan 7, 2022
3a88ed9
rebase
elachlan Jan 7, 2022
638775f
Enable warning on CA1838
elachlan Jan 7, 2022
751fea3
Merge branch 'main' into CA1838
elachlan Jan 11, 2022
661c2c8
CA1838 Avoid 'StringBuilder' parameters for P/Invokes. Consider using…
elachlan Jan 11, 2022
4f0424c
trying again
elachlan Jan 12, 2022
e042e66
Changes from review
elachlan Jan 27, 2022
4ddf661
GetGacPath changes
elachlan Jan 27, 2022
8f0c31c
Making sure we are form strings properly
elachlan Jan 28, 2022
04e4a1b
changes from review
elachlan Jan 28, 2022
903f35d
Remove unused field
elachlan Jan 28, 2022
69cf05f
Use ArrayPool in GetRuntimeVersion and fix usage in GetModuleFileName
elachlan Jan 28, 2022
300179b
Refactor GetRuntimeVersion to remove loop and use stackalloc
elachlan Jan 28, 2022
c392277
changes from review
elachlan Jan 31, 2022
de3618c
Fix conflict and xml doc
elachlan Jan 31, 2022
a0178d7
Merge branch 'main' into CA1838
elachlan Jan 31, 2022
54878ac
Fix possible StackOverflow in GetShortPathName/GetLongPathName by mov…
elachlan Feb 1, 2022
567cc08
Merge branch 'CA1838' of github.com:elachlan/msbuild into CA1838
elachlan Feb 1, 2022
8af24e5
remove unneeded unsafe from pinvoke definitions
elachlan Feb 1, 2022
031af51
Added cached GAC Path and changes from review
elachlan Feb 2, 2022
b4f6bf7
Add documentation to Pinvoke
elachlan Feb 3, 2022
b5f60db
Changes from review
elachlan Feb 4, 2022
53d3da4
Change from review
elachlan Feb 8, 2022
0a97bfd
Add VerifyThrow instead of loop condition
elachlan Feb 8, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eng/Common.globalconfig
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ dotnet_diagnostic.CA1836.severity = warning
dotnet_diagnostic.CA1837.severity = suggestion

# Avoid 'StringBuilder' parameters for P/Invokes
dotnet_diagnostic.CA1838.severity = suggestion
dotnet_diagnostic.CA1838.severity = warning

# Dispose objects before losing scope
dotnet_diagnostic.CA2000.severity = none
Expand Down
26 changes: 12 additions & 14 deletions src/Framework/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -959,13 +959,13 @@ internal static string GetShortFilePath(string path)

if (length > 0)
{
StringBuilder fullPathBuffer = new StringBuilder(length);
char[] fullPathBuffer = new char[length];
length = GetShortPathName(path, fullPathBuffer, length);
errorCode = Marshal.GetLastWin32Error();

if (length > 0)
{
string fullPath = fullPathBuffer.ToString();
string fullPath = new(fullPathBuffer, 0, length);
path = fullPath;
}
}
Expand Down Expand Up @@ -998,13 +998,13 @@ internal static string GetLongFilePath(string path)

if (length > 0)
{
StringBuilder fullPathBuffer = new StringBuilder(length);
char[] fullPathBuffer = new char[length];
length = GetLongPathName(path, fullPathBuffer, length);
errorCode = Marshal.GetLastWin32Error();

if (length > 0)
{
string fullPath = fullPathBuffer.ToString();
string fullPath = new(fullPathBuffer, 0, length);
path = fullPath;
}
}
Expand Down Expand Up @@ -1478,15 +1478,13 @@ internal static void VerifyThrowWin32Result(int result)
internal static extern bool GetFileAttributesEx(String name, int fileInfoLevel, ref WIN32_FILE_ATTRIBUTE_DATA lpFileInformation);

[DllImport(kernel32Dll, SetLastError = true, CharSet = CharSet.Unicode)]
private static extern uint SearchPath
(
private static extern uint SearchPath(
string path,
string fileName,
string extension,
int numBufferChars,
[Out] StringBuilder buffer,
int[] filePart
);
[Out] char[] buffer,
int[] filePart);

[DllImport("kernel32.dll", PreserveSig = true, SetLastError = true)]
[return: MarshalAs(UnmanagedType.Bool)]
Expand All @@ -1504,10 +1502,10 @@ internal static extern uint GetRequestedRuntimeInfo(String pExe,
String pConfigurationFile,
uint startupFlags,
uint runtimeInfoFlags,
[Out] StringBuilder pDirectory,
[Out] char[] pDirectory,
int dwDirectory,
out uint dwDirectoryLength,
[Out] StringBuilder pVersion,
[Out] char[] pVersion,
int cchBuffer,
out uint dwlength);

Expand All @@ -1521,7 +1519,7 @@ internal static extern int GetModuleFileName(
#else
IntPtr hModule,
#endif
[Out] StringBuilder buffer, int length);
[Out] char[] buffer, int length);

[DllImport("kernel32.dll")]
internal static extern IntPtr GetStdHandle(int nStdHandle);
Expand Down Expand Up @@ -1570,10 +1568,10 @@ internal static bool SetCurrentDirectory(string path)
private static extern bool GlobalMemoryStatusEx([In, Out] MemoryStatus lpBuffer);

[DllImport("kernel32.dll", CharSet = CharSet.Unicode, BestFitMapping = false)]
internal static extern int GetShortPathName(string path, [Out] StringBuilder fullpath, [In] int length);
internal static extern int GetShortPathName(string path, [Out] char[] fullpath, [In] int length);

[DllImport("kernel32.dll", CharSet = CharSet.Unicode, BestFitMapping = false)]
internal static extern int GetLongPathName([In] string path, [Out] StringBuilder fullpath, [In] int length);
internal static extern int GetLongPathName([In] string path, [Out] char[] fullpath, [In] int length);

[DllImport("kernel32.dll", CharSet = AutoOrUnicode, SetLastError = true)]
internal static extern bool CreatePipe(out SafeFileHandle hReadPipe, out SafeFileHandle hWritePipe, SecurityAttributes lpPipeAttributes, int nSize);
Expand Down
19 changes: 7 additions & 12 deletions src/Tasks/AssemblyDependency/AssemblyInformation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -567,8 +567,9 @@ internal static string GetRuntimeVersion(string path)
#if FEATURE_MSCOREE
if (NativeMethodsShared.IsWindows)
{
StringBuilder runtimeVersion;
char[] runtimeVersion;
uint hresult;
int dwLength;
#if DEBUG
// Just to make sure and exercise the code that doubles the size
// every time GetRequestedRuntimeInfo fails due to insufficient buffer size.
Expand All @@ -578,19 +579,13 @@ internal static string GetRuntimeVersion(string path)
#endif
do
{
runtimeVersion = new StringBuilder(bufferLength);
hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out _);
Copy link
Member

Choose a reason for hiding this comment

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

That said, I'm not a fan of this code, so I'm glad it's gone 😁

runtimeVersion = new char[bufferLength];
hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out dwLength);
bufferLength *= 2;
} while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER);

if (hresult == NativeMethodsShared.S_OK)
{
return runtimeVersion.ToString();
}
else
{
return String.Empty;
}
while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER);

return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength-1) : string.Empty;
}
else
{
Expand Down
6 changes: 3 additions & 3 deletions src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -367,16 +367,16 @@ bool specificVersion
}

/// <summary>
/// Return the root path of the GAC
/// Return the root path of the GAC.
/// </summary>
internal static string GetGacPath()
{
int gacPathLength = 0;
NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, null, ref gacPathLength);
StringBuilder gacPath = new StringBuilder(gacPathLength);
char[] gacPath = new char[gacPathLength];
NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, gacPath, ref gacPathLength);

return gacPath.ToString();
return new string(gacPath, 0, gacPathLength-1);
}
}
}
23 changes: 15 additions & 8 deletions src/Tasks/ComReference.cs
Original file line number Diff line number Diff line change
Expand Up @@ -407,21 +407,28 @@ internal static string StripTypeLibNumberFromPath(string typeLibPath, FileExists
private static string GetModuleFileName(IntPtr handle)
{
bool success = false;
var buffer = new StringBuilder();
char[] buffer = null;
int pathLength = 0;

// Try increased buffer sizes if on longpath-enabled Windows
for (int bufferSize = NativeMethodsShared.MAX_PATH; !success && bufferSize <= NativeMethodsShared.MaxPath; bufferSize *= 2)
{
buffer.EnsureCapacity(bufferSize);

var handleRef = new System.Runtime.InteropServices.HandleRef(buffer, handle);
int pathLength = NativeMethodsShared.GetModuleFileName(handleRef, buffer, buffer.Capacity);
buffer = System.Buffers.ArrayPool<char>.Shared.Rent(bufferSize);
try
{
var handleRef = new System.Runtime.InteropServices.HandleRef(buffer, handle);
pathLength = NativeMethodsShared.GetModuleFileName(handleRef, buffer, bufferSize);

bool isBufferTooSmall = ((uint)Marshal.GetLastWin32Error() == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER);
success = pathLength != 0 && !isBufferTooSmall;
bool isBufferTooSmall = (uint)Marshal.GetLastWin32Error() == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER;
success = pathLength != 0 && !isBufferTooSmall;
}
finally
{
System.Buffers.ArrayPool<char>.Shared.Return(buffer);
}
}

return success ? buffer.ToString() : string.Empty;
return success ? new string(buffer, 0, pathLength) : string.Empty;
}

/// <summary>
Expand Down
12 changes: 9 additions & 3 deletions src/Tasks/LockCheck.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@ private static extern int RmRegisterResources(uint pSessionHandle,
string[] rgsServiceNames);

[DllImport(RestartManagerDll, CharSet = CharSet.Unicode)]
private static extern int RmStartSession(out uint pSessionHandle,
int dwSessionFlags, StringBuilder strSessionKey);
private static extern int RmStartSession(
out uint pSessionHandle,
int dwSessionFlags,
char[] strSessionKey);

[DllImport(RestartManagerDll)]
private static extern int RmEndSession(uint pSessionHandle);
Expand Down Expand Up @@ -211,7 +213,11 @@ internal static IEnumerable<ProcessInfo> GetLockingProcessInfos(params string[]
const int maxRetries = 6;

// See http://blogs.msdn.com/b/oldnewthing/archive/2012/02/17/10268840.aspx.
var key = new StringBuilder(new string('\0', CCH_RM_SESSION_KEY + 1));
char[] key = new char[CCH_RM_SESSION_KEY + 1];
for (int i = 0; i < key.Length; i++)
{
key[i] = '\0';
}

int res = RmStartSession(out uint handle, 0, key);
if (res != 0)
Expand Down
16 changes: 10 additions & 6 deletions src/Tasks/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1050,8 +1050,12 @@ internal static extern int CreateAssemblyNameObject(
/// and then again to pass the client-allocated character buffer. StringBuilder is the most straightforward way
/// to allocate a mutable buffer of characters and pass it around.
/// </summary>
/// <param name="cacheFlags">Value that indicates the source of the cached assembly.</param>
/// <param name="cachePath">The returned pointer to the path.</param>
/// <param name="pcchPath">The requested maximum length of CachePath, and upon return, the actual length of CachePath.</param>
///
[DllImport("fusion.dll", CharSet = CharSet.Unicode)]
internal static extern int GetCachePath(AssemblyCacheFlags cacheFlags, StringBuilder cachePath, ref int pcchPath);
internal static extern int GetCachePath(AssemblyCacheFlags cacheFlags, [Out] char[] cachePath, ref int pcchPath);
#endif

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -1117,15 +1121,15 @@ internal static extern int CreateAssemblyNameObject(

#if FEATURE_MSCOREE
/// <summary>
/// Get the runtime version for a given file
/// Get the runtime version for a given file.
/// </summary>
/// <param name="szFullPath">The path of the file to be examined</param>
/// <param name="szFileName">The path of the file to be examined.</param>
/// <param name="szBuffer">The buffer allocated for the version information that is returned.</param>
/// <param name="cchBuffer">The size, in wide characters, of szBuffer</param>
/// <param name="cchBuffer">The size, in wide characters, of szBuffer.</param>
/// <param name="dwLength">The size, in bytes, of the returned szBuffer.</param>
/// <returns>HResult</returns>
/// <returns>HResult.</returns>
[DllImport(MscoreeDLL, SetLastError = true, CharSet = CharSet.Unicode)]
internal static extern uint GetFileVersion(String szFullPath, StringBuilder szBuffer, int cchBuffer, out uint dwLength);
internal static extern uint GetFileVersion([MarshalAs(UnmanagedType.LPWStr)] string szFileName, [Out] char[] szBuffer, int cchBuffer, out int dwLength);
#endif
#endregion

Expand Down