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

@@ -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

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);
-
     /// <summary>
-    /// Gets the fully qualified filename of the currently executing .exe
+    /// Gets the fully qualified filename of the currently executing .exe.
     /// </summary>
+    /// <param name="hModule"><see cref="HandleRef"/> of the module for which we are finding the file name.</param>
+    /// <param name="buffer">The character buffer used to return the file name.</param>
+    /// <param name="length">The length of the buffer.</param>
     [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;

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
         }
 
-
         /// <summary>
         /// Import assembly dependencies.
         /// </summary>
@@ -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;
                     }

src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs

@@ -31,6 +31,16 @@ internal static class GlobalAssemblyCache
         /// </summary>
         internal static readonly GetGacEnumerator gacEnumerator = GetGacNativeEnumerator;
 
+        /// <summary>
+        /// Lazy loaded cached root path of the GAC.
+        /// </summary>
+        private static readonly Lazy<string> _gacPath = new(() => GetGacPath());
+
+        /// <summary>
+        /// Gets the root path of the GAC.
+        /// </summary>
+        internal static string GacPath => _gacPath.Value;
+
         /// <summary>
         /// Given a strong name, find its path in the GAC.
         /// </summary>
@@ -367,16 +377,18 @@ 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);
-            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);
+            }
         }
     }
 }

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<char>.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<char>.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;
         }
 
         /// <summary>

src/Tasks/LockCheck.cs

@@ -55,10 +55,57 @@ internal enum ApplicationType
             uint nServices,
             string[] rgsServiceNames);
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        /// <param name="pSessionHandle">
+        /// A pointer to the handle of a Restart Manager session.
+        /// The session handle can be passed in subsequent calls
+        /// to the Restart Manager API.
+        /// </param>
+        /// <param name="dwSessionFlags">
+        /// Reserved. This parameter should be 0.
+        /// </param>
+        /// <param name="strSessionKey">
+        /// A null-terminated string that contains the session key
+        /// to the new session. The string must be allocated before
+        /// calling the RmStartSession function.
+        /// </param>
+        /// <returns>System error codes that are defined in Winerror.h.</returns>
+        /// <remarks>
+        /// The Rm­­StartSession 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).
+        /// <para>
+        /// see <see href="http://blogs.msdn.com/b/oldnewthing/archive/2012/02/17/10268840.aspx"/>.
+        /// </para>
+        /// </remarks>
         [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);
+
+        /// <summary>
+        /// Ends the Restart Manager session.
+        /// This function should be called by the primary installer that
+        /// has previously started the session by calling the <see cref="RmStartSession"/>
+        /// 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.
+        /// </summary>
+        /// <param name="pSessionHandle">A handle to an existing Restart Manager session.</param>
+        /// <returns>
+        /// The function can return one of the system error codes that are defined in Winerror.h.
+        /// </returns>
         [DllImport(RestartManagerDll)]
         private static extern int RmEndSession(uint pSessionHandle);
 
@@ -207,11 +254,16 @@ internal static IEnumerable<ProcessInfo> 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.");