From 994b6969031a9969b40fae9686e76fb0d92cee00 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Thu, 30 Dec 2021 13:35:50 +1000 Subject: [PATCH 01/21] CA1838 Avoid 'StringBuilder' parameters for P/Invokes --- eng/CodeAnalysis.ruleset | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/CodeAnalysis.ruleset b/eng/CodeAnalysis.ruleset index 2078c42fe6c..3d4a7e3a814 100644 --- a/eng/CodeAnalysis.ruleset +++ b/eng/CodeAnalysis.ruleset @@ -108,7 +108,7 @@ - + From 4b519a9226a4b6261e75bf0415022979872b3b31 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Sat, 8 Jan 2022 09:56:22 +1000 Subject: [PATCH 02/21] revert ruleset change --- eng/CodeAnalysis.ruleset | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/CodeAnalysis.ruleset b/eng/CodeAnalysis.ruleset index 3d4a7e3a814..2078c42fe6c 100644 --- a/eng/CodeAnalysis.ruleset +++ b/eng/CodeAnalysis.ruleset @@ -108,7 +108,7 @@ - + From 638775f36fdfc910db341b9c8291a0cf1496454c Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Sat, 8 Jan 2022 09:57:15 +1000 Subject: [PATCH 03/21] Enable warning on CA1838 --- eng/Common.globalconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/Common.globalconfig b/eng/Common.globalconfig index fd878420d57..24e3204ef30 100644 --- a/eng/Common.globalconfig +++ b/eng/Common.globalconfig @@ -319,7 +319,7 @@ dotnet_diagnostic.CA1836.severity = suggestion 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 From 661c2c82cfd53e47ecd63d2b86aee6b42079008a Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Wed, 12 Jan 2022 08:01:35 +1000 Subject: [PATCH 04/21] CA1838 Avoid 'StringBuilder' parameters for P/Invokes. Consider using a character buffer instead. --- src/Framework/NativeMethods.cs | 24 +++++++++---------- .../AssemblyDependency/AssemblyInformation.cs | 11 +++++---- .../AssemblyDependency/GlobalAssemblyCache.cs | 4 ++-- src/Tasks/ComReference.cs | 18 +++++++++----- src/Tasks/LockCheck.cs | 9 ++++--- src/Tasks/NativeMethods.cs | 4 ++-- 6 files changed, 39 insertions(+), 31 deletions(-) diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index 886fe7aa982..a30a121ddd1 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -959,7 +959,7 @@ 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(); @@ -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); path = fullPath; } } @@ -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)] @@ -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); @@ -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); @@ -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); diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index 8a6d3f7a516..2c403c8a03a 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -567,7 +567,7 @@ internal static string GetRuntimeVersion(string path) #if FEATURE_MSCOREE if (NativeMethodsShared.IsWindows) { - StringBuilder runtimeVersion; + char[] runtimeVersion; uint hresult; #if DEBUG // Just to make sure and exercise the code that doubles the size @@ -578,18 +578,19 @@ internal static string GetRuntimeVersion(string path) #endif do { - runtimeVersion = new StringBuilder(bufferLength); + runtimeVersion = new char[bufferLength]; hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out _); bufferLength *= 2; - } while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER); + } + while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER); if (hresult == NativeMethodsShared.S_OK) { - return runtimeVersion.ToString(); + return new string(runtimeVersion); } else { - return String.Empty; + return string.Empty; } } else diff --git a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs index b7df962c76f..07cfc2ea710 100644 --- a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs +++ b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs @@ -373,10 +373,10 @@ 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); } } } diff --git a/src/Tasks/ComReference.cs b/src/Tasks/ComReference.cs index cea831d5798..f509ead0aa2 100644 --- a/src/Tasks/ComReference.cs +++ b/src/Tasks/ComReference.cs @@ -407,21 +407,27 @@ 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) { - 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, buffer.Capacity); + int pathLength = NativeMethodsShared.GetModuleFileName(handleRef, buffer, bufferSize); - bool isBufferTooSmall = ((uint)Marshal.GetLastWin32Error() == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER); + bool isBufferTooSmall = (uint)Marshal.GetLastWin32Error() == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER; success = pathLength != 0 && !isBufferTooSmall; + } + finally + { + System.Buffers.ArrayPool.Shared.Return(buffer); + } } - return success ? buffer.ToString() : string.Empty; + return success ? new string(buffer) : string.Empty; } /// diff --git a/src/Tasks/LockCheck.cs b/src/Tasks/LockCheck.cs index 566553eff41..dce6ee7fbbc 100644 --- a/src/Tasks/LockCheck.cs +++ b/src/Tasks/LockCheck.cs @@ -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); @@ -211,7 +213,8 @@ internal static IEnumerable 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]; + key[0] = '\0'; int res = RmStartSession(out uint handle, 0, key); if (res != 0) diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index f2308684706..fb571105567 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -1051,7 +1051,7 @@ internal static extern int CreateAssemblyNameObject( /// to allocate a mutable buffer of characters and pass it around. /// [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, char[] cachePath, ref int pcchPath); #endif //------------------------------------------------------------------------------ @@ -1125,7 +1125,7 @@ internal static extern int CreateAssemblyNameObject( /// The size, in bytes, of the returned szBuffer. /// 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 uint GetFileVersion(string szFullPath, char[] szBuffer, int cchBuffer, out uint dwLength); #endif #endregion From 4f0424ca521013696585e33551ba7d02e9f46cdc Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Thu, 13 Jan 2022 09:33:41 +1000 Subject: [PATCH 05/21] trying again --- .../AssemblyDependency/GlobalAssemblyCache.cs | 2 +- src/Tasks/NativeMethods.cs | 16 ++++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs index 07cfc2ea710..d5844863917 100644 --- a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs +++ b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs @@ -373,7 +373,7 @@ internal static string GetGacPath() { int gacPathLength = 0; NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, null, ref gacPathLength); - char[] gacPath = new char[gacPathLength]; + char[] gacPath = new char[gacPathLength + 1]; NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, gacPath, ref gacPathLength); return new string(gacPath); diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index fb571105567..a0b076cd0fd 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -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. /// + /// 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, char[] cachePath, ref int pcchPath); + internal static extern int GetCachePath([In] AssemblyCacheFlags cacheFlags, char[] cachePath, ref int pcchPath); #endif //------------------------------------------------------------------------------ @@ -1117,15 +1121,15 @@ internal static extern int CreateAssemblyNameObject( #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, char[] szBuffer, int cchBuffer, out uint dwLength); + internal static extern uint GetFileVersion([MarshalAs(UnmanagedType.LPWStr)] string szFileName, char[] szBuffer, int cchBuffer, out int dwLength); #endif #endregion From e042e6632a89b28abb45b9aa58f4856d8d9826ba Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 28 Jan 2022 08:39:27 +1000 Subject: [PATCH 06/21] Changes from review --- src/Framework/NativeMethods.cs | 2 +- src/Tasks/AssemblyDependency/AssemblyInformation.cs | 12 +++--------- src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs | 4 ++-- src/Tasks/ComReference.cs | 11 ++++++----- src/Tasks/LockCheck.cs | 5 ++++- src/Tasks/NativeMethods.cs | 4 ++-- 6 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index a30a121ddd1..310cdfd322c 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -965,7 +965,7 @@ internal static string GetShortFilePath(string path) if (length > 0) { - string fullPath = fullPathBuffer.ToString(); + string fullPath = new(fullPathBuffer); path = fullPath; } } diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index 2c403c8a03a..8df16fe9312 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -569,6 +569,7 @@ internal static string GetRuntimeVersion(string path) { 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. @@ -579,19 +580,12 @@ internal static string GetRuntimeVersion(string path) do { runtimeVersion = new char[bufferLength]; - hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out _); + hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out dwLength); bufferLength *= 2; } while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER); - if (hresult == NativeMethodsShared.S_OK) - { - return new string(runtimeVersion); - } - else - { - return string.Empty; - } + return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength) : string.Empty; } else { diff --git a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs index d5844863917..8b4042a5c23 100644 --- a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs +++ b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs @@ -373,10 +373,10 @@ internal static string GetGacPath() { int gacPathLength = 0; NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, null, ref gacPathLength); - char[] gacPath = new char[gacPathLength + 1]; + char[] gacPath = new char[gacPathLength]; NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, gacPath, ref gacPathLength); - return new string(gacPath); + return new string(gacPath,0, gacPathLength); } } } diff --git a/src/Tasks/ComReference.cs b/src/Tasks/ComReference.cs index f509ead0aa2..b2ca45bb4e7 100644 --- a/src/Tasks/ComReference.cs +++ b/src/Tasks/ComReference.cs @@ -408,6 +408,7 @@ private static string GetModuleFileName(IntPtr handle) { bool success = false; 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) @@ -415,11 +416,11 @@ private static string GetModuleFileName(IntPtr handle) 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); + 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 { @@ -427,7 +428,7 @@ private static string GetModuleFileName(IntPtr handle) } } - return success ? new string(buffer) : string.Empty; + return success ? new string(buffer, 0, pathLength) : string.Empty; } /// diff --git a/src/Tasks/LockCheck.cs b/src/Tasks/LockCheck.cs index dce6ee7fbbc..d4177c23cd8 100644 --- a/src/Tasks/LockCheck.cs +++ b/src/Tasks/LockCheck.cs @@ -214,7 +214,10 @@ internal static IEnumerable GetLockingProcessInfos(params string[] // See http://blogs.msdn.com/b/oldnewthing/archive/2012/02/17/10268840.aspx. char[] key = new char[CCH_RM_SESSION_KEY + 1]; - key[0] = '\0'; + for (int i = 0; i < key.Length; i++) + { + key[i] = '\0'; + } int res = RmStartSession(out uint handle, 0, key); if (res != 0) diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index a0b076cd0fd..0553eb58d2a 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -1055,7 +1055,7 @@ internal static extern int CreateAssemblyNameObject( /// 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([In] AssemblyCacheFlags cacheFlags, char[] cachePath, ref int pcchPath); + internal static extern int GetCachePath(AssemblyCacheFlags cacheFlags, [Out] char[] cachePath, ref int pcchPath); #endif //------------------------------------------------------------------------------ @@ -1129,7 +1129,7 @@ internal static extern int CreateAssemblyNameObject( /// The size, in bytes, of the returned szBuffer. /// HResult. [DllImport(MscoreeDLL, SetLastError = true, CharSet = CharSet.Unicode)] - internal static extern uint GetFileVersion([MarshalAs(UnmanagedType.LPWStr)] string szFileName, char[] szBuffer, int cchBuffer, out int dwLength); + internal static extern uint GetFileVersion([MarshalAs(UnmanagedType.LPWStr)] string szFileName, [Out] char[] szBuffer, int cchBuffer, out int dwLength); #endif #endregion From 4ddf6610e43837e39bf4081b7ca9f9b6c72f7937 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 28 Jan 2022 09:05:38 +1000 Subject: [PATCH 07/21] GetGacPath changes --- src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs index 8b4042a5c23..ee4e6a5e41e 100644 --- a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs +++ b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs @@ -367,16 +367,16 @@ 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); - char[] gacPath = new char[gacPathLength]; + char[] gacPath = new char[gacPathLength + 1]; NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, gacPath, ref gacPathLength); - return new string(gacPath,0, gacPathLength); + return new string(gacPath, 0, gacPathLength); } } } From 8f0c31c670f1e6080bd236a7796fc83065b7d697 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 28 Jan 2022 12:31:32 +1000 Subject: [PATCH 08/21] Making sure we are form strings properly --- src/Framework/NativeMethods.cs | 4 ++-- src/Tasks/AssemblyDependency/AssemblyInformation.cs | 2 +- src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index 310cdfd322c..e04ad031845 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -965,7 +965,7 @@ internal static string GetShortFilePath(string path) if (length > 0) { - string fullPath = new(fullPathBuffer); + string fullPath = new(fullPathBuffer, 0, length); path = fullPath; } } @@ -1004,7 +1004,7 @@ internal static string GetLongFilePath(string path) if (length > 0) { - string fullPath = new(fullPathBuffer); + string fullPath = new(fullPathBuffer, 0, length); path = fullPath; } } diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index 8df16fe9312..32129b253a2 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -585,7 +585,7 @@ internal static string GetRuntimeVersion(string path) } while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER); - return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength) : string.Empty; + return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength-1) : string.Empty; } else { diff --git a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs index ee4e6a5e41e..3a4b583231b 100644 --- a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs +++ b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs @@ -373,10 +373,10 @@ internal static string GetGacPath() { int gacPathLength = 0; NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, null, ref gacPathLength); - char[] gacPath = new char[gacPathLength + 1]; + char[] gacPath = new char[gacPathLength]; NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, gacPath, ref gacPathLength); - return new string(gacPath, 0, gacPathLength); + return new string(gacPath, 0, gacPathLength-1); } } } From 04e4a1bea5413d75c0e22162c5737155fbc151f1 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 28 Jan 2022 14:13:50 +1000 Subject: [PATCH 09/21] changes from review --- src/Framework/NativeMethods.cs | 84 ++++++++----------- .../AssemblyDependency/AssemblyInformation.cs | 2 +- .../AssemblyDependency/GlobalAssemblyCache.cs | 12 +-- src/Tasks/LockCheck.cs | 15 ++-- src/Tasks/NativeMethods.cs | 2 +- 5 files changed, 51 insertions(+), 64 deletions(-) diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index e04ad031845..2ae8399f5f9 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -954,25 +954,28 @@ internal static string GetShortFilePath(string path) if (path != null) { - int length = GetShortPathName(path, null, 0); - int errorCode = Marshal.GetLastWin32Error(); - - if (length > 0) + unsafe { - char[] fullPathBuffer = new char[length]; - length = GetShortPathName(path, fullPathBuffer, length); - errorCode = Marshal.GetLastWin32Error(); + int length = GetShortPathName(path, null, 0); + int errorCode = Marshal.GetLastWin32Error(); if (length > 0) { - string fullPath = new(fullPathBuffer, 0, length); - path = fullPath; + char* fullPathBuffer = stackalloc char[length]; + length = GetShortPathName(path, fullPathBuffer, length); + errorCode = Marshal.GetLastWin32Error(); + + if (length > 0) + { + string fullPath = new(fullPathBuffer, 0, length); + path = fullPath; + } } - } - if (length == 0 && errorCode != 0) - { - ThrowExceptionForErrorCode(errorCode); + if (length == 0 && errorCode != 0) + { + ThrowExceptionForErrorCode(errorCode); + } } } @@ -993,25 +996,28 @@ internal static string GetLongFilePath(string path) if (path != null) { - int length = GetLongPathName(path, null, 0); - int errorCode = Marshal.GetLastWin32Error(); - - if (length > 0) + unsafe { - char[] fullPathBuffer = new char[length]; - length = GetLongPathName(path, fullPathBuffer, length); - errorCode = Marshal.GetLastWin32Error(); + int length = GetLongPathName(path, null, 0); + int errorCode = Marshal.GetLastWin32Error(); if (length > 0) { - string fullPath = new(fullPathBuffer, 0, length); - path = fullPath; + char* fullPathBuffer = stackalloc char[length]; + length = GetLongPathName(path, fullPathBuffer, length); + errorCode = Marshal.GetLastWin32Error(); + + if (length > 0) + { + string fullPath = new(fullPathBuffer, 0, length); + path = fullPath; + } } - } - if (length == 0 && errorCode != 0) - { - ThrowExceptionForErrorCode(errorCode); + if (length == 0 && errorCode != 0) + { + ThrowExceptionForErrorCode(errorCode); + } } } @@ -1477,15 +1483,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] char[] buffer, - int[] filePart); - [DllImport("kernel32.dll", PreserveSig = true, SetLastError = true)] [return: MarshalAs(UnmanagedType.Bool)] internal static extern bool FreeLibrary([In] IntPtr module); @@ -1496,19 +1493,6 @@ private static extern uint SearchPath( [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] char[] pDirectory, - int dwDirectory, - out uint dwDirectoryLength, - [Out] char[] pVersion, - int cchBuffer, - out uint dwlength); - /// /// Gets the fully qualified filename of the currently executing .exe /// @@ -1568,10 +1552,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] char[] fullpath, [In] int length); + internal static extern unsafe 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] char[] fullpath, [In] int length); + internal static extern unsafe 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); diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index 32129b253a2..dd6e98636f4 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -585,7 +585,7 @@ internal static string GetRuntimeVersion(string path) } while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER); - return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength-1) : string.Empty; + return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength - 1) : string.Empty; } else { diff --git a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs index 3a4b583231b..758b928ed37 100644 --- a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs +++ b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs @@ -372,11 +372,13 @@ bool specificVersion internal static string GetGacPath() { int gacPathLength = 0; - NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, null, ref gacPathLength); - char[] gacPath = new char[gacPathLength]; - NativeMethods.GetCachePath(AssemblyCacheFlags.GAC, gacPath, ref gacPathLength); - - return new string(gacPath, 0, gacPathLength-1); + 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/LockCheck.cs b/src/Tasks/LockCheck.cs index d4177c23cd8..964a433c4e5 100644 --- a/src/Tasks/LockCheck.cs +++ b/src/Tasks/LockCheck.cs @@ -56,10 +56,10 @@ private static extern int RmRegisterResources(uint pSessionHandle, string[] rgsServiceNames); [DllImport(RestartManagerDll, CharSet = CharSet.Unicode)] - private static extern int RmStartSession( + private static extern unsafe int RmStartSession( out uint pSessionHandle, int dwSessionFlags, - char[] strSessionKey); + char* strSessionKey); [DllImport(RestartManagerDll)] private static extern int RmEndSession(uint pSessionHandle); @@ -211,15 +211,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. - char[] key = new char[CCH_RM_SESSION_KEY + 1]; - for (int i = 0; i < key.Length; i++) + unsafe { - key[i] = '\0'; + // 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 0553eb58d2a..5315168e7c9 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -1055,7 +1055,7 @@ internal static extern int CreateAssemblyNameObject( /// 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, [Out] char[] cachePath, ref int pcchPath); + internal static extern unsafe int GetCachePath(AssemblyCacheFlags cacheFlags, [Out] char* cachePath, ref int pcchPath); #endif //------------------------------------------------------------------------------ From 903f35ddc54b9c03d0b99b52de14c7fb4dfdd2ed Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 28 Jan 2022 14:17:50 +1000 Subject: [PATCH 10/21] Remove unused field --- src/Framework/NativeMethods.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index 2ae8399f5f9..74b8e80491c 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -49,7 +49,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"; From 69cf05fce07361bb16957bf909567d19f59185ee Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 28 Jan 2022 15:40:05 +1000 Subject: [PATCH 11/21] Use ArrayPool in GetRuntimeVersion and fix usage in GetModuleFileName --- .../AssemblyDependency/AssemblyInformation.cs | 25 +++++++++++++------ src/Tasks/ComReference.cs | 10 +++++--- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index dd6e98636f4..4d3ee3c27fe 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -569,7 +569,7 @@ internal static string GetRuntimeVersion(string path) { char[] runtimeVersion; uint hresult; - int dwLength; + string output = string.Empty; #if DEBUG // Just to make sure and exercise the code that doubles the size // every time GetRequestedRuntimeInfo fails due to insufficient buffer size. @@ -579,13 +579,24 @@ internal static string GetRuntimeVersion(string path) #endif do { - runtimeVersion = new char[bufferLength]; - hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out dwLength); - bufferLength *= 2; + runtimeVersion = System.Buffers.ArrayPool.Shared.Rent(bufferLength); + try + { + hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out int dwLength); + bufferLength *= 2; + if (hresult == NativeMethodsShared.S_OK) + { + output = new string(runtimeVersion, 0, dwLength - 1); + } + } + finally + { + System.Buffers.ArrayPool.Shared.Return(runtimeVersion); + } } while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER); - return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength - 1) : string.Empty; + return output; } else { @@ -783,7 +794,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, @@ -904,7 +915,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/ComReference.cs b/src/Tasks/ComReference.cs index b2ca45bb4e7..e480cff968e 100644 --- a/src/Tasks/ComReference.cs +++ b/src/Tasks/ComReference.cs @@ -408,7 +408,7 @@ private static string GetModuleFileName(IntPtr handle) { bool success = false; char[] buffer = null; - int pathLength = 0; + string output = string.Empty; // Try increased buffer sizes if on longpath-enabled Windows for (int bufferSize = NativeMethodsShared.MAX_PATH; !success && bufferSize <= NativeMethodsShared.MaxPath; bufferSize *= 2) @@ -417,10 +417,14 @@ private static string GetModuleFileName(IntPtr handle) try { var handleRef = new System.Runtime.InteropServices.HandleRef(buffer, handle); - pathLength = NativeMethodsShared.GetModuleFileName(handleRef, buffer, bufferSize); + int pathLength = NativeMethodsShared.GetModuleFileName(handleRef, buffer, bufferSize); bool isBufferTooSmall = (uint)Marshal.GetLastWin32Error() == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER; success = pathLength != 0 && !isBufferTooSmall; + if (success) + { + output = new string(buffer, 0, pathLength); + } } finally { @@ -428,7 +432,7 @@ private static string GetModuleFileName(IntPtr handle) } } - return success ? new string(buffer, 0, pathLength) : string.Empty; + return output; } /// From 300179b8b434ffad4f94197160ff03824233b9f9 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Sat, 29 Jan 2022 09:12:11 +1000 Subject: [PATCH 12/21] Refactor GetRuntimeVersion to remove loop and use stackalloc --- .../AssemblyDependency/AssemblyInformation.cs | 42 ++++++------------- src/Tasks/NativeMethods.cs | 2 +- 2 files changed, 14 insertions(+), 30 deletions(-) diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index 4d3ee3c27fe..c304bdbe7e4 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -567,36 +567,20 @@ internal static string GetRuntimeVersion(string path) #if FEATURE_MSCOREE if (NativeMethodsShared.IsWindows) { - char[] runtimeVersion; - uint hresult; - string output = string.Empty; -#if DEBUG - // Just to make sure and exercise the code that doubles the size - // every time 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 + unsafe { - runtimeVersion = System.Buffers.ArrayPool.Shared.Rent(bufferLength); - try - { - hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out int dwLength); - bufferLength *= 2; - if (hresult == NativeMethodsShared.S_OK) - { - output = new string(runtimeVersion, 0, dwLength - 1); - } - } - finally - { - System.Buffers.ArrayPool.Shared.Return(runtimeVersion); - } - } - while (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER); - - return output; + // Run the first GetFileVersion to get the required buffer size. + int bufferLength = 1; + uint hresult = NativeMethods.GetFileVersion(path, null, bufferLength, out int dwLength); + + // Allocate buffer based on the returned length. + bufferLength = dwLength; + char* runtimeVersion = stackalloc char[dwLength]; + + // Get the RuntimeVersion in this second call. + hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out dwLength); + return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength - 1) : string.Empty; + } } else { diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index 5315168e7c9..539356d0861 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -1129,7 +1129,7 @@ internal static extern int CreateAssemblyNameObject( /// The size, in bytes, of the returned szBuffer. /// HResult. [DllImport(MscoreeDLL, SetLastError = true, CharSet = CharSet.Unicode)] - internal static extern uint GetFileVersion([MarshalAs(UnmanagedType.LPWStr)] string szFileName, [Out] char[] szBuffer, int cchBuffer, out int dwLength); + internal static extern unsafe uint GetFileVersion([MarshalAs(UnmanagedType.LPWStr)] string szFileName, [Out] char* szBuffer, int cchBuffer, out int dwLength); #endif #endregion From c392277686c3bd07d8e7b1a10bf35817d25b9369 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Tue, 1 Feb 2022 09:14:45 +1000 Subject: [PATCH 13/21] changes from review --- .../AssemblyDependency/AssemblyInformation.cs | 46 +++++++++++++------ src/Tasks/LockCheck.cs | 7 +++ 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index c304bdbe7e4..a72a6411675 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -567,19 +567,35 @@ internal static string GetRuntimeVersion(string path) #if FEATURE_MSCOREE if (NativeMethodsShared.IsWindows) { +#if DEBUG + // 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 + unsafe { - // Run the first GetFileVersion to get the required buffer size. - int bufferLength = 1; - uint hresult = NativeMethods.GetFileVersion(path, null, bufferLength, out int dwLength); + // Allocate an initial buffer + char* runtimeVersionInitial = stackalloc char[bufferLength]; - // Allocate buffer based on the returned length. - bufferLength = dwLength; - char* runtimeVersion = stackalloc char[dwLength]; + // 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, runtimeVersionInitial, bufferLength, out int dwLength); - // Get the RuntimeVersion in this second call. - hresult = NativeMethods.GetFileVersion(path, runtimeVersion, bufferLength, out dwLength); - return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength - 1) : string.Empty; + if (hresult == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER) + { + // Allocate new buffer based on the returned length. + char* runtimeVersion = stackalloc char[dwLength]; + + // 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; + } + + return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersionInitial, 0, dwLength - 1) : string.Empty; } } else @@ -589,14 +605,14 @@ internal static string GetRuntimeVersion(string path) #else return ManagedRuntimeVersionReader.GetRuntimeVersion(path); #endif - } + } - /// - /// Import assembly dependencies. - /// - /// The array of assembly dependencies. - private AssemblyNameExtension[] ImportAssemblyDependencies() + /// + /// Import assembly dependencies. + /// + /// The array of assembly dependencies. + private AssemblyNameExtension[] ImportAssemblyDependencies() { #if !FEATURE_ASSEMBLYLOADCONTEXT var asmRefs = new List(); diff --git a/src/Tasks/LockCheck.cs b/src/Tasks/LockCheck.cs index 964a433c4e5..5347d235baf 100644 --- a/src/Tasks/LockCheck.cs +++ b/src/Tasks/LockCheck.cs @@ -218,6 +218,13 @@ internal static IEnumerable GetLockingProcessInfos(params string[] { // See http://blogs.msdn.com/b/oldnewthing/archive/2012/02/17/10268840.aspx. char* key = stackalloc char[CCH_RM_SESSION_KEY + 1]; + + // Explicitly zero initialize the buffer. + for(int i = 0; i < (CCH_RM_SESSION_KEY + 1); i++) + { + key[i] = '\0'; + } + res = RmStartSession(out handle, 0, key); } From de3618c2d86993600f44b5a611af63765c1700da Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Tue, 1 Feb 2022 09:23:48 +1000 Subject: [PATCH 14/21] Fix conflict and xml doc --- src/Framework/NativeMethods.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index 74b8e80491c..0cb0a4a99bd 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -1493,16 +1493,13 @@ internal static void VerifyThrowWin32Result(int result) internal static extern IntPtr LoadLibrary(string fileName); /// - /// 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( -#if FEATURE_HANDLEREF - HandleRef hModule, -#else - IntPtr hModule, -#endif - [Out] char[] 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); From 54878ac32ffdb7d53b1c8551c73631fcdadcf701 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Wed, 2 Feb 2022 09:44:59 +1000 Subject: [PATCH 15/21] Fix possible StackOverflow in GetShortPathName/GetLongPathName by moving back to char array instead of stackalloc Remove Explicit zero initialization for RmStartSession --- src/Framework/NativeMethods.cs | 68 ++++++++++++++++------------------ src/Tasks/LockCheck.cs | 7 ---- 2 files changed, 31 insertions(+), 44 deletions(-) diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index 0cb0a4a99bd..d98baa9c80f 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -953,28 +953,25 @@ internal static string GetShortFilePath(string path) if (path != null) { - unsafe + int length = GetShortPathName(path, null, 0); + int errorCode = Marshal.GetLastWin32Error(); + + if (length > 0) { - int length = GetShortPathName(path, null, 0); - int errorCode = Marshal.GetLastWin32Error(); + char[] fullPathBuffer = new char[length]; + length = GetShortPathName(path, fullPathBuffer, length); + errorCode = Marshal.GetLastWin32Error(); if (length > 0) { - char* fullPathBuffer = stackalloc char[length]; - length = GetShortPathName(path, fullPathBuffer, length); - errorCode = Marshal.GetLastWin32Error(); - - if (length > 0) - { - string fullPath = new(fullPathBuffer, 0, length); - path = fullPath; - } + string fullPath = new(fullPathBuffer, 0, length); + path = fullPath; } + } - if (length == 0 && errorCode != 0) - { - ThrowExceptionForErrorCode(errorCode); - } + if (length == 0 && errorCode != 0) + { + ThrowExceptionForErrorCode(errorCode); } } @@ -995,28 +992,25 @@ internal static string GetLongFilePath(string path) if (path != null) { - unsafe + int length = GetLongPathName(path, null, 0); + int errorCode = Marshal.GetLastWin32Error(); + + if (length > 0) { - int length = GetLongPathName(path, null, 0); - int errorCode = Marshal.GetLastWin32Error(); + char[] fullPathBuffer = new char[length]; + length = GetLongPathName(path, fullPathBuffer, length); + errorCode = Marshal.GetLastWin32Error(); if (length > 0) { - char* fullPathBuffer = stackalloc char[length]; - length = GetLongPathName(path, fullPathBuffer, length); - errorCode = Marshal.GetLastWin32Error(); - - if (length > 0) - { - string fullPath = new(fullPathBuffer, 0, length); - path = fullPath; - } + string fullPath = new(fullPathBuffer, 0, length); + path = fullPath; } + } - if (length == 0 && errorCode != 0) - { - ThrowExceptionForErrorCode(errorCode); - } + if (length == 0 && errorCode != 0) + { + ThrowExceptionForErrorCode(errorCode); } } @@ -1098,7 +1092,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. @@ -1499,7 +1493,7 @@ internal static void VerifyThrowWin32Result(int result) /// 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] char[] 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); @@ -1548,10 +1542,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 unsafe int GetShortPathName(string path, [Out] char* fullpath, [In] int length); + internal static extern unsafe int GetShortPathName(string path, [Out] char[] fullpath, [In] int length); [DllImport("kernel32.dll", CharSet = CharSet.Unicode, BestFitMapping = false)] - internal static extern unsafe int GetLongPathName([In] string path, [Out] char* fullpath, [In] int length); + internal static extern unsafe 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); @@ -1643,7 +1637,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/LockCheck.cs b/src/Tasks/LockCheck.cs index 5347d235baf..964a433c4e5 100644 --- a/src/Tasks/LockCheck.cs +++ b/src/Tasks/LockCheck.cs @@ -218,13 +218,6 @@ internal static IEnumerable GetLockingProcessInfos(params string[] { // See http://blogs.msdn.com/b/oldnewthing/archive/2012/02/17/10268840.aspx. char* key = stackalloc char[CCH_RM_SESSION_KEY + 1]; - - // Explicitly zero initialize the buffer. - for(int i = 0; i < (CCH_RM_SESSION_KEY + 1); i++) - { - key[i] = '\0'; - } - res = RmStartSession(out handle, 0, key); } From 8af24e5df8ac71eae272ac4b84d65683d6cb3986 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Wed, 2 Feb 2022 09:48:14 +1000 Subject: [PATCH 16/21] remove unneeded unsafe from pinvoke definitions --- src/Framework/NativeMethods.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index 28cd0583ee2..bf6714d12ee 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -1533,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 unsafe int GetShortPathName(string path, [Out] char[] 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 unsafe int GetLongPathName([In] string path, [Out] char[] 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); From 031af514995598c6f7032020f860ea875c863ee7 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Thu, 3 Feb 2022 07:53:10 +1000 Subject: [PATCH 17/21] Added cached GAC Path and changes from review --- .../AssemblyDependency/AssemblyInformation.cs | 23 +++++++++---------- .../AssemblyDependency/GlobalAssemblyCache.cs | 10 ++++++++ src/Tasks/ComReference.cs | 5 ++-- src/Tasks/NativeMethods.cs | 4 +--- src/Tasks/ResolveComReference.cs | 2 +- 5 files changed, 25 insertions(+), 19 deletions(-) diff --git a/src/Tasks/AssemblyDependency/AssemblyInformation.cs b/src/Tasks/AssemblyDependency/AssemblyInformation.cs index 096dff8e870..5e5f39a1656 100644 --- a/src/Tasks/AssemblyDependency/AssemblyInformation.cs +++ b/src/Tasks/AssemblyDependency/AssemblyInformation.cs @@ -580,24 +580,24 @@ internal static string GetRuntimeVersion(string path) unsafe { // Allocate an initial buffer - char* runtimeVersionInitial = stackalloc char[bufferLength]; + 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, runtimeVersionInitial, bufferLength, out int dwLength); + 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* runtimeVersion = stackalloc char[dwLength]; + 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; } - return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersionInitial, 0, dwLength - 1) : string.Empty; + return hresult == NativeMethodsShared.S_OK ? new string(runtimeVersion, 0, dwLength - 1) : string.Empty; } } else @@ -607,14 +607,13 @@ internal static string GetRuntimeVersion(string path) #else return ManagedRuntimeVersionReader.GetRuntimeVersion(path); #endif - } - + } - /// - /// Import assembly dependencies. - /// - /// The array of assembly dependencies. - private AssemblyNameExtension[] ImportAssemblyDependencies() + /// + /// Import assembly dependencies. + /// + /// The array of assembly dependencies. + private AssemblyNameExtension[] ImportAssemblyDependencies() { #if !FEATURE_ASSEMBLYLOADCONTEXT var asmRefs = new List(); diff --git a/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs b/src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs index 758b928ed37..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. /// diff --git a/src/Tasks/ComReference.cs b/src/Tasks/ComReference.cs index e480cff968e..383c8614b86 100644 --- a/src/Tasks/ComReference.cs +++ b/src/Tasks/ComReference.cs @@ -408,7 +408,6 @@ private static string GetModuleFileName(IntPtr handle) { bool success = false; char[] buffer = null; - string output = string.Empty; // Try increased buffer sizes if on longpath-enabled Windows for (int bufferSize = NativeMethodsShared.MAX_PATH; !success && bufferSize <= NativeMethodsShared.MaxPath; bufferSize *= 2) @@ -423,7 +422,7 @@ private static string GetModuleFileName(IntPtr handle) success = pathLength != 0 && !isBufferTooSmall; if (success) { - output = new string(buffer, 0, pathLength); + return new string(buffer, 0, pathLength); } } finally @@ -432,7 +431,7 @@ private static string GetModuleFileName(IntPtr handle) } } - return output; + return string.Empty; } /// diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index a0d3403ca0b..54b21a7a904 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -1053,10 +1053,8 @@ internal static extern int CreateAssemblyNameObject( /// /// 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. 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(); From b4f6bf7c4a665af4e0c411b560d14a7383d78fef Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 4 Feb 2022 07:39:31 +1000 Subject: [PATCH 18/21] Add documentation to Pinvoke --- src/Tasks/LockCheck.cs | 45 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/Tasks/LockCheck.cs b/src/Tasks/LockCheck.cs index 1b85b545c1a..9ef5cdd62d8 100644 --- a/src/Tasks/LockCheck.cs +++ b/src/Tasks/LockCheck.cs @@ -55,12 +55,57 @@ private static extern int RmRegisterResources(uint pSessionHandle, 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 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). + /// + /// see . + /// + /// [DllImport(RestartManagerDll, CharSet = CharSet.Unicode)] 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); From b5f60db3620d70d687bd1c702b5ffc02763820bb Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 4 Feb 2022 19:28:34 +1000 Subject: [PATCH 19/21] Changes from review --- src/Tasks/ComReference.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Tasks/ComReference.cs b/src/Tasks/ComReference.cs index 383c8614b86..ce0dc9f15cd 100644 --- a/src/Tasks/ComReference.cs +++ b/src/Tasks/ComReference.cs @@ -406,11 +406,10 @@ internal static string StripTypeLibNumberFromPath(string typeLibPath, FileExists private static string GetModuleFileName(IntPtr handle) { - bool success = false; 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 = System.Buffers.ArrayPool.Shared.Rent(bufferSize); try @@ -419,8 +418,7 @@ private static string GetModuleFileName(IntPtr handle) int pathLength = NativeMethodsShared.GetModuleFileName(handleRef, buffer, bufferSize); bool isBufferTooSmall = (uint)Marshal.GetLastWin32Error() == NativeMethodsShared.ERROR_INSUFFICIENT_BUFFER; - success = pathLength != 0 && !isBufferTooSmall; - if (success) + if (pathLength != 0 && !isBufferTooSmall) { return new string(buffer, 0, pathLength); } From 53d3da4dd84e9cb888ca97fc71892b79a65de75f Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Wed, 9 Feb 2022 07:48:54 +1000 Subject: [PATCH 20/21] Change from review --- src/Tasks/ComReference.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tasks/ComReference.cs b/src/Tasks/ComReference.cs index ce0dc9f15cd..6a3f26c2c7a 100644 --- a/src/Tasks/ComReference.cs +++ b/src/Tasks/ComReference.cs @@ -409,7 +409,7 @@ private static string GetModuleFileName(IntPtr handle) char[] buffer = null; // Try increased buffer sizes if on longpath-enabled Windows - for (int bufferSize = NativeMethodsShared.MAX_PATH; bufferSize <= NativeMethodsShared.MaxPath; bufferSize *= 2) + for (int bufferSize = NativeMethodsShared.MAX_PATH; bufferSize <= NativeMethodsShared.MaxPath && bufferSize <= int.MaxValue / 2; bufferSize *= 2) { buffer = System.Buffers.ArrayPool.Shared.Rent(bufferSize); try From 0a97bfd0cad9134a2c25310a43cd119dc7eb8e9c Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Wed, 9 Feb 2022 07:54:06 +1000 Subject: [PATCH 21/21] Add VerifyThrow instead of loop condition --- src/Tasks/ComReference.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Tasks/ComReference.cs b/src/Tasks/ComReference.cs index 6a3f26c2c7a..eaf732c4ec4 100644 --- a/src/Tasks/ComReference.cs +++ b/src/Tasks/ComReference.cs @@ -409,7 +409,7 @@ private static string GetModuleFileName(IntPtr handle) char[] buffer = null; // Try increased buffer sizes if on longpath-enabled Windows - for (int bufferSize = NativeMethodsShared.MAX_PATH; bufferSize <= NativeMethodsShared.MaxPath && bufferSize <= int.MaxValue / 2; bufferSize *= 2) + for (int bufferSize = NativeMethodsShared.MAX_PATH; bufferSize <= NativeMethodsShared.MaxPath; bufferSize *= 2) { buffer = System.Buffers.ArrayPool.Shared.Rent(bufferSize); try @@ -427,6 +427,9 @@ private static string GetModuleFileName(IntPtr handle) { System.Buffers.ArrayPool.Shared.Return(buffer); } + + // Double check that the buffer is not insanely big + ErrorUtilities.VerifyThrow(bufferSize <= int.MaxValue / 2, "Buffer size approaching int.MaxValue"); } return string.Empty;