From b8d493a5a92958d532134581e63131b8fd75cd72 Mon Sep 17 00:00:00 2001
From: Lachlan Ennis <2433737+elachlan@users.noreply.github.com>
Date: Wed, 16 Feb 2022 03:57:37 +1000
Subject: [PATCH] CA1838 Avoid 'StringBuilder' parameters for P/Invokes (#7186)
Relates to #7174
https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1838
There is extensive discussion in this PR (#7186) on GitHub.
---
eng/Common.globalconfig | 2 +-
src/Framework/NativeMethods.cs | 50 ++++-----------
.../AssemblyDependency/AssemblyInformation.cs | 45 +++++++------
.../AssemblyDependency/GlobalAssemblyCache.cs | 24 +++++--
src/Tasks/ComReference.cs | 29 ++++++---
src/Tasks/LockCheck.cs | 64 +++++++++++++++++--
src/Tasks/NativeMethods.cs | 20 +++---
src/Tasks/ResolveComReference.cs | 2 +-
8 files changed, 147 insertions(+), 89 deletions(-)
diff --git a/eng/Common.globalconfig b/eng/Common.globalconfig
index c75fbec9493..7f557b4c7ba 100644
--- a/eng/Common.globalconfig
+++ b/eng/Common.globalconfig
@@ -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
diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs
index 9c3a8dbaec5..bf6714d12ee 100644
--- a/src/Framework/NativeMethods.cs
+++ b/src/Framework/NativeMethods.cs
@@ -48,7 +48,6 @@ internal static class NativeMethods
internal const int MAX_PATH = 260;
private const string kernel32Dll = "kernel32.dll";
- private const string mscoreeDLL = "mscoree.dll";
private const string WINDOWS_FILE_SYSTEM_REGISTRY_KEY = @"SYSTEM\CurrentControlSet\Control\FileSystem";
private const string WINDOWS_LONG_PATHS_ENABLED_VALUE_NAME = "LongPathsEnabled";
@@ -950,13 +949,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;
}
}
@@ -989,13 +988,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;
}
}
@@ -1084,7 +1083,7 @@ DateTime LastWriteFileUtcTime(string path)
if (success && (data.fileAttributes & NativeMethods.FILE_ATTRIBUTE_DIRECTORY) == 0)
{
- long dt = ((long) (data.ftLastWriteTimeHigh) << 32) | ((long) data.ftLastWriteTimeLow);
+ long dt = ((long)(data.ftLastWriteTimeHigh) << 32) | ((long)data.ftLastWriteTimeLow);
fileModifiedTime = DateTime.FromFileTimeUtc(dt);
// If file is a symlink _and_ we're not instructed to do the wrong thing, get a more accurate timestamp.
@@ -1468,17 +1467,6 @@ internal static void VerifyThrowWin32Result(int result)
[return: MarshalAs(UnmanagedType.Bool)]
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
- (
- string path,
- string fileName,
- string extension,
- int numBufferChars,
- [Out] StringBuilder buffer,
- int[] filePart
- );
-
[DllImport("kernel32.dll", PreserveSig = true, SetLastError = true)]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern bool FreeLibrary([In] IntPtr module);
@@ -1489,26 +1477,14 @@ int[] filePart
[DllImport("kernel32.dll", CharSet = CharSet.Unicode, PreserveSig = true, SetLastError = true)]
internal static extern IntPtr LoadLibrary(string fileName);
- [DllImport(mscoreeDLL, SetLastError = true, CharSet = CharSet.Unicode)]
- internal static extern uint GetRequestedRuntimeInfo(String pExe,
- String pwszVersion,
- String pConfigurationFile,
- uint startupFlags,
- uint runtimeInfoFlags,
- [Out] StringBuilder pDirectory,
- int dwDirectory,
- out uint dwDirectoryLength,
- [Out] StringBuilder pVersion,
- int cchBuffer,
- out uint dwlength);
-
///
- /// Gets the fully qualified filename of the currently executing .exe
+ /// Gets the fully qualified filename of the currently executing .exe.
///
+ /// of the module for which we are finding the file name.
+ /// The character buffer used to return the file name.
+ /// The length of the buffer.
[DllImport(kernel32Dll, SetLastError = true, CharSet = CharSet.Unicode)]
- internal static extern int GetModuleFileName(
- HandleRef hModule,
- [Out] StringBuilder buffer, int length);
+ internal static extern int GetModuleFileName(HandleRef hModule, [Out] char[] buffer, int length);
[DllImport("kernel32.dll")]
internal static extern IntPtr GetStdHandle(int nStdHandle);
@@ -1557,10 +1533,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 = CharSet.Auto, SetLastError = true)]
internal static extern bool CreatePipe(out SafeFileHandle hReadPipe, out SafeFileHandle hWritePipe, SecurityAttributes lpPipeAttributes, int nSize);
@@ -1648,7 +1624,7 @@ internal static bool MsgWaitOne(this WaitHandle handle, int timeout)
if (!(returnValue == 0 || ((uint)returnValue == RPC_S_CALLPENDING && timeout != Timeout.Infinite)))
{
- throw new InternalErrorException($"Received {returnValue} from CoWaitForMultipleHandles, but expected 0 (S_OK)");
+ throw new InternalErrorException($"Received {returnValue} from CoWaitForMultipleHandles, but expected 0 (S_OK)");
}
return returnValue == 0;
diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs
index 574040bdeac..5e5f39a1656 100644
--- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs
+++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs
@@ -569,30 +569,36 @@ internal static string GetRuntimeVersion(string path)
#if FEATURE_MSCOREE
if (NativeMethodsShared.IsWindows)
{
- StringBuilder runtimeVersion;
- uint hresult;
#if DEBUG
- // Just to make sure and exercise the code that doubles the size
- // every time GetRequestedRuntimeInfo fails due to insufficient buffer size.
+ // Just to make sure and exercise the code that uses dwLength to allocate the buffer
+ // when GetRequestedRuntimeInfo fails due to insufficient buffer size.
int bufferLength = 1;
#else
int bufferLength = 11; // 11 is the length of a runtime version and null terminator v2.0.50727/0
#endif
- do
- {
- runtimeVersion = new StringBuilder(bufferLength);
- hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out _);
- bufferLength *= 2;
- } while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER);
- if (hresult == NativeMethodsShared.S_OK)
+ unsafe
{
- return runtimeVersion.ToString();
- }
- else
- {
- return String.Empty;
- }
+ // Allocate an initial buffer
+ char* runtimeVersion = stackalloc char[bufferLength];
+
+ // Run GetFileVersion, this should succeed using the initial buffer.
+ // It also returns the dwLength which is used if there is insufficient buffer.
+ uint hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out int dwLength);
+
+ if (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER)
+ {
+ // Allocate new buffer based on the returned length.
+ char* runtimeVersion2 = stackalloc char[dwLength];
+ runtimeVersion = runtimeVersion2;
+
+ // Get the RuntimeVersion in this second call.
+ bufferLength = dwLength;
+ hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out dwLength);
+ }
+
+ return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength - 1) : string.Empty;
+ }
}
else
{
@@ -603,7 +609,6 @@ internal static string GetRuntimeVersion(string path)
#endif
}
-
///
/// Import assembly dependencies.
///
@@ -790,7 +795,7 @@ private static AssemblyNameExtension ConstructAssemblyName(IntPtr asmMetaPtr, ch
// Construct the assembly name. (Note asmNameLength should/must be > 0.)
var assemblyName = new AssemblyName
{
- Name = new string(asmNameBuf, 0, (int) asmNameLength - 1),
+ Name = new string(asmNameBuf, 0, (int)asmNameLength - 1),
Version = new Version(
asmMeta.usMajorVersion,
asmMeta.usMinorVersion,
@@ -911,7 +916,7 @@ public static string GetRuntimeVersion(string path)
// Read the PE header signature
sr.BaseStream.Position = peHeaderOffset;
- if (!ReadBytes(sr, (byte) 'P', (byte) 'E', 0, 0))
+ if (!ReadBytes(sr, (byte)'P', (byte)'E', 0, 0))
{
return string.Empty;
}
diff --git a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs
index b7df962c76f..7611767ca12 100644
--- a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs
+++ b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs
@@ -31,6 +31,16 @@ internal static class GlobalAssemblyCache
///
internal static readonly GetGacEnumerator gacEnumerator = GetGacNativeEnumerator;
+ ///
+ /// Lazy loaded cached root path of the GAC.
+ ///
+ private static readonly Lazy _gacPath = new(() => GetGacPath());
+
+ ///
+ /// Gets the root path of the GAC.
+ ///
+ internal static string GacPath => _gacPath.Value;
+
///
/// Given a strong name, find its path in the GAC.
///
@@ -367,16 +377,18 @@ bool specificVersion
}
///
- /// Return the root path of the GAC
+ /// Return the root path of the GAC.
///
internal static string GetGacPath()
{
int gacPathLength = 0;
- NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, null, ref gacPathLength);
- StringBuilder gacPath = new StringBuilder(gacPathLength);
- NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, gacPath, ref gacPathLength);
-
- return gacPath.ToString();
+ unsafe
+ {
+ NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, null, ref gacPathLength);
+ char* gacPath = stackalloc char[gacPathLength];
+ NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, gacPath, ref gacPathLength);
+ return new string(gacPath, 0, gacPathLength - 1);
+ }
}
}
}
diff --git a/src/Tasks/ComReference.cs b/src/Tasks/ComReference.cs
index cea831d5798..eaf732c4ec4 100644
--- a/src/Tasks/ComReference.cs
+++ b/src/Tasks/ComReference.cs
@@ -406,22 +406,33 @@ internal static string StripTypeLibNumberFromPath(string typeLibPath, FileExists
private static string GetModuleFileName(IntPtr handle)
{
- bool success = false;
- var buffer = new StringBuilder();
+ char[] buffer = null;
// Try increased buffer sizes if on longpath-enabled Windows
- for (int bufferSize = NativeMethodsShared.MAX_PATH; !success && bufferSize <= NativeMethodsShared.MaxPath; bufferSize *= 2)
+ for (int bufferSize = NativeMethodsShared.MAX_PATH; bufferSize <= NativeMethodsShared.MaxPath; bufferSize *= 2)
{
- buffer.EnsureCapacity(bufferSize);
+ buffer = System.Buffers.ArrayPool.Shared.Rent(bufferSize);
+ try
+ {
+ var handleRef = new System.Runtime.InteropServices.HandleRef(buffer, handle);
+ int pathLength = NativeMethodsShared.GetModuleFileName(handleRef, buffer, bufferSize);
- var handleRef = new System.Runtime.InteropServices.HandleRef(buffer, handle);
- int pathLength = NativeMethodsShared.GetModuleFileName(handleRef, buffer, buffer.Capacity);
+ bool isBufferTooSmall = (uint)Marshal.GetLastWin32Error() == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER;
+ if (pathLength != 0 && !isBufferTooSmall)
+ {
+ return new string(buffer, 0, pathLength);
+ }
+ }
+ finally
+ {
+ System.Buffers.ArrayPool.Shared.Return(buffer);
+ }
- bool isBufferTooSmall = ((uint)Marshal.GetLastWin32Error() == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER);
- success = pathLength != 0 && !isBufferTooSmall;
+ // Double check that the buffer is not insanely big
+ ErrorUtilities.VerifyThrow(bufferSize <= int.MaxValue / 2, "Buffer size approaching int.MaxValue");
}
- return success ? buffer.ToString() : string.Empty;
+ return string.Empty;
}
///
diff --git a/src/Tasks/LockCheck.cs b/src/Tasks/LockCheck.cs
index fcd8a41a300..9ef5cdd62d8 100644
--- a/src/Tasks/LockCheck.cs
+++ b/src/Tasks/LockCheck.cs
@@ -55,10 +55,57 @@ internal enum ApplicationType
uint nServices,
string[] rgsServiceNames);
+ ///
+ /// Starts a new Restart Manager session.
+ /// A maximum of 64 Restart Manager sessions per user session
+ /// can be open on the system at the same time. When this
+ /// function starts a session, it returns a session handle
+ /// and session key that can be used in subsequent calls to
+ /// the Restart Manager API.
+ ///
+ ///
+ /// A pointer to the handle of a Restart Manager session.
+ /// The session handle can be passed in subsequent calls
+ /// to the Restart Manager API.
+ ///
+ ///
+ /// Reserved. This parameter should be 0.
+ ///
+ ///
+ /// A null-terminated string that contains the session key
+ /// to the new session. The string must be allocated before
+ /// calling the RmStartSession function.
+ ///
+ /// System error codes that are defined in Winerror.h.
+ ///
+ /// The RmStartSession function doesn’t properly null-terminate
+ /// the session key, even though the function is documented as
+ /// returning a null-terminated string. To work around this bug,
+ /// we pre-fill the buffer with null characters so that whatever
+ /// ends gets written will have a null terminator (namely, one of
+ /// the null characters we placed ahead of time).
+ ///
+ /// see .
+ ///
+ ///
[DllImport(RestartManagerDll, CharSet = CharSet.Unicode)]
- private static extern int RmStartSession(out uint pSessionHandle,
- int dwSessionFlags, StringBuilder strSessionKey);
-
+ private static extern unsafe int RmStartSession(
+ out uint pSessionHandle,
+ int dwSessionFlags,
+ char* strSessionKey);
+
+ ///
+ /// Ends the Restart Manager session.
+ /// This function should be called by the primary installer that
+ /// has previously started the session by calling the
+ /// function. The RmEndSession function can be called by a secondary installer
+ /// that is joined to the session once no more resources need to be registered
+ /// by the secondary installer.
+ ///
+ /// A handle to an existing Restart Manager session.
+ ///
+ /// The function can return one of the system error codes that are defined in Winerror.h.
+ ///
[DllImport(RestartManagerDll)]
private static extern int RmEndSession(uint pSessionHandle);
@@ -207,11 +254,16 @@ internal static IEnumerable GetLockingProcessInfos(params string[]
}
const int maxRetries = 6;
+ uint handle;
+ int res;
- // 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));
+ unsafe
+ {
+ // See http://blogs.msdn.com/b/oldnewthing/archive/2012/02/17/10268840.aspx.
+ char* key = stackalloc char[CCH_RM_SESSION_KEY + 1];
+ res = RmStartSession(out handle, 0, key);
+ }
- int res = RmStartSession(out uint handle, 0, key);
if (res != 0)
{
throw GetException(res, "RmStartSession", "Failed to begin restart manager session.");
diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs
index 45bc7d9faad..54b21a7a904 100644
--- a/src/Tasks/NativeMethods.cs
+++ b/src/Tasks/NativeMethods.cs
@@ -1053,13 +1053,15 @@ internal static bool AllDrivesMapped()
///
/// GetCachePath from fusion.dll.
- /// Using StringBuilder here is a way to pass a preallocated buffer of characters to (native) functions that require it.
/// A common design pattern in unmanaged C++ is calling a function twice, once to determine the length of the string
- /// 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.
+ /// and then again to pass the client-allocated character buffer.
///
+ /// Value that indicates the source of the cached assembly.
+ /// The returned pointer to the path.
+ /// The requested maximum length of CachePath, and upon return, the actual length of CachePath.
+ ///
[DllImport("fusion.dll", CharSet = CharSet.Unicode)]
- internal static extern int GetCachePath(AssemblyCacheFlags cacheFlags, StringBuilder cachePath, ref int pcchPath);
+ internal static extern unsafe int GetCachePath(AssemblyCacheFlags cacheFlags, [Out] char* cachePath, ref int pcchPath);
#endif
//------------------------------------------------------------------------------
@@ -1125,15 +1127,15 @@ internal static bool AllDrivesMapped()
#if FEATURE_MSCOREE
///
- /// Get the runtime version for a given file
+ /// Get the runtime version for a given file.
///
- /// The path of the file to be examined
+ /// The path of the file to be examined.
/// The buffer allocated for the version information that is returned.
- /// The size, in wide characters, of szBuffer
+ /// The size, in wide characters, of szBuffer.
/// The size, in bytes, of the returned szBuffer.
- /// HResult
+ /// HResult.
[DllImport(MscoreeDLL, SetLastError = true, CharSet = CharSet.Unicode)]
- internal static extern uint GetFileVersion(String szFullPath, StringBuilder szBuffer, int cchBuffer, out uint dwLength);
+ internal static extern unsafe uint GetFileVersion([MarshalAs(UnmanagedType.LPWStr)] string szFileName, [Out] char* szBuffer, int cchBuffer, out int dwLength);
#endif
#endregion
diff --git a/src/Tasks/ResolveComReference.cs b/src/Tasks/ResolveComReference.cs
index afa2a75cb88..911d84afb0b 100644
--- a/src/Tasks/ResolveComReference.cs
+++ b/src/Tasks/ResolveComReference.cs
@@ -432,7 +432,7 @@ public override bool Execute()
}
}
- SetCopyLocalToFalseOnGacOrNoPIAAssemblies(resolvedReferenceList, GlobalAssemblyCache.GetGacPath());
+ SetCopyLocalToFalseOnGacOrNoPIAAssemblies(resolvedReferenceList, GlobalAssemblyCache.GacPath);
ResolvedModules = moduleList.ToArray();
ResolvedFiles = resolvedReferenceList.ToArray();