From 75737ddac80da0d1372796af532ff8ff21579a91 Mon Sep 17 00:00:00 2001 From: Sicong Liu Date: Wed, 11 Mar 2026 10:13:04 -0700 Subject: [PATCH 01/18] Add InstallExternalLibrary and UninstallExternalLibrary API to .NET extension Implement the optional InstallExternalLibrary and UninstallExternalLibrary APIs for the .NET C# language extension, enabling CREATE/DROP EXTERNAL LIBRARY support for both ZIP-packaged and raw DLL C# libraries in SQL Server. Implementation: - Native C++ entry points in nativecsharpextension.cpp with null-runtime guards - Managed C# implementation in CSharpExtension.cs using System.IO.Compression.ZipFile - ZIP magic byte detection (PK header) to distinguish ZIP vs raw DLL content - ZIP path: extract to temp dir, scan for inner .zip and extract to install dir; if no inner ZIP, copy files directly. Creates {libName}.dll alias when extracted file names differ from the SQL library name, so DllUtils can discover them. - Raw DLL path: copy file to install dir as {libName}.dll - UninstallExternalLibrary clears all files/dirs from the install directory - Unique temp folder per call (Guid-based) to prevent race conditions - Fix DllUtils.CreateDllList to search userLibName + ".*" pattern instead of exact name (pre-existing bug: exact name never matched files with extensions) Testing: - 14 new test cases in CSharpLibraryTests.cpp - Updated ExecuteInvalidLibraryNameScriptTest for DllUtils pattern change - 9 test packages in test/test_packages/ - All 73 tests pass (59 existing + 14 new) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../include/nativecsharpextension.h | 27 + .../src/managed/CSharpExtension.cs | 263 +++++++++ .../src/managed/utils/DllUtils.cs | 5 +- .../src/native/nativecsharpextension.cpp | 117 ++++ .../test/include/CSharpExtensionApiTests.h | 28 + .../test/src/native/CSharpExecuteTests.cpp | 4 +- .../src/native/CSharpExtensionApiTests.cpp | 10 + .../test/src/native/CSharpLibraryTests.cpp | 534 ++++++++++++++++++ .../test/test_packages/bad-package-ZIP.zip | 1 + .../test/test_packages/testpackageA-ZIP.zip | Bin 0 -> 290 bytes .../test/test_packages/testpackageB-DLL.zip | Bin 0 -> 305 bytes .../test/test_packages/testpackageC-EMPTY.zip | Bin 0 -> 22 bytes .../test_packages/testpackageD-NESTED.zip | Bin 0 -> 412 bytes .../test_packages/testpackageE-RAWDLL.dll | Bin 0 -> 4 bytes .../test_packages/testpackageF-SPACES.zip | Bin 0 -> 250 bytes .../test_packages/testpackageG-MANYFILES.zip | Bin 0 -> 5945 bytes .../test_packages/testpackageH-ZIPSLIP.zip | Bin 0 -> 279 bytes 17 files changed, 985 insertions(+), 4 deletions(-) create mode 100644 language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/bad-package-ZIP.zip create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageA-ZIP.zip create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageB-DLL.zip create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageC-EMPTY.zip create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageD-NESTED.zip create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageE-RAWDLL.dll create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageF-SPACES.zip create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageG-MANYFILES.zip create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageH-ZIPSLIP.zip diff --git a/language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h b/language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h index 8a1a3412..fd2c46c5 100644 --- a/language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h +++ b/language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h @@ -142,6 +142,33 @@ SQLEXTENSION_INTERFACE SQLRETURN CleanupSession(SQLGUID sessionId, SQLUSMALLINT // SQLEXTENSION_INTERFACE SQLRETURN Cleanup(); +// Installs an external library to the specified directory. +// The library file is expected to be a zip. If it contains an inner zip, +// that zip is extracted to the install directory. Otherwise, all files +// are copied directly. +// +SQLEXTENSION_INTERFACE SQLRETURN InstallExternalLibrary( + SQLGUID setupSessionId, + SQLCHAR *libraryName, + SQLINTEGER libraryNameLength, + SQLCHAR *libraryFile, + SQLINTEGER libraryFileLength, + SQLCHAR *libraryInstallDirectory, + SQLINTEGER libraryInstallDirectoryLength, + SQLCHAR **libraryError, + SQLINTEGER *libraryErrorLength); + +// Uninstalls an external library from the specified directory. +// +SQLEXTENSION_INTERFACE SQLRETURN UninstallExternalLibrary( + SQLGUID setupSessionId, + SQLCHAR *libraryName, + SQLINTEGER libraryNameLength, + SQLCHAR *libraryInstallDirectory, + SQLINTEGER libraryInstallDirectoryLength, + SQLCHAR **libraryError, + SQLINTEGER *libraryErrorLength); + // Dotnet environment pointer // static DotnetEnvironment* g_dotnet_runtime = nullptr; \ No newline at end of file diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 0f3be1f3..ff3ac700 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -9,6 +9,8 @@ // //********************************************************************* using System; +using System.IO; +using System.IO.Compression; using System.Reflection; using System.Runtime.CompilerServices; using System.Collections.Generic; @@ -637,5 +639,266 @@ public static short CleanupSession( _currentSession = null; }); } + + /// + /// This delegate declares the delegate type of InstallExternalLibrary. + /// + public delegate short InstallExternalLibraryDelegate( + Guid setupSessionId, + char *libraryName, + int libraryNameLength, + char *libraryFile, + int libraryFileLength, + char *libraryInstallDirectory, + int libraryInstallDirectoryLength, + char **libraryError, + int *libraryErrorLength); + + /// + /// This method implements InstallExternalLibrary API. + /// Installs an external library to the specified directory. + /// The library file is expected to be a zip. If it contains an inner zip, + /// that zip is extracted to the install directory. Otherwise, all files + /// are copied directly. + /// + /// + /// SQL_SUCCESS(0), SQL_ERROR(-1) + /// + public static short InstallExternalLibrary( + Guid setupSessionId, + char *libraryName, + int libraryNameLength, + char *libraryFile, + int libraryFileLength, + char *libraryInstallDirectory, + int libraryInstallDirectoryLength, + char **libraryError, + int *libraryErrorLength) + { + Logging.Trace("CSharpExtension::InstallExternalLibrary"); + + short result = SQL_SUCCESS; + string tempFolder = null; + + try + { + string libFilePath = Interop.UTF8PtrToStr(libraryFile, (ulong)libraryFileLength); + string installDir = Interop.UTF8PtrToStr(libraryInstallDirectory, (ulong)libraryInstallDirectoryLength); + string libName = Interop.UTF8PtrToStr(libraryName, (ulong)libraryNameLength); + + // Check if the file is actually a ZIP by reading magic bytes (PK prefix). + // All ZIP format variants start with 0x50 0x4B ('PK'). + // If not a ZIP (e.g. a raw DLL), there is nothing to extract — the file + // is already stored by SQL Server where DllUtils will find it, so we + // return success without any action. + bool isZipFile = false; + using (var fs = new FileStream(libFilePath, FileMode.Open, FileAccess.Read)) + { + byte[] magic = new byte[2]; + isZipFile = fs.Read(magic, 0, 2) == 2 && + magic[0] == 0x50 && magic[1] == 0x4B; + } + + if (isZipFile) + { + tempFolder = Path.Combine(installDir, Guid.NewGuid().ToString()); + + // Extract the outer zip to a temp directory. + if (Directory.Exists(tempFolder)) + { + Directory.Delete(tempFolder, true); + } + + ZipFile.ExtractToDirectory(libFilePath, tempFolder); + + // Verify the archive contained at least one entry + if (Directory.GetFiles(tempFolder).Length == 0 && + Directory.GetDirectories(tempFolder).Length == 0) + { + throw new InvalidOperationException( + "The library archive contains no entries. Nothing to install."); + } + + // Look for an inner zip file + string innerZipPath = null; + foreach (string file in Directory.GetFiles(tempFolder)) + { + if (Path.GetExtension(file).Equals(".zip", StringComparison.OrdinalIgnoreCase)) + { + innerZipPath = file; + break; + } + } + + if (!Directory.Exists(installDir)) + { + Directory.CreateDirectory(installDir); + } + + if (innerZipPath != null) + { + // Extract the inner zip to the install directory + ZipFile.ExtractToDirectory(innerZipPath, installDir, true); + } + else + { + // Copy all extracted files directly to the install directory + foreach (string file in Directory.GetFiles(tempFolder)) + { + string destFile = Path.Combine(installDir, Path.GetFileName(file)); + File.Copy(file, destFile, true); + } + + // Copy subdirectories + foreach (string dir in Directory.GetDirectories(tempFolder)) + { + string destDir = Path.Combine(installDir, Path.GetFileName(dir)); + CopyDirectory(dir, destDir); + } + } + + // Ensure at least one file matches the library name pattern so that + // DllUtils.CreateDllList (which searches for "{libName}.*") can find it. + // When the ZIP contains files with different names (e.g. "RegexSample.dll" + // for a library named "regex"), copy the first DLL as "{libName}.dll". + if (Directory.GetFiles(installDir, libName + ".*").Length == 0) + { + string[] dlls = Directory.GetFiles(installDir, "*.dll"); + if (dlls.Length > 0) + { + string destFile = Path.Combine(installDir, libName + ".dll"); + File.Copy(dlls[0], destFile, true); + } + } + } + else + { + // Not a ZIP (e.g. a raw DLL) — copy file to the install directory + // using the library name so DllUtils can discover it. + if (!Directory.Exists(installDir)) + { + Directory.CreateDirectory(installDir); + } + + string destFile = Path.Combine(installDir, libName + ".dll"); + File.Copy(libFilePath, destFile, true); + } + } + catch (Exception e) + { + var stackTracePart = string.IsNullOrEmpty(e.StackTrace) ? string.Empty : e.StackTrace + Environment.NewLine; + Logging.Error(stackTracePart + "Error: " + e.Message); + SetLibraryError(e.Message, libraryError, libraryErrorLength); + result = SQL_ERROR; + } + finally + { + // Clean up the temp folder + if (tempFolder != null && Directory.Exists(tempFolder)) + { + try { Directory.Delete(tempFolder, true); } + catch { /* Best effort cleanup */ } + } + } + + return result; + } + + /// + /// This delegate declares the delegate type of UninstallExternalLibrary. + /// + public delegate short UninstallExternalLibraryDelegate( + Guid setupSessionId, + char *libraryName, + int libraryNameLength, + char *libraryInstallDirectory, + int libraryInstallDirectoryLength, + char **libraryError, + int *libraryErrorLength); + + /// + /// This method implements UninstallExternalLibrary API. + /// Uninstalls an external library from the specified directory. + /// + /// + /// SQL_SUCCESS(0), SQL_ERROR(-1) + /// + public static short UninstallExternalLibrary( + Guid setupSessionId, + char *libraryName, + int libraryNameLength, + char *libraryInstallDirectory, + int libraryInstallDirectoryLength, + char **libraryError, + int *libraryErrorLength) + { + Logging.Trace("CSharpExtension::UninstallExternalLibrary"); + + short result = SQL_SUCCESS; + + try + { + string installDir = Interop.UTF8PtrToStr(libraryInstallDirectory, (ulong)libraryInstallDirectoryLength); + + if (Directory.Exists(installDir)) + { + // Delete all contents within the install directory + foreach (string file in Directory.GetFiles(installDir)) + { + File.Delete(file); + } + + foreach (string dir in Directory.GetDirectories(installDir)) + { + Directory.Delete(dir, true); + } + } + } + catch (Exception e) + { + string stackTracePart = string.IsNullOrEmpty(e.StackTrace) ? string.Empty : e.StackTrace + Environment.NewLine; + Logging.Error(stackTracePart + "Error: " + e.Message); + SetLibraryError(e.Message, libraryError, libraryErrorLength); + result = SQL_ERROR; + } + + return result; + } + + /// + /// Allocates an unmanaged error string and populates the error output parameters. + /// + private static void SetLibraryError(string errorMessage, char **libraryError, int *libraryErrorLength) + { + if (libraryError != null && libraryErrorLength != null) + { + byte[] errorBytes = System.Text.Encoding.UTF8.GetBytes(errorMessage); + IntPtr errorPtr = Marshal.AllocHGlobal(errorBytes.Length + 1); + Marshal.Copy(errorBytes, 0, errorPtr, errorBytes.Length); + ((byte*)errorPtr)[errorBytes.Length] = 0; + *libraryError = (char*)errorPtr; + *libraryErrorLength = errorBytes.Length; + } + } + + /// + /// Recursively copies a directory and its contents. + /// + private static void CopyDirectory(string sourceDir, string destDir) + { + Directory.CreateDirectory(destDir); + + foreach (string file in Directory.GetFiles(sourceDir)) + { + string destFile = Path.Combine(destDir, Path.GetFileName(file)); + File.Copy(file, destFile, true); + } + + foreach (string dir in Directory.GetDirectories(sourceDir)) + { + string destSubDir = Path.Combine(destDir, Path.GetFileName(dir)); + CopyDirectory(dir, destSubDir); + } + } } } diff --git a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs index 9d6f9165..67bddb38 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs @@ -91,14 +91,15 @@ public static List CreateDllList( } else { + // Search for files matching the library name pattern (e.g. "regex.*") if (!string.IsNullOrEmpty(privatePath)) { - dllList.AddRange(Directory.GetFiles(privatePath, userLibName)); + dllList.AddRange(Directory.GetFiles(privatePath, userLibName + ".*")); } if (!string.IsNullOrEmpty(publicPath)) { - dllList.AddRange(Directory.GetFiles(publicPath, userLibName)); + dllList.AddRange(Directory.GetFiles(publicPath, userLibName + ".*")); } } diff --git a/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp b/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp index b70831d7..2c86af43 100644 --- a/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp +++ b/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp @@ -340,4 +340,121 @@ SQLRETURN Cleanup() LOG("nativecsharpextension::Cleanup"); delete g_dotnet_runtime; return SQL_SUCCESS; +} + +//-------------------------------------------------------------------------------------------------- +// Name: SetLibraryError +// +// Description: +// Helper to populate the library error output parameters. +// +static void SetLibraryError( + const std::string &errorString, + SQLCHAR **libraryError, + SQLINTEGER *libraryErrorLength) +{ + if (!errorString.empty()) + { + *libraryErrorLength = static_cast(errorString.length()); + std::string *pError = new std::string(errorString); + *libraryError = const_cast( + reinterpret_cast(pError->c_str())); + } +} + +//-------------------------------------------------------------------------------------------------- +// Name: InstallExternalLibrary +// +// Description: +// Installs an external library to the specified directory. +// The library file is expected to be a zip containing the library files. +// If it contains an inner zip, that zip is extracted to the install directory. +// Otherwise, all files are copied directly. +// +// Returns: +// SQL_SUCCESS on success, else SQL_ERROR +// +SQLRETURN InstallExternalLibrary( + SQLGUID setupSessionId, + SQLCHAR *libraryName, + SQLINTEGER libraryNameLength, + SQLCHAR *libraryFile, + SQLINTEGER libraryFileLength, + SQLCHAR *libraryInstallDirectory, + SQLINTEGER libraryInstallDirectoryLength, + SQLCHAR **libraryError, + SQLINTEGER *libraryErrorLength) +{ + LOG("nativecsharpextension::InstallExternalLibrary"); + + SQLRETURN result = SQL_ERROR; + + if (g_dotnet_runtime == nullptr) + { + SetLibraryError( + "Extension not initialized. Call Init before InstallExternalLibrary.", + libraryError, + libraryErrorLength); + } + else + { + result = g_dotnet_runtime->call_managed_method( + nameof(InstallExternalLibrary), + setupSessionId, + libraryName, + libraryNameLength, + libraryFile, + libraryFileLength, + libraryInstallDirectory, + libraryInstallDirectoryLength, + libraryError, + libraryErrorLength); + } + + return result; +} + +//-------------------------------------------------------------------------------------------------- +// Name: UninstallExternalLibrary +// +// Description: +// Uninstalls an external library from the specified directory. +// +// Returns: +// SQL_SUCCESS on success, else SQL_ERROR +// +SQLRETURN UninstallExternalLibrary( + SQLGUID setupSessionId, + SQLCHAR *libraryName, + SQLINTEGER libraryNameLength, + SQLCHAR *libraryInstallDirectory, + SQLINTEGER libraryInstallDirectoryLength, + SQLCHAR **libraryError, + SQLINTEGER *libraryErrorLength) +{ + LOG("nativecsharpextension::UninstallExternalLibrary"); + + SQLRETURN result = SQL_ERROR; + + if (g_dotnet_runtime == nullptr) + { + SetLibraryError( + "Extension not initialized. Call Init before UninstallExternalLibrary.", + libraryError, + libraryErrorLength); + } + else + { + result = g_dotnet_runtime->call_managed_method( + nameof(UninstallExternalLibrary), + setupSessionId, + libraryName, + libraryNameLength, + libraryInstallDirectory, + libraryInstallDirectoryLength, + libraryError, + libraryErrorLength); + } + + return result; } \ No newline at end of file diff --git a/language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h b/language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h index 34c01625..998c140b 100644 --- a/language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h +++ b/language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h @@ -99,6 +99,26 @@ typedef SQLRETURN FN_cleanupSession( typedef SQLRETURN FN_cleanup(); +typedef SQLRETURN FN_installExternalLibrary( + SQLGUID, // setupSessionId + SQLCHAR *, // libraryName + SQLINTEGER, // libraryNameLength + SQLCHAR *, // libraryFile + SQLINTEGER, // libraryFileLength + SQLCHAR *, // libraryInstallDirectory + SQLINTEGER, // libraryInstallDirectoryLength + SQLCHAR **, // libraryError + SQLINTEGER *);// libraryErrorLength + +typedef SQLRETURN FN_uninstallExternalLibrary( + SQLGUID, // setupSessionId + SQLCHAR *, // libraryName + SQLINTEGER, // libraryNameLength + SQLCHAR *, // libraryInstallDirectory + SQLINTEGER, // libraryInstallDirectoryLength + SQLCHAR **, // libraryError + SQLINTEGER *);// libraryErrorLength + namespace ExtensionApiTest { // Forward declaration @@ -447,6 +467,14 @@ namespace ExtensionApiTest // Pointer to the Cleanup function // static FN_cleanup *sm_cleanupFuncPtr; + + // Pointer to the InstallExternalLibrary function + // + static FN_installExternalLibrary *sm_installExternalLibraryFuncPtr; + + // Pointer to the UninstallExternalLibrary function + // + static FN_uninstallExternalLibrary *sm_uninstallExternalLibraryFuncPtr; }; // ColumnInfo template class to store information diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp index 8a89fc7c..84cb9249 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp @@ -103,9 +103,9 @@ namespace ExtensionApiTest // TEST_F(CSharpExtensionApiTests, ExecuteInvalidLibraryNameScriptTest) { - // Unmatched library name with the dll file name. + // Use a library name that doesn't match any DLL file in the library path. // - string userLibName = "Microsoft.SqlServer.CSharpExtensionTest"; + string userLibName = "NonExistentLibrary"; string scriptString = userLibName + m_Separator + m_UserClassFullName; InitializeSession( 0, // inputSchemaColumnsNumber diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExtensionApiTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExtensionApiTests.cpp index fed15afb..08a8d635 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExtensionApiTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExtensionApiTests.cpp @@ -31,6 +31,8 @@ namespace ExtensionApiTest FN_getOutputParam *CSharpExtensionApiTests::sm_getOutputParamFuncPtr = nullptr; FN_cleanupSession *CSharpExtensionApiTests::sm_cleanupSessionFuncPtr = nullptr; FN_cleanup *CSharpExtensionApiTests::sm_cleanupFuncPtr = nullptr; + FN_installExternalLibrary *CSharpExtensionApiTests::sm_installExternalLibraryFuncPtr = nullptr; + FN_uninstallExternalLibrary *CSharpExtensionApiTests::sm_uninstallExternalLibraryFuncPtr = nullptr; //---------------------------------------------------------------------------------------------- // Name: CSharpExtensionApiTest::SetUpTestCase @@ -250,6 +252,14 @@ namespace ExtensionApiTest sm_cleanupFuncPtr = reinterpret_cast(GetProcAddress(sm_libHandle, "Cleanup")); EXPECT_TRUE(sm_cleanupFuncPtr != nullptr); + + sm_installExternalLibraryFuncPtr = reinterpret_cast( + GetProcAddress(sm_libHandle, "InstallExternalLibrary")); + EXPECT_TRUE(sm_installExternalLibraryFuncPtr != nullptr); + + sm_uninstallExternalLibraryFuncPtr = reinterpret_cast( + GetProcAddress(sm_libHandle, "UninstallExternalLibrary")); + EXPECT_TRUE(sm_uninstallExternalLibraryFuncPtr != nullptr); } //---------------------------------------------------------------------------------------------- diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp new file mode 100644 index 00000000..cf1a9d59 --- /dev/null +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -0,0 +1,534 @@ +//********************************************************************* +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +// +// @File: CSharpLibraryTests.cpp +// +// Purpose: +// Tests the .NET Core CSharpExtension's implementation of the +// InstallExternalLibrary and UninstallExternalLibrary APIs. +// Uses the CSharpExtensionApiTests fixture to share the single +// .NET runtime initialization. +// +//********************************************************************* +#include "CSharpExtensionApiTests.h" + +using namespace std; +namespace fs = experimental::filesystem; + +namespace ExtensionApiTest +{ + // Helper: get path to test_packages directory + // + static string GetPackagesPath() + { + string result; + const char *enlRoot = getenv("ENL_ROOT"); + + if (enlRoot != nullptr) + { + result = (fs::path(enlRoot) / + "language-extensions" / "dotnet-core-CSharp" / "test" / "test_packages").string(); + } + else + { + // Fallback: navigate from executable path + char path[MAX_PATH + 1] = { 0 }; + GetModuleFileName(NULL, path, MAX_PATH); + fs::path buildOutputPath = fs::path(path).parent_path().parent_path().parent_path().parent_path(); + result = (buildOutputPath.parent_path().parent_path() / + "language-extensions" / "dotnet-core-CSharp" / "test" / "test_packages").string(); + } + + return result; + } + + // Helper: create a clean temporary install directory + // + static string CreateInstallDir() + { + char path[MAX_PATH + 1] = { 0 }; + GetModuleFileName(NULL, path, MAX_PATH); + string installDir = (fs::path(path).parent_path() / "testInstallLibs").string(); + + if (fs::exists(installDir)) + { + fs::remove_all(installDir); + } + fs::create_directories(installDir); + + return installDir; + } + + // Helper: clean up install directory + // + static void CleanupInstallDir(const string &installDir) + { + if (fs::exists(installDir)) + { + fs::remove_all(installDir); + } + } + + // Helper: call InstallExternalLibrary and check result + // + static SQLRETURN CallInstall( + FN_installExternalLibrary *installFunc, + const string &libName, + const string &libFilePath, + const string &installDir) + { + SQLCHAR *libError = nullptr; + SQLINTEGER libErrorLength = 0; + + SQLRETURN result = (*installFunc)( + SQLGUID(), + reinterpret_cast(const_cast(libName.c_str())), + static_cast(libName.length()), + reinterpret_cast(const_cast(libFilePath.c_str())), + static_cast(libFilePath.length()), + reinterpret_cast(const_cast(installDir.c_str())), + static_cast(installDir.length()), + &libError, + &libErrorLength); + + return result; + } + + // Helper: call UninstallExternalLibrary and check result + // + static SQLRETURN CallUninstall( + FN_uninstallExternalLibrary *uninstallFunc, + const string &libName, + const string &installDir) + { + SQLCHAR *libError = nullptr; + SQLINTEGER libErrorLength = 0; + + SQLRETURN result = (*uninstallFunc)( + SQLGUID(), + reinterpret_cast(const_cast(libName.c_str())), + static_cast(libName.length()), + reinterpret_cast(const_cast(installDir.c_str())), + static_cast(installDir.length()), + &libError, + &libErrorLength); + + return result; + } + + // Helper: check if directory has any files or subdirectories + // + static bool DirectoryHasFiles(const string &dir) + { + bool hasFiles = false; + + for (const auto &entry : fs::directory_iterator(dir)) + { + hasFiles = true; + break; + } + + return hasFiles; + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallZipContainingZipTest + // + // Description: + // Tests installing an outer zip that contains an inner zip. + // Verifies that files from the inner zip are extracted to the install directory. + // + TEST_F(CSharpExtensionApiTests, InstallZipContainingZipTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "testpackageA", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + EXPECT_TRUE(DirectoryHasFiles(installDir)) + << "No files found in install directory after installation"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallZipContainingDllsTest + // + // Description: + // Tests installing an outer zip that contains DLLs directly (no inner zip). + // Verifies that DLLs are copied to the install directory. + // + TEST_F(CSharpExtensionApiTests, InstallZipContainingDllsTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "testpackageB", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // Verify that DLL files were copied to the install directory + bool hasDll = false; + for (const auto &entry : fs::directory_iterator(installDir)) + { + if (entry.path().extension() == ".dll") + { + hasDll = true; + break; + } + } + EXPECT_TRUE(hasDll) << "No DLL files found in install directory after installation"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallInvalidZipTest + // + // Description: + // Tests that installing a file with .zip extension that is not actually + // a valid ZIP (magic bytes are not PK) copies it to the install directory + // as a raw file named after the library and returns SQL_SUCCESS. + // + TEST_F(CSharpExtensionApiTests, InstallInvalidZipTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "bad-package-ZIP.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "bad-package", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + EXPECT_TRUE(fs::exists(fs::path(installDir) / "bad-package.dll")) + << "Raw file not found in install directory as bad-package.dll"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallNonExistentFileTest + // + // Description: + // Tests that installing a non-existent file returns SQL_ERROR. + // + TEST_F(CSharpExtensionApiTests, InstallNonExistentFileTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "nonexistent.zip").string(); + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "nonexistent", packagePath, installDir); + EXPECT_EQ(result, SQL_ERROR); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: UninstallLibraryTest + // + // Description: + // Tests installing a library, then uninstalling it, and verifying the + // install directory is empty. + // + TEST_F(CSharpExtensionApiTests, UninstallLibraryTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + // Install first + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "testpackageB", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + EXPECT_TRUE(DirectoryHasFiles(installDir)) << "No files found after installation"; + + // Uninstall + result = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "testpackageB", installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // Verify directory is empty + EXPECT_FALSE(DirectoryHasFiles(installDir)) << "Files still present after uninstall"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: ReinstallLibraryTest + // + // Description: + // Tests installing a library, then reinstalling with a different package + // to verify overwrite behavior works correctly. + // + TEST_F(CSharpExtensionApiTests, ReinstallLibraryTest) + { + string packagesPath = GetPackagesPath(); + string packagePathA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + string packagePathB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_TRUE(fs::exists(packagePathA)) << "Test package not found: " << packagePathA; + ASSERT_TRUE(fs::exists(packagePathB)) << "Test package not found: " << packagePathB; + + string installDir = CreateInstallDir(); + + // Install first package + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "testpackage", packagePathA, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // Install second package (overwrite) + result = CallInstall(sm_installExternalLibraryFuncPtr, + "testpackage", packagePathB, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // Verify the install directory has files from the second package + bool hasDll = false; + for (const auto &entry : fs::directory_iterator(installDir)) + { + if (entry.path().extension() == ".dll") + { + hasDll = true; + break; + } + } + EXPECT_TRUE(hasDll) << "DLL from second package not found after reinstall"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallEmptyZipTest + // + // Description: + // Tests that installing an empty zip (zero entries) returns SQL_ERROR. + // The install logic detects that the archive contains no entries and + // explicitly fails the operation rather than proceeding with an empty + // set of files. + // + TEST_F(CSharpExtensionApiTests, InstallEmptyZipTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageC-EMPTY.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "emptypackage", packagePath, installDir); + EXPECT_EQ(result, SQL_ERROR); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallZipWithNestedDirectoriesTest + // + // Description: + // Tests that installing a zip with nested subdirectories (e.g., + // lib/net8.0/, runtimes/win-x64/) preserves the full directory tree. + // + TEST_F(CSharpExtensionApiTests, InstallZipWithNestedDirectoriesTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageD-NESTED.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "nestedpackage", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // Verify subdirectories were preserved + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib" / "net8.0" / "MyLib.dll")) + << "Nested file lib/net8.0/MyLib.dll not found"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "runtimes" / "win-x64" / "native.dll")) + << "Nested file runtimes/win-x64/native.dll not found"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "MyLib.deps.json")) + << "Root file MyLib.deps.json not found"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallRawDllNotZipTest + // + // Description: + // Tests that installing a raw DLL file (not a zip) copies it to the + // install directory named after the library and returns SQL_SUCCESS. + // + TEST_F(CSharpExtensionApiTests, InstallRawDllNotZipTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "rawdllpackage", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // The raw DLL should be copied using the library name with .dll extension + EXPECT_TRUE(fs::exists(fs::path(installDir) / "rawdllpackage.dll")) + << "Raw DLL not found in install directory as rawdllpackage.dll"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallZipWithSpacesInFilenamesTest + // + // Description: + // Tests that installing a zip whose entries contain spaces in filenames + // extracts correctly. + // + TEST_F(CSharpExtensionApiTests, InstallZipWithSpacesInFilenamesTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageF-SPACES.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "spacespackage", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + EXPECT_TRUE(fs::exists(fs::path(installDir) / "My Library.dll")) + << "File with spaces 'My Library.dll' not found"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "config file.json")) + << "File with spaces 'config file.json' not found"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallZipWithManyFilesTest + // + // Description: + // Tests that installing a zip containing many files (50 DLLs) extracts + // all of them correctly. + // + TEST_F(CSharpExtensionApiTests, InstallZipWithManyFilesTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageG-MANYFILES.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "manyfilespackage", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // Count extracted DLL files + int dllCount = 0; + for (const auto &entry : fs::directory_iterator(installDir)) + { + if (entry.path().extension() == ".dll") + { + dllCount++; + } + } + EXPECT_EQ(dllCount, 51) << "Expected 50 extracted DLL files + 1 alias for library name, found " << dllCount; + + // Verify the alias DLL exists so DllUtils can discover the library by name + EXPECT_TRUE(fs::exists(fs::path(installDir) / "manyfilespackage.dll")); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallZipSlipTest + // + // Description: + // Tests that installing a zip containing path-traversal entries (e.g., "../../malicious.dll") + // returns SQL_ERROR. .NET 8's ZipFile.ExtractToDirectory has built-in protection against + // zip-slip attacks and throws IOException for such entries. + // + TEST_F(CSharpExtensionApiTests, InstallZipSlipTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageH-ZIPSLIP.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "zipslippackage", packagePath, installDir); + EXPECT_EQ(result, SQL_ERROR); + + // Verify no files were written outside the install directory + fs::path parentDir = fs::path(installDir).parent_path(); + EXPECT_FALSE(fs::exists(parentDir / "malicious.dll")) + << "Zip-slip attack: file escaped install directory"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: DoubleUninstallTest + // + // Description: + // Tests that uninstalling a library twice is idempotent. The second + // uninstall should succeed even though the library is already gone. + // + TEST_F(CSharpExtensionApiTests, DoubleUninstallTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + // Install, then uninstall twice + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "doubleuninstall", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + result = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "doubleuninstall", installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + result = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "doubleuninstall", installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: UninstallNonExistentLibraryTest + // + // Description: + // Tests that uninstalling a library that was never installed succeeds. + // The install directory may or may not exist; either way the operation + // should not fail. + // + TEST_F(CSharpExtensionApiTests, UninstallNonExistentLibraryTest) + { + string installDir = CreateInstallDir(); + + SQLRETURN result = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "neverinstalled", installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + CleanupInstallDir(installDir); + } +} diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/bad-package-ZIP.zip b/language-extensions/dotnet-core-CSharp/test/test_packages/bad-package-ZIP.zip new file mode 100644 index 00000000..cf898e03 --- /dev/null +++ b/language-extensions/dotnet-core-CSharp/test/test_packages/bad-package-ZIP.zip @@ -0,0 +1 @@ +This is not a valid zip file diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageA-ZIP.zip b/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageA-ZIP.zip new file mode 100644 index 0000000000000000000000000000000000000000..e6e2b1cbdd5488bca276a2ec6d0cbca31dccd497 GIT binary patch literal 290 zcmWIWW@Zs#U|`^2sHw|}S-9s@?*brCn~{M*07#dl7MBzxCTAz6r#k9YWfpLMNJ|q; zOh|Z;uwv!uu7|$+MVFj9aA3ii0|yii95@g#DJmvt!i+g{XUu!GV)~@Xhil*Uyo>ag z^}m0hsb8Mi_T1d--)vUPz9@{c_c$CJ{{D&N`{({MC71$wdhB^7iwV_VWKP39RVkKe)`msMFvfip`rEpUjY$A(15^ oA(7TJm5Cw1n~_O`0k?mEUP1!sz6_Viq8e35fZD*tgOrGf6KcwV+rpt2jSz^*UCD02B>u zcY}5d0Tl`WG02#l%q0E1)DjCl1AVC8oE&fOGk#ZnPoBK!d+xlp=UHt=G&7z{n=A$C z=L2GCATBD+E6L1FE!Hp3%+swfGttjWEXgcO)l13AIqj`;Qpfu$l0l42B4CTLyB}y8 x5iUx=#=$lmeM}{1xEM$_7%*41_0u^ga-W0RXQBUnu|p literal 0 HcmV?d00001 diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageE-RAWDLL.dll b/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageE-RAWDLL.dll new file mode 100644 index 0000000000000000000000000000000000000000..9215d5d102947c19355e4b2e77c790b8d567be62 GIT binary patch literal 4 LcmeZ`VqgFO0!9F+ literal 0 HcmV?d00001 diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageF-SPACES.zip b/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageF-SPACES.zip new file mode 100644 index 0000000000000000000000000000000000000000..a73bdebd75ba7c72e64f10aa5efdcadd432960c4 GIT binary patch literal 250 zcmWIWW@Zs#U|`^2xYnH&_Viq8e35W%NI5|HrEi+vqEi)%oFRM5|Z}mD>h5!^D z#|$}ncz`OofS3=6eJd4wGLwoDiz@X}a&olwJbe6oFZ!N5$q3ZP$Rxsm+b*DHB!F%+ ex^8rP5n7FaOjK6{c(byBqY%;g!QM;4K9mnwkl}|xfqhsn z(1(IruMpK{5t*Atj3^kaq%1R2TMJe}L9`7Pv0)5$IOX|2*LD7vazK$E_kCaYpY!~& zqIA&Uq~kb4ot}ML)@CP`>_`N^!$Gbu52a)LnM>Ug7eN z!Yrpk&~@Y9HGCY9F~KtubXKNKbsBi$s9kjgi-l9x%8%VqRRnut)T(fn{>rdY~7+=>D;qtZ}CeDC5v z1dDKbRydbB$*Ba3aJE?YoRh3uK(GjBi*?Vti(6L4!zpgcx~S^*(O<{coh`!YTbs^* zQeR832&Zpt`o+_KD+w0i^sR87ecM<jFLn|v!r5lsbJ>aqU&O;HhHcGy zTvyl0__`?(PS@J>>R+1n5G=y!TAM!A|9LaPBAl)j&fosr@EyS-oURqlv5jAnkGPbW zmun5I>Tn79h)aoZx>h(3UbyuK*|rF$YlXA(`VajCi*UMDIJ346CsigT!g<-c=S4N! zNR>&6a9+0VdGzA%3*+Gw^RmV@<&0prx+!kbgslZvicJmxr906`eDD1!$>@JY*U&%(Ird#X~kh zhKbZtS9r)5kYOOT)cqYYKRY^Sm`5#jd55fq4CAP!ZtjpLB1x}P_jbTOKpH+>+9Agd z8aSp$!>1cNWF;ist*+~k2Ow$qbXSMG14+ZDi#p`6!O=Ns_;gE$Tnb6Urz<*SBP0!< z?&pv$=bI9VHfeFz|(Y+k79*~AlmvYD!NE$xf$RYoNq~X(b9CCVY zbWD0q-Nhj{L(=f+A`W>Dl7>&WaLDJ7G<>>(LrxkJos))7_ixB`kTiU{d_x|Eq~X)e z8?rZ&bhNs6115$J9P@n(*DqULx*^LUY4~*GhTIQH!>8*uWIH4cpYGa_xx=D!(h2FJ z4Y?SShEKO_$X$>$e7a&oUW26J)BPH Date: Sat, 21 Mar 2026 11:17:40 -0700 Subject: [PATCH 02/18] InstallExternalLibrary/UninstallExternalLibrary: ZIP extraction, manifest-based cleanup, conflict detection, ALTER support - Fix build script to use VS 2022 instead of VS 2019 - Add global.json to pin .NET SDK 8.0 - Fix: use library name as-is (no unconditional .dll append) - InstallExternalLibrary: ZIP extraction with file conflict detection, manifest tracking - UninstallExternalLibrary: manifest-based targeted file deletion, bottom-up empty dir cleanup - ALTER EXTERNAL LIBRARY: clean up previous install before re-installing - CHANGES.md documenting all changes --- .../src/managed/CSharpExtension.cs | 166 ++++++++++++++++-- 1 file changed, 153 insertions(+), 13 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index ff3ac700..17f97d8d 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -686,6 +686,17 @@ public static short InstallExternalLibrary( string installDir = Interop.UTF8PtrToStr(libraryInstallDirectory, (ulong)libraryInstallDirectoryLength); string libName = Interop.UTF8PtrToStr(libraryName, (ulong)libraryNameLength); + // If a manifest from a previous install of this same library + // exists (e.g. ALTER EXTERNAL LIBRARY), clean up the old files + // before installing the new content. This prevents orphaned + // files and avoids false conflict errors when the new ZIP + // contains some of the same filenames as the old one. + string existingManifest = Path.Combine(installDir, libName + ".manifest"); + if (File.Exists(existingManifest)) + { + CleanupManifest(existingManifest, installDir); + } + // Check if the file is actually a ZIP by reading magic bytes (PK prefix). // All ZIP format variants start with 0x50 0x4B ('PK'). // If not a ZIP (e.g. a raw DLL), there is nothing to extract — the file @@ -735,21 +746,62 @@ public static short InstallExternalLibrary( Directory.CreateDirectory(installDir); } + // Check for file-level conflicts before extracting. + // Directory (folder) overlaps are allowed — multiple libraries + // may extract into the same parent folders. + var extractedFiles = new List(); + if (innerZipPath != null) { - // Extract the inner zip to the install directory - ZipFile.ExtractToDirectory(innerZipPath, installDir, true); + using (var innerArchive = ZipFile.OpenRead(innerZipPath)) + { + foreach (var entry in innerArchive.Entries) + { + if (string.IsNullOrEmpty(entry.Name)) + continue; + + string relPath = entry.FullName.Replace('/', Path.DirectorySeparatorChar); + string destFile = Path.Combine(installDir, relPath); + + if (File.Exists(destFile)) + { + throw new InvalidOperationException( + $"Cannot install library '{libName}': file '{relPath}' already exists. " + + "Another library has already installed a file with this name."); + } + + extractedFiles.Add(relPath); + } + } + + // All conflict checks passed — extract + ZipFile.ExtractToDirectory(innerZipPath, installDir, false); } else { - // Copy all extracted files directly to the install directory + // Collect files from the extracted temp directory + CollectRelativeFiles(tempFolder, "", extractedFiles); + + // Check for file conflicts before copying + foreach (string relPath in extractedFiles) + { + string destFile = Path.Combine(installDir, relPath); + if (File.Exists(destFile)) + { + throw new InvalidOperationException( + $"Cannot install library '{libName}': file '{relPath}' already exists. " + + "Another library has already installed a file with this name."); + } + } + + // All conflict checks passed — copy files foreach (string file in Directory.GetFiles(tempFolder)) { string destFile = Path.Combine(installDir, Path.GetFileName(file)); - File.Copy(file, destFile, true); + File.Copy(file, destFile, false); } - // Copy subdirectories + // Copy subdirectories (merge into existing dirs) foreach (string dir in Directory.GetDirectories(tempFolder)) { string destDir = Path.Combine(installDir, Path.GetFileName(dir)); @@ -761,15 +813,25 @@ public static short InstallExternalLibrary( // DllUtils.CreateDllList (which searches for "{libName}.*") can find it. // When the ZIP contains files with different names (e.g. "RegexSample.dll" // for a library named "regex"), copy the first DLL as "{libName}.dll". - if (Directory.GetFiles(installDir, libName + ".*").Length == 0) + if (Directory.GetFiles(installDir, libName + ".*").Length == 0 && + !File.Exists(Path.Combine(installDir, libName))) { string[] dlls = Directory.GetFiles(installDir, "*.dll"); if (dlls.Length > 0) { - string destFile = Path.Combine(installDir, libName + ".dll"); + string destFile = Path.Combine(installDir, libName); File.Copy(dlls[0], destFile, true); + extractedFiles.Add(libName); } } + + // Write a manifest listing all extracted file paths so that + // UninstallExternalLibrary can remove exactly these files. + if (extractedFiles.Count > 0) + { + string manifestPath = Path.Combine(installDir, libName + ".manifest"); + File.WriteAllLines(manifestPath, extractedFiles); + } } else { @@ -780,7 +842,7 @@ public static short InstallExternalLibrary( Directory.CreateDirectory(installDir); } - string destFile = Path.Combine(installDir, libName + ".dll"); + string destFile = Path.Combine(installDir, libName); File.Copy(libFilePath, destFile, true); } } @@ -839,18 +901,24 @@ public static short UninstallExternalLibrary( try { string installDir = Interop.UTF8PtrToStr(libraryInstallDirectory, (ulong)libraryInstallDirectoryLength); + string libName = Interop.UTF8PtrToStr(libraryName, (ulong)libraryNameLength); if (Directory.Exists(installDir)) { - // Delete all contents within the install directory - foreach (string file in Directory.GetFiles(installDir)) + // Check for a manifest written during install that lists + // all files extracted from the library's ZIP content. + string manifestPath = Path.Combine(installDir, libName + ".manifest"); + if (File.Exists(manifestPath)) { - File.Delete(file); + CleanupManifest(manifestPath, installDir); } - foreach (string dir in Directory.GetDirectories(installDir)) + // Delete the library file itself (for non-ZIP libraries + // that were copied directly during install) + string libraryFile = Path.Combine(installDir, libName); + if (File.Exists(libraryFile)) { - Directory.Delete(dir, true); + File.Delete(libraryFile); } } } @@ -900,5 +968,77 @@ private static void CopyDirectory(string sourceDir, string destDir) CopyDirectory(dir, destSubDir); } } + + /// + /// Recursively collects all file paths relative to the root directory. + /// + private static void CollectRelativeFiles(string directory, string prefix, List results) + { + foreach (string file in Directory.GetFiles(directory)) + { + string relPath = string.IsNullOrEmpty(prefix) + ? Path.GetFileName(file) + : Path.Combine(prefix, Path.GetFileName(file)); + results.Add(relPath); + } + + foreach (string dir in Directory.GetDirectories(directory)) + { + string dirName = Path.GetFileName(dir); + string newPrefix = string.IsNullOrEmpty(prefix) + ? dirName + : Path.Combine(prefix, dirName); + CollectRelativeFiles(dir, newPrefix, results); + } + } + + /// + /// Reads a manifest file, deletes all listed files, removes any + /// directories that become empty (bottom-up), then deletes the + /// manifest itself. Used by both UninstallExternalLibrary and + /// InstallExternalLibrary (to clean up a previous version during + /// ALTER EXTERNAL LIBRARY). + /// + private static void CleanupManifest(string manifestPath, string installDir) + { + string[] extractedFiles = File.ReadAllLines(manifestPath); + var parentDirs = new HashSet(StringComparer.OrdinalIgnoreCase); + + foreach (string relPath in extractedFiles) + { + if (string.IsNullOrWhiteSpace(relPath)) + continue; + + string fullPath = Path.Combine(installDir, relPath); + if (File.Exists(fullPath)) + { + File.Delete(fullPath); + } + + // Track parent directories for empty-dir cleanup + string dir = Path.GetDirectoryName(fullPath); + while (!string.IsNullOrEmpty(dir) && + !dir.Equals(installDir, StringComparison.OrdinalIgnoreCase)) + { + parentDirs.Add(dir); + dir = Path.GetDirectoryName(dir); + } + } + + // Remove empty directories bottom-up (deepest first) + var sortedDirs = new List(parentDirs); + sortedDirs.Sort((a, b) => b.Length.CompareTo(a.Length)); + foreach (string dir in sortedDirs) + { + if (Directory.Exists(dir) && + Directory.GetFiles(dir).Length == 0 && + Directory.GetDirectories(dir).Length == 0) + { + Directory.Delete(dir, false); + } + } + + File.Delete(manifestPath); + } } } From 1a782e32f2d2d14c7de55919d478b7479f8dc91a Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Fri, 17 Apr 2026 05:18:14 -0700 Subject: [PATCH 03/18] Add tests for manifest-based install/uninstall, conflict detection, ALTER New tests: - ManifestWrittenTest, ManifestListsNestedFilesTest - InstallLibNameAliasNoExtensionTest - DirectoryOverlapAllowedTest, FileConflictFailsTest - UninstallPreservesOtherLibrariesTest - UninstallRemovesEmptyNestedDirsTest - AlterExternalLibraryTest Update 3 existing tests to reflect new libName-without-extension naming: - InstallInvalidZipTest, InstallRawDllNotZipTest, InstallZipWithManyFilesTest --- .../test/src/native/CSharpLibraryTests.cpp | 365 +++++++++++++++++- 1 file changed, 357 insertions(+), 8 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index cf1a9d59..93f1f974 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -12,6 +12,8 @@ // //********************************************************************* #include "CSharpExtensionApiTests.h" +#include +#include using namespace std; namespace fs = experimental::filesystem; @@ -211,8 +213,8 @@ namespace ExtensionApiTest "bad-package", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - EXPECT_TRUE(fs::exists(fs::path(installDir) / "bad-package.dll")) - << "Raw file not found in install directory as bad-package.dll"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "bad-package")) + << "Raw file not found in install directory as bad-package"; CleanupInstallDir(installDir); } @@ -384,9 +386,9 @@ namespace ExtensionApiTest "rawdllpackage", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - // The raw DLL should be copied using the library name with .dll extension - EXPECT_TRUE(fs::exists(fs::path(installDir) / "rawdllpackage.dll")) - << "Raw DLL not found in install directory as rawdllpackage.dll"; + // The raw DLL should be copied using the library name (no extension) + EXPECT_TRUE(fs::exists(fs::path(installDir) / "rawdllpackage")) + << "Raw DLL not found in install directory as rawdllpackage"; CleanupInstallDir(installDir); } @@ -446,10 +448,10 @@ namespace ExtensionApiTest dllCount++; } } - EXPECT_EQ(dllCount, 51) << "Expected 50 extracted DLL files + 1 alias for library name, found " << dllCount; + EXPECT_EQ(dllCount, 50) << "Expected 50 extracted DLL files, found " << dllCount; - // Verify the alias DLL exists so DllUtils can discover the library by name - EXPECT_TRUE(fs::exists(fs::path(installDir) / "manyfilespackage.dll")); + // Verify the extensionless alias exists so DllUtils can discover the library by name + EXPECT_TRUE(fs::exists(fs::path(installDir) / "manyfilespackage")); CleanupInstallDir(installDir); } @@ -531,4 +533,351 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + + //---------------------------------------------------------------------------------------------- + // Helper: read manifest file into a vector of relative paths + // + static vector ReadManifest(const string &manifestPath) + { + vector lines; + ifstream f(manifestPath); + string line; + while (getline(f, line)) + { + if (!line.empty() && line.back() == '\r') + { + line.pop_back(); + } + if (!line.empty()) + { + lines.push_back(line); + } + } + return lines; + } + + //---------------------------------------------------------------------------------------------- + // Name: ManifestWrittenTest + // + // Description: + // Verifies that after installing a ZIP package, a manifest file named + // "{libName}.manifest" is written in the install directory and lists + // every file extracted from the package. + // + TEST_F(CSharpExtensionApiTests, ManifestWrittenTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)); + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "testpackageB", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + string manifestPath = (fs::path(installDir) / "testpackageB.manifest").string(); + ASSERT_TRUE(fs::exists(manifestPath)) << "Manifest file not created"; + + vector entries = ReadManifest(manifestPath); + EXPECT_GE(entries.size(), 2u) << "Manifest should list at least 2 extracted files"; + + // Manifest should contain the actual extracted file names + bool hasDll = false; + bool hasDeps = false; + for (const auto &e : entries) + { + if (e.find("testpackageB.dll") != string::npos) hasDll = true; + if (e.find("testpackageB.deps.json") != string::npos) hasDeps = true; + } + EXPECT_TRUE(hasDll) << "Manifest missing testpackageB.dll"; + EXPECT_TRUE(hasDeps) << "Manifest missing testpackageB.deps.json"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: ManifestListsNestedFilesTest + // + // Description: + // Verifies that the manifest records nested file paths using the + // relative path form so that uninstall can locate the files. + // + TEST_F(CSharpExtensionApiTests, ManifestListsNestedFilesTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageD-NESTED.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)); + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "nestedlib", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + string manifestPath = (fs::path(installDir) / "nestedlib.manifest").string(); + ASSERT_TRUE(fs::exists(manifestPath)); + + vector entries = ReadManifest(manifestPath); + + bool hasNestedDll = false; + bool hasRuntimeDll = false; + for (const auto &e : entries) + { + // Accept either separator for cross-platform resilience + if (e.find("MyLib.dll") != string::npos && + (e.find("net8.0") != string::npos)) + { + hasNestedDll = true; + } + if (e.find("native.dll") != string::npos && + e.find("win-x64") != string::npos) + { + hasRuntimeDll = true; + } + } + EXPECT_TRUE(hasNestedDll) << "Manifest missing lib/net8.0/MyLib.dll entry"; + EXPECT_TRUE(hasRuntimeDll) << "Manifest missing runtimes/win-x64/native.dll entry"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallLibNameAliasNoExtensionTest + // + // Description: + // When the ZIP does not contain a file matching "{libName}.*", the + // install routine creates an alias named "{libName}" (without any + // extension). Verifies the alias is present and the manifest lists it. + // + TEST_F(CSharpExtensionApiTests, InstallLibNameAliasNoExtensionTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)); + + string installDir = CreateInstallDir(); + + // Library name "myAlias" does not match the package's testpackageB.* + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "myAlias", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // Alias file created as libName exactly (no ".dll" extension) + EXPECT_TRUE(fs::exists(fs::path(installDir) / "myAlias")) + << "Expected alias file 'myAlias' (no extension) not found"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / "myAlias.dll")) + << "Alias should NOT have .dll extension"; + + // Manifest should include the alias + vector entries = ReadManifest( + (fs::path(installDir) / "myAlias.manifest").string()); + bool hasAlias = false; + for (const auto &e : entries) + { + if (e == "myAlias") { hasAlias = true; break; } + } + EXPECT_TRUE(hasAlias) << "Manifest missing alias entry 'myAlias'"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: DirectoryOverlapAllowedTest + // + // Description: + // Installing two libraries into the same install directory succeeds + // as long as they do not share any filenames. Directory (folder) + // overlap is explicitly permitted. + // + TEST_F(CSharpExtensionApiTests, DirectoryOverlapAllowedTest) + { + string packagesPath = GetPackagesPath(); + string pkgA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_TRUE(fs::exists(pkgA)); + ASSERT_TRUE(fs::exists(pkgB)); + + string installDir = CreateInstallDir(); + + // Install lib1 from package A (contents: testpackageA.dll, testpackageA.txt) + SQLRETURN r1 = CallInstall(sm_installExternalLibraryFuncPtr, + "lib1", pkgA, installDir); + EXPECT_EQ(r1, SQL_SUCCESS); + + // Install lib2 from package B (contents: testpackageB.dll, testpackageB.deps.json) + // No filename conflict => succeeds + SQLRETURN r2 = CallInstall(sm_installExternalLibraryFuncPtr, + "lib2", pkgB, installDir); + EXPECT_EQ(r2, SQL_SUCCESS); + + // Both libraries' files coexist + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageA.dll")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib1.manifest")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib2.manifest")); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: FileConflictFailsTest + // + // Description: + // Installing a second library that would overwrite a file already + // installed by another library must fail with SQL_ERROR. The first + // library's files must remain intact. + // + TEST_F(CSharpExtensionApiTests, FileConflictFailsTest) + { + string packagesPath = GetPackagesPath(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_TRUE(fs::exists(pkgB)); + + string installDir = CreateInstallDir(); + + // Install "lib1" from package B + SQLRETURN r1 = CallInstall(sm_installExternalLibraryFuncPtr, + "lib1", pkgB, installDir); + EXPECT_EQ(r1, SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + + // Install a DIFFERENT library "lib2" from the same package. + // Both would write testpackageB.dll => conflict, must fail. + SQLRETURN r2 = CallInstall(sm_installExternalLibraryFuncPtr, + "lib2", pkgB, installDir); + EXPECT_EQ(r2, SQL_ERROR) << "Expected conflict error on duplicate filename"; + + // lib1's files must survive + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib1.manifest")); + // lib2 must not have written a manifest + EXPECT_FALSE(fs::exists(fs::path(installDir) / "lib2.manifest")); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: UninstallPreservesOtherLibrariesTest + // + // Description: + // Uninstalling one library must only delete that library's files + // (as listed in its manifest). Other libraries' files in the same + // directory must remain untouched. + // + TEST_F(CSharpExtensionApiTests, UninstallPreservesOtherLibrariesTest) + { + string packagesPath = GetPackagesPath(); + string pkgA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + + string installDir = CreateInstallDir(); + + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "lib1", pkgA, installDir), SQL_SUCCESS); + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "lib2", pkgB, installDir), SQL_SUCCESS); + + // Uninstall lib1 only + SQLRETURN r = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "lib1", installDir); + EXPECT_EQ(r, SQL_SUCCESS); + + // lib1's files + manifest gone + EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.dll")); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.txt")); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "lib1.manifest")); + + // lib2's files + manifest intact + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.deps.json")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib2.manifest")); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: UninstallRemovesEmptyNestedDirsTest + // + // Description: + // After uninstalling a library whose files lived in nested + // subdirectories, the now-empty nested directories are removed + // bottom-up. + // + TEST_F(CSharpExtensionApiTests, UninstallRemovesEmptyNestedDirsTest) + { + string packagesPath = GetPackagesPath(); + string pkgD = (fs::path(packagesPath) / "testpackageD-NESTED.zip").string(); + ASSERT_TRUE(fs::exists(pkgD)); + + string installDir = CreateInstallDir(); + + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "nestedlib", pkgD, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "lib" / "net8.0")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "runtimes" / "win-x64")); + + // Uninstall + SQLRETURN r = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "nestedlib", installDir); + EXPECT_EQ(r, SQL_SUCCESS); + + // Nested directories should have been removed when they became empty + EXPECT_FALSE(fs::exists(fs::path(installDir) / "lib" / "net8.0")); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "lib")); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "runtimes" / "win-x64")); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "runtimes")); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: AlterExternalLibraryTest + // + // Description: + // Installing a library a second time with the same libName (simulating + // ALTER EXTERNAL LIBRARY) removes the old content tracked by the + // previous manifest before extracting the new package, even when the + // new package would otherwise conflict with leftover files. + // + TEST_F(CSharpExtensionApiTests, AlterExternalLibraryTest) + { + string packagesPath = GetPackagesPath(); + string pkgA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + + string installDir = CreateInstallDir(); + + // v1: install as "myLib" from package A => testpackageA.dll, testpackageA.txt + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", pkgA, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "testpackageA.dll")); + + // v2: install again as "myLib" from package B (ALTER) + SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", pkgB, installDir); + EXPECT_EQ(r, SQL_SUCCESS) << "ALTER-style reinstall should succeed"; + + // v1's unique files gone + EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.dll")); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.txt")); + + // v2's files present + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.deps.json")); + + // Manifest reflects v2 content only + vector entries = ReadManifest( + (fs::path(installDir) / "myLib.manifest").string()); + bool hasA = false, hasB = false; + for (const auto &e : entries) + { + if (e.find("testpackageA") != string::npos) hasA = true; + if (e.find("testpackageB") != string::npos) hasB = true; + } + EXPECT_FALSE(hasA) << "Manifest still references v1 files"; + EXPECT_TRUE(hasB) << "Manifest missing v2 files"; + + CleanupInstallDir(installDir); + } } From 80606500e776c4dc1f1084de9ffc48874db0ed89 Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Fri, 17 Apr 2026 06:14:29 -0700 Subject: [PATCH 04/18] Add 6 production-quality tests for Install/Uninstall API - ErrorMessagePopulatedOnFailureTest: verify libError is surfaced to SQL Server for non-existent file, zip-slip, and file-conflict failure modes - UninstallNonZipLibraryTest: raw-DLL install/uninstall (no-manifest path) - InnerZipFileConflictFailsTest: conflict detection in inner-zip code path - TempFolderCleanedUpAfterConflictTest: no GUID temp-dir leaks after failures - AlterFromNonZipToZipTest: ALTER from raw DLL to ZIP (missing-manifest case) - AliasFileRemovedOnUninstallTest: libName-alias file recorded in manifest and removed on uninstall --- .../test/src/native/CSharpLibraryTests.cpp | 302 ++++++++++++++++++ 1 file changed, 302 insertions(+) diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index 93f1f974..546f9cd9 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -134,6 +134,68 @@ namespace ExtensionApiTest return hasFiles; } + // Helper: call InstallExternalLibrary and capture the error message (if any). + // Returns SQL result; populates errorMessage with the UTF-8 error text. + // + static SQLRETURN CallInstallCaptureError( + FN_installExternalLibrary *installFunc, + const string &libName, + const string &libFilePath, + const string &installDir, + string &errorMessage) + { + SQLCHAR *libError = nullptr; + SQLINTEGER libErrorLength = 0; + + SQLRETURN result = (*installFunc)( + SQLGUID(), + reinterpret_cast(const_cast(libName.c_str())), + static_cast(libName.length()), + reinterpret_cast(const_cast(libFilePath.c_str())), + static_cast(libFilePath.length()), + reinterpret_cast(const_cast(installDir.c_str())), + static_cast(installDir.length()), + &libError, + &libErrorLength); + + errorMessage.clear(); + if (libError != nullptr && libErrorLength > 0) + { + errorMessage.assign(reinterpret_cast(libError), + static_cast(libErrorLength)); + } + + return result; + } + + // Helper: count GUID-shaped subdirectories in a directory (temp folders + // created by the install code during ZIP extraction). + // + static int CountGuidTempDirs(const string &dir) + { + int count = 0; + if (!fs::exists(dir)) + { + return 0; + } + for (const auto &entry : fs::directory_iterator(dir)) + { + if (!fs::is_directory(entry.path())) + { + continue; + } + string name = entry.path().filename().string(); + // GUID format: 8-4-4-4-12 = 36 chars with hyphens at 8,13,18,23 + if (name.length() == 36 && + name[8] == '-' && name[13] == '-' && + name[18] == '-' && name[23] == '-') + { + ++count; + } + } + return count; + } + //---------------------------------------------------------------------------------------------- // Name: InstallZipContainingZipTest // @@ -880,4 +942,244 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + + //---------------------------------------------------------------------------------------------- + // Name: ErrorMessagePopulatedOnFailureTest + // + // Description: + // SQL Server surfaces the libraryError string to end users. Every failure + // path must populate libError with a non-empty, UTF-8-decodable message. + // Validates this for three distinct failure modes: non-existent source file, + // zip-slip attack, and file-level conflict. + // + TEST_F(CSharpExtensionApiTests, ErrorMessagePopulatedOnFailureTest) + { + string packagesPath = GetPackagesPath(); + string installDir = CreateInstallDir(); + string msg; + + // Failure mode 1: non-existent source file + SQLRETURN r = CallInstallCaptureError(sm_installExternalLibraryFuncPtr, + "missing", "C:\\does\\not\\exist.zip", installDir, msg); + EXPECT_EQ(r, SQL_ERROR); + EXPECT_FALSE(msg.empty()) << "No error message populated for missing file"; + + // Failure mode 2: zip-slip attack + string zipSlip = (fs::path(packagesPath) / "testpackageH-ZIPSLIP.zip").string(); + r = CallInstallCaptureError(sm_installExternalLibraryFuncPtr, + "slip", zipSlip, installDir, msg); + EXPECT_EQ(r, SQL_ERROR); + EXPECT_FALSE(msg.empty()) << "No error message populated for zip-slip"; + + // Failure mode 3: file-level conflict + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "libA", pkgB, installDir), SQL_SUCCESS); + r = CallInstallCaptureError(sm_installExternalLibraryFuncPtr, + "libB", pkgB, installDir, msg); + EXPECT_EQ(r, SQL_ERROR); + EXPECT_FALSE(msg.empty()) << "No error message populated for conflict"; + // Message should mention the conflicting library name for diagnosability + EXPECT_NE(msg.find("libB"), string::npos) + << "Conflict message should include library name. Got: " << msg; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: UninstallNonZipLibraryTest + // + // Description: + // When a raw DLL (non-ZIP) is installed, no manifest is written. Uninstall + // must still remove the library file via the libName fallback path. + // Exercises the File.Delete(libraryFile) branch in UninstallExternalLibrary. + // + TEST_F(CSharpExtensionApiTests, UninstallNonZipLibraryTest) + { + string packagesPath = GetPackagesPath(); + string rawDll = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + ASSERT_TRUE(fs::exists(rawDll)); + + string installDir = CreateInstallDir(); + + // Install the raw DLL as "rawlib" — no manifest should be written + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "rawlib", rawDll, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "rawlib")); + ASSERT_FALSE(fs::exists(fs::path(installDir) / "rawlib.manifest")) + << "Raw-DLL install should not write a manifest"; + + // Uninstall must still delete the library file via the libName fallback + SQLRETURN r = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "rawlib", installDir); + EXPECT_EQ(r, SQL_SUCCESS); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "rawlib")) + << "Raw library file not removed by uninstall"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InnerZipFileConflictFailsTest + // + // Description: + // FileConflictFailsTest exercises the direct-files code path (package B, + // no inner zip). This test exercises the inner-zip code path (package A) + // which has its own separate conflict-detection loop. Regressing only + // one path must be caught. + // + TEST_F(CSharpExtensionApiTests, InnerZipFileConflictFailsTest) + { + string packagesPath = GetPackagesPath(); + string pkgA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + ASSERT_TRUE(fs::exists(pkgA)); + + string installDir = CreateInstallDir(); + + // Install "lib1" from package A (inner-zip path) => testpackageA.dll + .txt + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "lib1", pkgA, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "testpackageA.dll")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "lib1.manifest")); + + // Install "lib2" from the same package — inner-zip conflict must fail + SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, + "lib2", pkgA, installDir); + EXPECT_EQ(r, SQL_ERROR) << "Inner-zip path must detect filename conflict"; + + // lib1's state must be untouched + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageA.dll")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib1.manifest")); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "lib2.manifest")); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: TempFolderCleanedUpAfterConflictTest + // + // Description: + // After a failed install (conflict or otherwise), the GUID-named temp + // folder used for outer-zip extraction must be cleaned up by the finally + // block. Regression would slowly leak disk space on every failed install. + // + TEST_F(CSharpExtensionApiTests, TempFolderCleanedUpAfterConflictTest) + { + string packagesPath = GetPackagesPath(); + string pkgA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + + string installDir = CreateInstallDir(); + + // Trigger a conflict: install then reinstall same package under different names + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "lib1", pkgB, installDir), SQL_SUCCESS); + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "lib2", pkgB, installDir), SQL_ERROR); + + // Also trigger inner-zip path conflict + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "libA", pkgA, installDir), SQL_SUCCESS); + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "libB", pkgA, installDir), SQL_ERROR); + + // Also trigger zip-slip failure + string zipSlip = (fs::path(packagesPath) / "testpackageH-ZIPSLIP.zip").string(); + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "slip", zipSlip, installDir), SQL_ERROR); + + EXPECT_EQ(CountGuidTempDirs(installDir), 0) + << "GUID-named temp folder leaked after failed install(s)"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: AlterFromNonZipToZipTest + // + // Description: + // ALTER EXTERNAL LIBRARY scenario where v1 was a raw DLL (no manifest) + // and v2 is a ZIP package. Install code must handle the missing-manifest + // case gracefully and complete the new install. The old lib file remains + // until explicit uninstall — that matches sicongliu's baseline semantics. + // + TEST_F(CSharpExtensionApiTests, AlterFromNonZipToZipTest) + { + string packagesPath = GetPackagesPath(); + string rawDll = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + + string installDir = CreateInstallDir(); + + // v1: raw DLL install (no manifest created) + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", rawDll, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib")); + ASSERT_FALSE(fs::exists(fs::path(installDir) / "myLib.manifest")); + + // v2: ZIP install of the same libName — must NOT crash on missing manifest + SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", pkgB, installDir); + EXPECT_EQ(r, SQL_SUCCESS) + << "ALTER from non-ZIP to ZIP should succeed even without prior manifest"; + + // v2's files must be present + v2 manifest must exist + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: AliasFileRemovedOnUninstallTest + // + // Description: + // When a ZIP contains DLLs whose names don't match the library name, + // the install code creates an alias file named exactly {libName} (no + // extension) so DllUtils.CreateDllList can discover it. This alias + // must be recorded in the manifest and removed on uninstall — + // otherwise orphaned alias files accumulate over install/uninstall + // cycles. + // + TEST_F(CSharpExtensionApiTests, AliasFileRemovedOnUninstallTest) + { + string packagesPath = GetPackagesPath(); + string pkgA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + + string installDir = CreateInstallDir(); + + // Install package A (contains testpackageA.dll) under a different libName. + // Since no file matches "aliaslib.*", the install code must create an + // "aliaslib" alias file copied from the first DLL. + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "aliaslib", pkgA, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "aliaslib")) + << "Alias file not created"; + + // The alias must also be listed in the manifest + vector entries = ReadManifest( + (fs::path(installDir) / "aliaslib.manifest").string()); + bool hasAlias = false; + for (const auto &e : entries) + { + if (e == "aliaslib") + { + hasAlias = true; + break; + } + } + EXPECT_TRUE(hasAlias) << "Alias file not recorded in manifest"; + + // Uninstall must remove both the content and the alias + ASSERT_EQ(CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "aliaslib", installDir), SQL_SUCCESS); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "aliaslib")) + << "Alias file leaked after uninstall"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.dll")) + << "Content file leaked after uninstall"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / "aliaslib.manifest")) + << "Manifest file leaked after uninstall"; + + CleanupInstallDir(installDir); + } } From f55001dec2b938a925ad348ce7749becfcbcd9f3 Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Fri, 17 Apr 2026 06:42:26 -0700 Subject: [PATCH 05/18] Address review: alias naming, zip-slip DID, ALTER atomicity, libName validation, Linux case-sensitivity, cleanup - Alias file now written as {libName}.dll so DllUtils.CreateDllList ({libName}.*) finds it (fixes feature regression on Linux) - Raw-DLL install writes {libName}.dll; uninstall fallback updated to match - Defense-in-depth zip-slip check on inner-ZIP entries before adding to manifest and again in CleanupManifest before deleting - ALTER is now transactional: stage + validate new ZIP before removing old version, with manifest-aware conflict suppression - Reject library names containing path separators, '..', null, or absolute paths - Use OS-appropriate path comparer (Ordinal on Linux, OrdinalIgnoreCase on Windows) - Sort dirs for bottom-up cleanup by separator count, not string length - Extract conflict detection into single helper (removes duplicate check) - Trim error messages and comments; update 3 tests to match new alias naming --- .../src/managed/CSharpExtension.cs | 364 +++++++++++------- .../test/src/native/CSharpLibraryTests.cpp | 72 ++-- 2 files changed, 251 insertions(+), 185 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 17f97d8d..9e2fb4eb 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -686,180 +686,131 @@ public static short InstallExternalLibrary( string installDir = Interop.UTF8PtrToStr(libraryInstallDirectory, (ulong)libraryInstallDirectoryLength); string libName = Interop.UTF8PtrToStr(libraryName, (ulong)libraryNameLength); - // If a manifest from a previous install of this same library - // exists (e.g. ALTER EXTERNAL LIBRARY), clean up the old files - // before installing the new content. This prevents orphaned - // files and avoids false conflict errors when the new ZIP - // contains some of the same filenames as the old one. - string existingManifest = Path.Combine(installDir, libName + ".manifest"); - if (File.Exists(existingManifest)) - { - CleanupManifest(existingManifest, installDir); - } + ValidateLibraryName(libName); - // Check if the file is actually a ZIP by reading magic bytes (PK prefix). - // All ZIP format variants start with 0x50 0x4B ('PK'). - // If not a ZIP (e.g. a raw DLL), there is nothing to extract — the file - // is already stored by SQL Server where DllUtils will find it, so we - // return success without any action. - bool isZipFile = false; - using (var fs = new FileStream(libFilePath, FileMode.Open, FileAccess.Read)) + if (!Directory.Exists(installDir)) { - byte[] magic = new byte[2]; - isZipFile = fs.Read(magic, 0, 2) == 2 && - magic[0] == 0x50 && magic[1] == 0x4B; + Directory.CreateDirectory(installDir); } - if (isZipFile) - { - tempFolder = Path.Combine(installDir, Guid.NewGuid().ToString()); + string manifestPath = Path.Combine(installDir, libName + ".manifest"); + HashSet oldManifestEntries = ReadManifestEntries(manifestPath); - // Extract the outer zip to a temp directory. - if (Directory.Exists(tempFolder)) + if (!IsZipFile(libFilePath)) + { + // Raw DLL. Clean up any previous manifest-based install of the same + // library, then copy the file as "{libName}.dll" so DllUtils can find it. + if (File.Exists(manifestPath)) { - Directory.Delete(tempFolder, true); + CleanupManifest(manifestPath, installDir); } - ZipFile.ExtractToDirectory(libFilePath, tempFolder); + File.Copy(libFilePath, Path.Combine(installDir, libName + ".dll"), true); + return SQL_SUCCESS; + } - // Verify the archive contained at least one entry - if (Directory.GetFiles(tempFolder).Length == 0 && - Directory.GetDirectories(tempFolder).Length == 0) - { - throw new InvalidOperationException( - "The library archive contains no entries. Nothing to install."); - } + // ZIP path. Stage the new content to a temp folder, validate it, then + // clean up the old version and move the new files into place. This keeps + // ALTER EXTERNAL LIBRARY transactional: a corrupt ZIP leaves the existing + // install intact. + tempFolder = Path.Combine(installDir, Guid.NewGuid().ToString()); + ZipFile.ExtractToDirectory(libFilePath, tempFolder); - // Look for an inner zip file - string innerZipPath = null; - foreach (string file in Directory.GetFiles(tempFolder)) - { - if (Path.GetExtension(file).Equals(".zip", StringComparison.OrdinalIgnoreCase)) - { - innerZipPath = file; - break; - } - } + if (Directory.GetFiles(tempFolder).Length == 0 && + Directory.GetDirectories(tempFolder).Length == 0) + { + throw new InvalidOperationException( + "The library archive contains no entries."); + } - if (!Directory.Exists(installDir)) + string innerZipPath = null; + foreach (string file in Directory.GetFiles(tempFolder)) + { + if (Path.GetExtension(file).Equals(".zip", StringComparison.OrdinalIgnoreCase)) { - Directory.CreateDirectory(installDir); + innerZipPath = file; + break; } + } - // Check for file-level conflicts before extracting. - // Directory (folder) overlaps are allowed — multiple libraries - // may extract into the same parent folders. - var extractedFiles = new List(); - - if (innerZipPath != null) + // Collect relative paths we're about to install, validating each path + // stays under installDir (defense-in-depth zip-slip check on top of + // ZipFile.ExtractToDirectory's own validation). + var extractedFiles = new List(); + if (innerZipPath != null) + { + using (var innerArchive = ZipFile.OpenRead(innerZipPath)) { - using (var innerArchive = ZipFile.OpenRead(innerZipPath)) + foreach (var entry in innerArchive.Entries) { - foreach (var entry in innerArchive.Entries) - { - if (string.IsNullOrEmpty(entry.Name)) - continue; - - string relPath = entry.FullName.Replace('/', Path.DirectorySeparatorChar); - string destFile = Path.Combine(installDir, relPath); - - if (File.Exists(destFile)) - { - throw new InvalidOperationException( - $"Cannot install library '{libName}': file '{relPath}' already exists. " + - "Another library has already installed a file with this name."); - } - - extractedFiles.Add(relPath); - } - } + if (string.IsNullOrEmpty(entry.Name)) + continue; - // All conflict checks passed — extract - ZipFile.ExtractToDirectory(innerZipPath, installDir, false); - } - else - { - // Collect files from the extracted temp directory - CollectRelativeFiles(tempFolder, "", extractedFiles); - - // Check for file conflicts before copying - foreach (string relPath in extractedFiles) - { - string destFile = Path.Combine(installDir, relPath); - if (File.Exists(destFile)) - { - throw new InvalidOperationException( - $"Cannot install library '{libName}': file '{relPath}' already exists. " + - "Another library has already installed a file with this name."); - } + extractedFiles.Add(ValidateRelativePath(installDir, entry.FullName)); } + } + } + else + { + CollectRelativeFiles(tempFolder, "", extractedFiles); + } - // All conflict checks passed — copy files - foreach (string file in Directory.GetFiles(tempFolder)) - { - string destFile = Path.Combine(installDir, Path.GetFileName(file)); - File.Copy(file, destFile, false); - } + CheckForConflicts(installDir, libName, extractedFiles, oldManifestEntries); - // Copy subdirectories (merge into existing dirs) - foreach (string dir in Directory.GetDirectories(tempFolder)) - { - string destDir = Path.Combine(installDir, Path.GetFileName(dir)); - CopyDirectory(dir, destDir); - } - } + // All checks passed. Remove the previous version's files (if any), then + // extract/copy the new content into installDir. + if (File.Exists(manifestPath)) + { + CleanupManifest(manifestPath, installDir); + } - // Ensure at least one file matches the library name pattern so that - // DllUtils.CreateDllList (which searches for "{libName}.*") can find it. - // When the ZIP contains files with different names (e.g. "RegexSample.dll" - // for a library named "regex"), copy the first DLL as "{libName}.dll". - if (Directory.GetFiles(installDir, libName + ".*").Length == 0 && - !File.Exists(Path.Combine(installDir, libName))) + if (innerZipPath != null) + { + ZipFile.ExtractToDirectory(innerZipPath, installDir, false); + } + else + { + foreach (string file in Directory.GetFiles(tempFolder)) { - string[] dlls = Directory.GetFiles(installDir, "*.dll"); - if (dlls.Length > 0) - { - string destFile = Path.Combine(installDir, libName); - File.Copy(dlls[0], destFile, true); - extractedFiles.Add(libName); - } + File.Copy(file, Path.Combine(installDir, Path.GetFileName(file)), false); } - - // Write a manifest listing all extracted file paths so that - // UninstallExternalLibrary can remove exactly these files. - if (extractedFiles.Count > 0) + foreach (string dir in Directory.GetDirectories(tempFolder)) { - string manifestPath = Path.Combine(installDir, libName + ".manifest"); - File.WriteAllLines(manifestPath, extractedFiles); + CopyDirectory(dir, Path.Combine(installDir, Path.GetFileName(dir))); } } - else + + // If no file in installDir matches "{libName}.*", copy the first .dll + // found as "{libName}.dll" so DllUtils.CreateDllList can discover it. + if (Directory.GetFiles(installDir, libName + ".*").Length == 0) { - // Not a ZIP (e.g. a raw DLL) — copy file to the install directory - // using the library name so DllUtils can discover it. - if (!Directory.Exists(installDir)) + string[] dlls = Directory.GetFiles(installDir, "*.dll"); + if (dlls.Length > 0) { - Directory.CreateDirectory(installDir); + string alias = Path.Combine(installDir, libName + ".dll"); + File.Copy(dlls[0], alias, false); + extractedFiles.Add(libName + ".dll"); } + } - string destFile = Path.Combine(installDir, libName); - File.Copy(libFilePath, destFile, true); + if (extractedFiles.Count > 0) + { + File.WriteAllLines(manifestPath, extractedFiles); } } catch (Exception e) { - var stackTracePart = string.IsNullOrEmpty(e.StackTrace) ? string.Empty : e.StackTrace + Environment.NewLine; + string stackTracePart = string.IsNullOrEmpty(e.StackTrace) ? string.Empty : e.StackTrace + Environment.NewLine; Logging.Error(stackTracePart + "Error: " + e.Message); SetLibraryError(e.Message, libraryError, libraryErrorLength); result = SQL_ERROR; } finally { - // Clean up the temp folder if (tempFolder != null && Directory.Exists(tempFolder)) { try { Directory.Delete(tempFolder, true); } - catch { /* Best effort cleanup */ } + catch { /* best-effort */ } } } @@ -913,9 +864,9 @@ public static short UninstallExternalLibrary( CleanupManifest(manifestPath, installDir); } - // Delete the library file itself (for non-ZIP libraries - // that were copied directly during install) - string libraryFile = Path.Combine(installDir, libName); + // Raw-DLL fallback: libraries installed without a manifest + // were copied directly as "{libName}.dll". + string libraryFile = Path.Combine(installDir, libName + ".dll"); if (File.Exists(libraryFile)) { File.Delete(libraryFile); @@ -992,42 +943,148 @@ private static void CollectRelativeFiles(string directory, string prefix, List= 0 || + libName.Contains("..") || + Path.IsPathRooted(libName)) + { + throw new ArgumentException( + $"Library name '{libName}' contains invalid characters."); + } + } + + // A ZIP file of any variant begins with the 'PK' magic bytes (0x50 0x4B). + private static bool IsZipFile(string path) + { + using (var fs = new FileStream(path, FileMode.Open, FileAccess.Read)) + { + byte[] magic = new byte[2]; + return fs.Read(magic, 0, 2) == 2 && magic[0] == 0x50 && magic[1] == 0x4B; + } + } + + // Reads an existing manifest into a set of relative paths. Empty set if absent. + private static HashSet ReadManifestEntries(string manifestPath) + { + var set = new HashSet(s_pathComparer); + if (File.Exists(manifestPath)) + { + foreach (string line in File.ReadAllLines(manifestPath)) + { + if (!string.IsNullOrWhiteSpace(line)) + { + set.Add(line); + } + } + } + return set; + } + + // Converts a ZIP entry path to a platform-native relative path and verifies + // it does not escape installDir (defense-in-depth zip-slip check). + private static string ValidateRelativePath(string installDir, string zipEntryFullName) + { + string relPath = zipEntryFullName.Replace('/', Path.DirectorySeparatorChar); + string fullInstall = Path.GetFullPath(installDir); + string fullCombined = Path.GetFullPath(Path.Combine(fullInstall, relPath)); + string sep = Path.DirectorySeparatorChar.ToString(); + string prefix = fullInstall.EndsWith(sep) ? fullInstall : fullInstall + sep; + + if (!fullCombined.StartsWith(prefix, s_pathComparison)) + { + throw new InvalidOperationException( + $"Library archive contains entry with invalid path: '{zipEntryFullName}'."); + } + return relPath; + } + + // Throws if any staged relative path collides with an existing file that is not + // owned by the previous install of this same library. + private static void CheckForConflicts( + string installDir, + string libName, + List relPaths, + HashSet ownedByPrevious) + { + foreach (string relPath in relPaths) + { + if (ownedByPrevious.Contains(relPath)) + { + continue; + } + if (File.Exists(Path.Combine(installDir, relPath))) + { + throw new InvalidOperationException( + $"Cannot install library '{libName}': file '{relPath}' already exists in the install directory."); + } + } + } + /// - /// Reads a manifest file, deletes all listed files, removes any - /// directories that become empty (bottom-up), then deletes the - /// manifest itself. Used by both UninstallExternalLibrary and - /// InstallExternalLibrary (to clean up a previous version during - /// ALTER EXTERNAL LIBRARY). + /// Reads a manifest, deletes each listed file, removes any directories + /// that become empty (bottom-up), then deletes the manifest itself. /// private static void CleanupManifest(string manifestPath, string installDir) { - string[] extractedFiles = File.ReadAllLines(manifestPath); - var parentDirs = new HashSet(StringComparer.OrdinalIgnoreCase); + string fullInstall = Path.GetFullPath(installDir); + string sep = Path.DirectorySeparatorChar.ToString(); + string prefix = fullInstall.EndsWith(sep) ? fullInstall : fullInstall + sep; - foreach (string relPath in extractedFiles) + string[] entries = File.ReadAllLines(manifestPath); + var parentDirs = new HashSet(s_pathComparer); + + foreach (string relPath in entries) { if (string.IsNullOrWhiteSpace(relPath)) + { continue; + } + + string fullPath; + try + { + fullPath = Path.GetFullPath(Path.Combine(fullInstall, relPath)); + } + catch + { + continue; + } + + // Defense in depth: skip any entry that resolves outside installDir. + if (!fullPath.StartsWith(prefix, s_pathComparison)) + { + continue; + } - string fullPath = Path.Combine(installDir, relPath); if (File.Exists(fullPath)) { File.Delete(fullPath); } - // Track parent directories for empty-dir cleanup string dir = Path.GetDirectoryName(fullPath); while (!string.IsNullOrEmpty(dir) && - !dir.Equals(installDir, StringComparison.OrdinalIgnoreCase)) + !dir.Equals(fullInstall, s_pathComparison)) { parentDirs.Add(dir); dir = Path.GetDirectoryName(dir); } } - // Remove empty directories bottom-up (deepest first) + // Remove empty directories deepest first. var sortedDirs = new List(parentDirs); - sortedDirs.Sort((a, b) => b.Length.CompareTo(a.Length)); + sortedDirs.Sort((a, b) => SeparatorCount(b).CompareTo(SeparatorCount(a))); foreach (string dir in sortedDirs) { if (Directory.Exists(dir) && @@ -1040,5 +1097,18 @@ private static void CleanupManifest(string manifestPath, string installDir) File.Delete(manifestPath); } + + private static int SeparatorCount(string path) + { + int count = 0; + for (int i = 0; i < path.Length; i++) + { + if (path[i] == Path.DirectorySeparatorChar) + { + count++; + } + } + return count; + } } } diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index 546f9cd9..b4c6bbfc 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -275,8 +275,8 @@ namespace ExtensionApiTest "bad-package", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - EXPECT_TRUE(fs::exists(fs::path(installDir) / "bad-package")) - << "Raw file not found in install directory as bad-package"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "bad-package.dll")) + << "Raw file not found in install directory as bad-package.dll"; CleanupInstallDir(installDir); } @@ -448,9 +448,9 @@ namespace ExtensionApiTest "rawdllpackage", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - // The raw DLL should be copied using the library name (no extension) - EXPECT_TRUE(fs::exists(fs::path(installDir) / "rawdllpackage")) - << "Raw DLL not found in install directory as rawdllpackage"; + // The raw DLL should be copied using the library name with .dll extension. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "rawdllpackage.dll")) + << "Raw DLL not found in install directory as rawdllpackage.dll"; CleanupInstallDir(installDir); } @@ -512,8 +512,8 @@ namespace ExtensionApiTest } EXPECT_EQ(dllCount, 50) << "Expected 50 extracted DLL files, found " << dllCount; - // Verify the extensionless alias exists so DllUtils can discover the library by name - EXPECT_TRUE(fs::exists(fs::path(installDir) / "manyfilespackage")); + // Verify the alias exists so DllUtils can discover the library by name. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "manyfilespackage.dll")); CleanupInstallDir(installDir); } @@ -705,14 +705,14 @@ namespace ExtensionApiTest } //---------------------------------------------------------------------------------------------- - // Name: InstallLibNameAliasNoExtensionTest + // Name: InstallLibNameAliasTest // // Description: // When the ZIP does not contain a file matching "{libName}.*", the - // install routine creates an alias named "{libName}" (without any - // extension). Verifies the alias is present and the manifest lists it. + // install routine creates an alias named "{libName}.dll" so that + // DllUtils.CreateDllList (which searches "{libName}.*") can find it. // - TEST_F(CSharpExtensionApiTests, InstallLibNameAliasNoExtensionTest) + TEST_F(CSharpExtensionApiTests, InstallLibNameAliasTest) { string packagesPath = GetPackagesPath(); string packagePath = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); @@ -725,21 +725,18 @@ namespace ExtensionApiTest "myAlias", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - // Alias file created as libName exactly (no ".dll" extension) - EXPECT_TRUE(fs::exists(fs::path(installDir) / "myAlias")) - << "Expected alias file 'myAlias' (no extension) not found"; - EXPECT_FALSE(fs::exists(fs::path(installDir) / "myAlias.dll")) - << "Alias should NOT have .dll extension"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "myAlias.dll")) + << "Expected alias file 'myAlias.dll' not found"; - // Manifest should include the alias + // Manifest should include the alias. vector entries = ReadManifest( (fs::path(installDir) / "myAlias.manifest").string()); bool hasAlias = false; for (const auto &e : entries) { - if (e == "myAlias") { hasAlias = true; break; } + if (e == "myAlias.dll") { hasAlias = true; break; } } - EXPECT_TRUE(hasAlias) << "Manifest missing alias entry 'myAlias'"; + EXPECT_TRUE(hasAlias) << "Manifest missing alias entry 'myAlias.dll'"; CleanupInstallDir(installDir); } @@ -1002,18 +999,18 @@ namespace ExtensionApiTest string installDir = CreateInstallDir(); - // Install the raw DLL as "rawlib" — no manifest should be written + // Install the raw DLL as "rawlib" — no manifest should be written. ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, "rawlib", rawDll, installDir), SQL_SUCCESS); - ASSERT_TRUE(fs::exists(fs::path(installDir) / "rawlib")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "rawlib.dll")); ASSERT_FALSE(fs::exists(fs::path(installDir) / "rawlib.manifest")) << "Raw-DLL install should not write a manifest"; - // Uninstall must still delete the library file via the libName fallback + // Uninstall must still delete the library file via the libName fallback. SQLRETURN r = CallUninstall(sm_uninstallExternalLibraryFuncPtr, "rawlib", installDir); EXPECT_EQ(r, SQL_SUCCESS); - EXPECT_FALSE(fs::exists(fs::path(installDir) / "rawlib")) + EXPECT_FALSE(fs::exists(fs::path(installDir) / "rawlib.dll")) << "Raw library file not removed by uninstall"; CleanupInstallDir(installDir); @@ -1111,19 +1108,19 @@ namespace ExtensionApiTest string installDir = CreateInstallDir(); - // v1: raw DLL install (no manifest created) + // v1: raw DLL install (no manifest created). ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, "myLib", rawDll, installDir), SQL_SUCCESS); - ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.dll")); ASSERT_FALSE(fs::exists(fs::path(installDir) / "myLib.manifest")); - // v2: ZIP install of the same libName — must NOT crash on missing manifest + // v2: ZIP install of the same libName — must NOT crash on missing manifest. SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, "myLib", pkgB, installDir); EXPECT_EQ(r, SQL_SUCCESS) << "ALTER from non-ZIP to ZIP should succeed even without prior manifest"; - // v2's files must be present + v2 manifest must exist + // v2's files must be present + v2 manifest must exist. EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); EXPECT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); @@ -1135,11 +1132,10 @@ namespace ExtensionApiTest // // Description: // When a ZIP contains DLLs whose names don't match the library name, - // the install code creates an alias file named exactly {libName} (no - // extension) so DllUtils.CreateDllList can discover it. This alias - // must be recorded in the manifest and removed on uninstall — - // otherwise orphaned alias files accumulate over install/uninstall - // cycles. + // the install code creates an alias file named {libName}.dll so + // DllUtils.CreateDllList can discover it. This alias must be recorded + // in the manifest and removed on uninstall — otherwise orphaned alias + // files accumulate over install/uninstall cycles. // TEST_F(CSharpExtensionApiTests, AliasFileRemovedOnUninstallTest) { @@ -1150,19 +1146,19 @@ namespace ExtensionApiTest // Install package A (contains testpackageA.dll) under a different libName. // Since no file matches "aliaslib.*", the install code must create an - // "aliaslib" alias file copied from the first DLL. + // "aliaslib.dll" alias file copied from the first DLL. ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, "aliaslib", pkgA, installDir), SQL_SUCCESS); - ASSERT_TRUE(fs::exists(fs::path(installDir) / "aliaslib")) + ASSERT_TRUE(fs::exists(fs::path(installDir) / "aliaslib.dll")) << "Alias file not created"; - // The alias must also be listed in the manifest + // The alias must also be listed in the manifest. vector entries = ReadManifest( (fs::path(installDir) / "aliaslib.manifest").string()); bool hasAlias = false; for (const auto &e : entries) { - if (e == "aliaslib") + if (e == "aliaslib.dll") { hasAlias = true; break; @@ -1170,10 +1166,10 @@ namespace ExtensionApiTest } EXPECT_TRUE(hasAlias) << "Alias file not recorded in manifest"; - // Uninstall must remove both the content and the alias + // Uninstall must remove both the content and the alias. ASSERT_EQ(CallUninstall(sm_uninstallExternalLibraryFuncPtr, "aliaslib", installDir), SQL_SUCCESS); - EXPECT_FALSE(fs::exists(fs::path(installDir) / "aliaslib")) + EXPECT_FALSE(fs::exists(fs::path(installDir) / "aliaslib.dll")) << "Alias file leaked after uninstall"; EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.dll")) << "Content file leaked after uninstall"; From 0b3916b9a5f394e2582dac1ec95f52b71bbce696 Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Fri, 17 Apr 2026 06:48:45 -0700 Subject: [PATCH 06/18] Remove 'fallback' language from comments --- .../dotnet-core-CSharp/src/managed/CSharpExtension.cs | 4 ++-- .../dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 9e2fb4eb..70e6cba4 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -864,8 +864,8 @@ public static short UninstallExternalLibrary( CleanupManifest(manifestPath, installDir); } - // Raw-DLL fallback: libraries installed without a manifest - // were copied directly as "{libName}.dll". + // Non-ZIP installs write a single "{libName}.dll" file and + // no manifest; remove that file directly. string libraryFile = Path.Combine(installDir, libName + ".dll"); if (File.Exists(libraryFile)) { diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index b4c6bbfc..599b65bc 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -988,7 +988,7 @@ namespace ExtensionApiTest // // Description: // When a raw DLL (non-ZIP) is installed, no manifest is written. Uninstall - // must still remove the library file via the libName fallback path. + // must still remove the library file by its libName.dll path. // Exercises the File.Delete(libraryFile) branch in UninstallExternalLibrary. // TEST_F(CSharpExtensionApiTests, UninstallNonZipLibraryTest) @@ -1006,7 +1006,7 @@ namespace ExtensionApiTest ASSERT_FALSE(fs::exists(fs::path(installDir) / "rawlib.manifest")) << "Raw-DLL install should not write a manifest"; - // Uninstall must still delete the library file via the libName fallback. + // Uninstall must still delete the libName.dll file. SQLRETURN r = CallUninstall(sm_uninstallExternalLibraryFuncPtr, "rawlib", installDir); EXPECT_EQ(r, SQL_SUCCESS); From af9f8d721a5bd095e5d2c73b05543a63f498f52b Mon Sep 17 00:00:00 2001 From: stuartpa Date: Fri, 17 Apr 2026 09:07:20 -0700 Subject: [PATCH 07/18] Add Microsoft.Data.SqlClient 5.2.2 reference to csproj This PR branch is based on sicongliu's branch which predates PR #83 (DECIMAL Type Support on main). PR #83 added Microsoft.Data.SqlClient 5.2.2 as a required runtime dependency for SqlDecimal handling. Without it, sp_execute_external_script fails with HRESULT 0x80004004 when any script touches DECIMAL types. --- .../src/managed/Microsoft.SqlServer.CSharpExtension.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj b/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj index 3b8c8e44..82cb3f69 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj +++ b/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj @@ -11,5 +11,6 @@ + \ No newline at end of file From a342b0c2b4e6d1faccbba42a43b47686d67588cf Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Tue, 21 Apr 2026 07:23:46 -0700 Subject: [PATCH 08/18] Fix: do not append .dll when library name already ends in .dll When CREATE EXTERNAL LIBRARY is called with a name ending in .dll (e.g. [Scriptoria.dll] WITH (LANGUAGE='dotnet')), the raw-DLL install path and missing-alias fallback both unconditionally appended '.dll', producing files like 'Scriptoria.dll.dll' on disk. The CLR assembly resolver could not locate the assembly by simple name and ExtHost died with 'Could not load file or assembly Scriptoria'. Added DllFileNameFor(libName) helper that returns libName as-is if it already ends in .dll (case-insensitive on Windows via existing s_pathComparison), otherwise appends .dll. Applied at the three sites in CSharpExtension.cs: raw-DLL install path, missing-alias fallback inside the ZIP install path, and UninstallExternalLibrary raw-DLL cleanup. ZIP-based installs whose library names do not end in .dll are unaffected. Verified end-to-end on consumer side: Test-SpAiTaskSkills.ps1 reports 4 passed, 0 failed. --- .../src/managed/CSharpExtension.cs | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 70e6cba4..4b32fc01 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -705,7 +705,7 @@ public static short InstallExternalLibrary( CleanupManifest(manifestPath, installDir); } - File.Copy(libFilePath, Path.Combine(installDir, libName + ".dll"), true); + File.Copy(libFilePath, Path.Combine(installDir, DllFileNameFor(libName)), true); return SQL_SUCCESS; } @@ -782,14 +782,16 @@ public static short InstallExternalLibrary( // If no file in installDir matches "{libName}.*", copy the first .dll // found as "{libName}.dll" so DllUtils.CreateDllList can discover it. - if (Directory.GetFiles(installDir, libName + ".*").Length == 0) + string aliasFileName = DllFileNameFor(libName); + if (Directory.GetFiles(installDir, libName + ".*").Length == 0 && + !File.Exists(Path.Combine(installDir, aliasFileName))) { string[] dlls = Directory.GetFiles(installDir, "*.dll"); if (dlls.Length > 0) { - string alias = Path.Combine(installDir, libName + ".dll"); + string alias = Path.Combine(installDir, aliasFileName); File.Copy(dlls[0], alias, false); - extractedFiles.Add(libName + ".dll"); + extractedFiles.Add(aliasFileName); } } @@ -866,7 +868,7 @@ public static short UninstallExternalLibrary( // Non-ZIP installs write a single "{libName}.dll" file and // no manifest; remove that file directly. - string libraryFile = Path.Combine(installDir, libName + ".dll"); + string libraryFile = Path.Combine(installDir, DllFileNameFor(libName)); if (File.Exists(libraryFile)) { File.Delete(libraryFile); @@ -949,6 +951,21 @@ private static void CollectRelativeFiles(string directory, string prefix, List Date: Tue, 21 Apr 2026 07:46:50 -0700 Subject: [PATCH 09/18] Add regression test for double-.dll bug InstallRawDllWithDllSuffixedLibNameTest covers the case where the library is created via CREATE EXTERNAL LIBRARY [foo.dll] (libName already ending in .dll). Asserts that the raw DLL is written as 'foo.dll' (not 'foo.dll.dll') and that uninstall removes it from the same single-.dll path. Pairs with the DllFileNameFor helper added in this PR. --- .../test/src/native/CSharpLibraryTests.cpp | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index 599b65bc..1e8e584c 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -1178,4 +1178,45 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + + //---------------------------------------------------------------------------------------------- + // Name: InstallRawDllWithDllSuffixedLibNameTest + // + // Description: + // Regression test for the double-".dll" bug. When a caller invokes + // CREATE EXTERNAL LIBRARY [foo.dll] (i.e. the registered library name + // already ends in ".dll") with raw-DLL CONTENT, the install code must + // write the file as "foo.dll" — NOT "foo.dll.dll". The CLR assembly + // resolver looks up assemblies by simple name and will not find + // "foo.dll.dll" when asked to load "foo". + // + // Symmetric behavior is required on uninstall: the file removed must + // be "foo.dll", not "foo.dll.dll". + // + TEST_F(CSharpExtensionApiTests, InstallRawDllWithDllSuffixedLibNameTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + ASSERT_TRUE(fs::exists(packagePath)) << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + // libName already ends in ".dll" — must NOT get a second ".dll" appended. + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "rawdllpackage.dll", packagePath, installDir), SQL_SUCCESS); + + EXPECT_TRUE(fs::exists(fs::path(installDir) / "rawdllpackage.dll")) + << "Raw DLL not found at expected single-.dll path"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / "rawdllpackage.dll.dll")) + << "Raw DLL incorrectly written with double-.dll extension; " + "CLR assembly resolver would fail to locate it."; + + // Uninstall must also use the single-.dll path. + ASSERT_EQ(CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "rawdllpackage.dll", installDir), SQL_SUCCESS); + EXPECT_FALSE(fs::exists(fs::path(installDir) / "rawdllpackage.dll")) + << "Raw DLL not removed by uninstall"; + + CleanupInstallDir(installDir); + } } From 246067b969c6cf045f369982e4bb71d14392733c Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Tue, 21 Apr 2026 08:02:17 -0700 Subject: [PATCH 10/18] Address PR #85 review: DllUtils exact-name, Uninstall libName validation, recursive/conflict-checked alias, CopyDirectory no-overwrite DllUtils.CreateDllList: try exact userLibName first; fall back to userLibName + '.*' only if no exact match. Extracted into AddMatches helper. Fixes callers that pass an explicit filename like 'Foo.dll' which previously became 'Foo.dll.*' and matched nothing. CSharpExtension.UninstallExternalLibrary: call ValidateLibraryName before building manifestPath / libraryFile. Prevents malicious or legacy names with path separators from resolving outside installDir. CSharpExtension.InstallExternalLibrary alias creation: (a) search the full extracted tree (not just top-level) for an existing '{libName}.*' before deciding to create an alias; (b) include the alias path in the conflict-check input so a collision fails BEFORE any content is written to installDir. Prevents partial-state failures when another library already owns '{libName}.dll' at the root. CSharpExtension.CopyDirectory: use File.Copy overwrite:false (was overwrite:true) so TOCTOU changes between conflict-check and write fail loud rather than silently clobbering another library's files. Tests: added UninstallRejectsPathTraversalLibNameTest and AliasConflictDetectedBeforeExtractionTest to cover the new behaviors. --- .../src/managed/CSharpExtension.cs | 64 ++++++++++++--- .../src/managed/utils/DllUtils.cs | 40 +++++++--- .../test/src/native/CSharpLibraryTests.cpp | 77 +++++++++++++++++++ 3 files changed, 160 insertions(+), 21 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 4b32fc01..aeeccf9f 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -755,6 +755,43 @@ public static short InstallExternalLibrary( CollectRelativeFiles(tempFolder, "", extractedFiles); } + // Determine whether we need a "{libName}.dll" alias so + // DllUtils.CreateDllList can discover the library. Examine the + // entire extracted tree (not just the top level) so packages + // laid out under e.g. lib/... do not generate a redundant alias + // when a DLL named "{libName}.dll" (or "{libName}.*") is already + // present deeper in the tree. If an alias IS needed, include it + // in extractedFiles now so CheckForConflicts fails BEFORE we + // start writing to installDir. + string aliasFileName = DllFileNameFor(libName); + string aliasSourceRelPath = null; + bool libNameAlreadyPresent = false; + foreach (string relPath in extractedFiles) + { + string name = Path.GetFileName(relPath); + if (name.StartsWith(libName + ".", s_pathComparison) || + name.Equals(aliasFileName, s_pathComparison)) + { + libNameAlreadyPresent = true; + break; + } + } + if (!libNameAlreadyPresent) + { + foreach (string relPath in extractedFiles) + { + if (Path.GetExtension(relPath).Equals(".dll", s_pathComparison)) + { + aliasSourceRelPath = relPath; + break; + } + } + if (aliasSourceRelPath != null) + { + extractedFiles.Add(aliasFileName); + } + } + CheckForConflicts(installDir, libName, extractedFiles, oldManifestEntries); // All checks passed. Remove the previous version's files (if any), then @@ -780,18 +817,15 @@ public static short InstallExternalLibrary( } } - // If no file in installDir matches "{libName}.*", copy the first .dll - // found as "{libName}.dll" so DllUtils.CreateDllList can discover it. - string aliasFileName = DllFileNameFor(libName); - if (Directory.GetFiles(installDir, libName + ".*").Length == 0 && - !File.Exists(Path.Combine(installDir, aliasFileName))) + // Create the alias file now that content is in place. The source + // was chosen and conflict-checked above. + if (aliasSourceRelPath != null) { - string[] dlls = Directory.GetFiles(installDir, "*.dll"); - if (dlls.Length > 0) + string aliasSrc = Path.Combine(installDir, aliasSourceRelPath); + string alias = Path.Combine(installDir, aliasFileName); + if (File.Exists(aliasSrc)) { - string alias = Path.Combine(installDir, aliasFileName); - File.Copy(dlls[0], alias, false); - extractedFiles.Add(aliasFileName); + File.Copy(aliasSrc, alias, false); } } @@ -856,6 +890,11 @@ public static short UninstallExternalLibrary( string installDir = Interop.UTF8PtrToStr(libraryInstallDirectory, (ulong)libraryInstallDirectoryLength); string libName = Interop.UTF8PtrToStr(libraryName, (ulong)libraryNameLength); + // Reject names containing path separators etc. before they are used + // to build manifestPath / libraryFile via Path.Combine. Without this, + // a malicious libName could resolve outside installDir. + ValidateLibraryName(libName); + if (Directory.Exists(installDir)) { // Check for a manifest written during install that lists @@ -912,7 +951,10 @@ private static void CopyDirectory(string sourceDir, string destDir) foreach (string file in Directory.GetFiles(sourceDir)) { string destFile = Path.Combine(destDir, Path.GetFileName(file)); - File.Copy(file, destFile, true); + // overwrite: false so that if filesystem state changed between the + // conflict check and here (TOCTOU), we fail loud rather than silently + // replacing a file belonging to another library. + File.Copy(file, destFile, false); } foreach (string dir in Directory.GetDirectories(sourceDir)) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs index 67bddb38..fd9ae68f 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs @@ -91,16 +91,12 @@ public static List CreateDllList( } else { - // Search for files matching the library name pattern (e.g. "regex.*") - if (!string.IsNullOrEmpty(privatePath)) - { - dllList.AddRange(Directory.GetFiles(privatePath, userLibName + ".*")); - } - - if (!string.IsNullOrEmpty(publicPath)) - { - dllList.AddRange(Directory.GetFiles(publicPath, userLibName + ".*")); - } + // Callers may pass either a bare library name ("regex") or an explicit + // filename ("Foo.dll"). Try the exact name first so filenames with + // extensions resolve correctly; fall back to the "{name}.*" wildcard + // for bare names. + AddMatches(privatePath, userLibName, dllList); + AddMatches(publicPath, userLibName, dllList); } if (dllList.Count == 0) @@ -111,6 +107,30 @@ public static List CreateDllList( return dllList; } + /// + /// Adds DLL matches for under + /// . Tries the exact name first (so callers + /// that pass "Foo.dll" resolve correctly) and falls back to the + /// "{name}.*" wildcard for bare names (so callers that pass "Foo" + /// still match "Foo.dll", "Foo.runtimeconfig.json", etc.). + /// + private static void AddMatches(string searchPath, string userLibName, List dllList) + { + if (string.IsNullOrEmpty(searchPath) || !Directory.Exists(searchPath)) + { + return; + } + + string exactPath = Path.Combine(searchPath, userLibName); + if (File.Exists(exactPath)) + { + dllList.Add(exactPath); + return; + } + + dllList.AddRange(Directory.GetFiles(searchPath, userLibName + ".*")); + } + /// /// This method finds the corresponding loaded dll for user dll's dependencies. /// It searches for the corresponding loaded dll that matches args.Name. diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index 1e8e584c..b3b679a1 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -1219,4 +1219,81 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + + //---------------------------------------------------------------------------------------------- + // Name: UninstallRejectsPathTraversalLibNameTest + // + // Description: + // UninstallExternalLibrary must reject libNames that contain path + // separators or traversal segments before using them to build + // manifestPath / libraryFile via Path.Combine. Without validation, + // a malicious libName like "../foo" would resolve outside installDir + // and allow unintended file reads/deletes. + // + TEST_F(CSharpExtensionApiTests, UninstallRejectsPathTraversalLibNameTest) + { + string installDir = CreateInstallDir(); + + // Create a sentinel file OUTSIDE installDir that uninstall must not touch. + fs::path sentinelDir = fs::path(installDir).parent_path(); + fs::path sentinel = sentinelDir / "do-not-delete.manifest"; + { std::ofstream os(sentinel.string()); os << "sentinel"; } + ASSERT_TRUE(fs::exists(sentinel)); + + // Attempt uninstall with a traversal libName. + SQLRETURN result = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "../do-not-delete", installDir); + EXPECT_EQ(result, SQL_ERROR) << "Uninstall must reject libName with traversal"; + EXPECT_TRUE(fs::exists(sentinel)) << "Sentinel file outside installDir was deleted"; + + fs::remove(sentinel); + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: AliasConflictDetectedBeforeExtractionTest + // + // Description: + // If library A has already installed a file "shared.dll" at the root of + // installDir, and library B (whose package contains DLLs but none named + // "shared.*") is then installed with libName "shared", the install code + // would normally create a "shared.dll" alias. That alias now collides + // with A's file. Install must fail during the conflict-check phase + // BEFORE any of B's content is written into installDir (no partial + // state left behind). + // + TEST_F(CSharpExtensionApiTests, AliasConflictDetectedBeforeExtractionTest) + { + string packagesPath = GetPackagesPath(); + string pkgA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); + + string installDir = CreateInstallDir(); + + // Install library A normally. + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "libA", pkgA, installDir), SQL_SUCCESS); + + // Plant a "shared.dll" file in installDir (simulates ownership by another library). + fs::path squatter = fs::path(installDir) / "shared.dll"; + { std::ofstream os(squatter.string()); os << "squatter"; } + ASSERT_TRUE(fs::exists(squatter)); + + // Count files currently in installDir; install of B must not add any. + size_t fileCountBefore = 0; + for (auto &p : fs::recursive_directory_iterator(installDir)) if (fs::is_regular_file(p)) ++fileCountBefore; + + // Install B with libName "shared" - it has no shared.* file, so install + // would create a "shared.dll" alias, which collides with squatter. + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "shared", pkgA, installDir); + EXPECT_EQ(result, SQL_ERROR) << "Alias conflict must be detected and install must fail"; + + // No B content should have been written. + size_t fileCountAfter = 0; + for (auto &p : fs::recursive_directory_iterator(installDir)) if (fs::is_regular_file(p)) ++fileCountAfter; + EXPECT_EQ(fileCountBefore, fileCountAfter) + << "Failed install left partial state in installDir"; + + CleanupInstallDir(installDir); + } } From e7ec844d32629a076739211bfc574b3c176dc06f Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Wed, 22 Apr 2026 00:26:03 -0700 Subject: [PATCH 11/18] Address PR #85 second review pass: raw-DLL fail-if-exists+manifest, alias root-only, native null-check, FileShare.Read, reparse-point skip, DllUtils .dll filter, comments, tests --- .../src/managed/CSharpExtension.cs | 120 +++++++++++-- .../src/managed/utils/DllUtils.cs | 8 +- .../src/native/nativecsharpextension.cpp | 18 ++ .../test/include/CSharpExtensionApiTests.h | 2 +- .../test/src/native/CSharpLibraryTests.cpp | 160 ++++++++++++++++-- 5 files changed, 272 insertions(+), 36 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index aeeccf9f..21994217 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -661,6 +661,14 @@ public delegate short InstallExternalLibraryDelegate( /// that zip is extracted to the install directory. Otherwise, all files /// are copied directly. /// + /// + /// NOTE: Unlike most other extension APIs, Install/Uninstall report + /// errors via the libraryError out-parameter rather than through + /// ExceptionUtils.WrapError. This matches the sqlexternallibrary.h + /// contract: the host expects the error string to come back through + /// libraryError / libraryErrorLength so it can surface it as part of + /// CREATE/ALTER/DROP EXTERNAL LIBRARY diagnostics. + /// /// /// SQL_SUCCESS(0), SQL_ERROR(-1) /// @@ -700,19 +708,47 @@ public static short InstallExternalLibrary( { // Raw DLL. Clean up any previous manifest-based install of the same // library, then copy the file as "{libName}.dll" so DllUtils can find it. + // Match the pre-PR ExtHost CopyFileW(..., bFailIfExists=TRUE) contract: + // if "{libName}.dll" already exists at the target path AND it is NOT + // owned by this library (i.e. not listed in our manifest), fail rather + // than silently overwrite a file that may belong to another library. + string dllFileName = DllFileNameFor(libName); + string installedDllPath = Path.Combine(installDir, dllFileName); + if (File.Exists(manifestPath)) { + // Prior manifest-based install of the SAME library: clean it up + // first so the upcoming copy has a free slot. CleanupManifest(manifestPath, installDir); } + else if (File.Exists(installedDllPath)) + { + // No manifest: we don't own this file. Fail rather than overwrite. + throw new IOException( + $"Cannot install library '{libName}': file '{dllFileName}' " + + "already exists in the install directory and is not owned by this library."); + } + + File.Copy(libFilePath, installedDllPath, false); + + // Track the raw-DLL install in a manifest too. This is what makes + // ALTER from raw-DLL to ZIP work: the ZIP path's CheckForConflicts + // sees "{libName}.dll" in oldManifestEntries and treats it as + // owned-by-previous, allowing the upgrade to proceed cleanly. + File.WriteAllLines(manifestPath, new[] { dllFileName }); - File.Copy(libFilePath, Path.Combine(installDir, DllFileNameFor(libName)), true); return SQL_SUCCESS; } // ZIP path. Stage the new content to a temp folder, validate it, then - // clean up the old version and move the new files into place. This keeps - // ALTER EXTERNAL LIBRARY transactional: a corrupt ZIP leaves the existing - // install intact. + // clean up the old version and move the new files into place. A corrupt + // ZIP leaves the existing install intact (validation runs against the + // staged copy in tempFolder, before we touch installDir). + // + // NOTE: On-disk replacement is NOT atomic at the per-file level. A + // crash between CleanupManifest and the CopyDirectory loop can leave + // the install directory inconsistent. We rely on SQL Server's catalog- + // based recovery: the next session re-installs from the catalog. tempFolder = Path.Combine(installDir, Guid.NewGuid().ToString()); ZipFile.ExtractToDirectory(libFilePath, tempFolder); @@ -723,6 +759,12 @@ public static short InstallExternalLibrary( "The library archive contains no entries."); } + // If the outer ZIP contains exactly one inner .zip at its top level, + // treat it as the real package and extract that instead. This matches + // the way the SQL Server engine wraps user-supplied archives. We pick + // the FIRST .zip found; multiple inner zips are unsupported and the + // remainder are extracted as opaque files. (Callers that need to ship + // multiple zips should pack them inside subdirectories.) string innerZipPath = null; foreach (string file in Directory.GetFiles(tempFolder)) { @@ -756,27 +798,39 @@ public static short InstallExternalLibrary( } // Determine whether we need a "{libName}.dll" alias so - // DllUtils.CreateDllList can discover the library. Examine the - // entire extracted tree (not just the top level) so packages - // laid out under e.g. lib/... do not generate a redundant alias - // when a DLL named "{libName}.dll" (or "{libName}.*") is already - // present deeper in the tree. If an alias IS needed, include it - // in extractedFiles now so CheckForConflicts fails BEFORE we - // start writing to installDir. + // DllUtils.CreateDllList can discover the library. + // + // DllUtils.CreateDllList searches only the TOP LEVEL of the + // library path (non-recursive). So a deeply-nested DLL like + // "lib/net8.0/{libName}.dll" does NOT make the library + // discoverable, and we still need to create a root-level alias. + // Only a root-level "{libName}.*" suppresses alias creation. + // + // If an alias IS needed, append it to extractedFiles BEFORE + // CheckForConflicts runs so any "{libName}.dll" collision with + // another library fails fast with no content written to + // installDir. string aliasFileName = DllFileNameFor(libName); string aliasSourceRelPath = null; - bool libNameAlreadyPresent = false; + bool libNameAlreadyPresentAtRoot = false; foreach (string relPath in extractedFiles) { + bool isRootLevel = + relPath.IndexOf('/') < 0 && + relPath.IndexOf('\\') < 0; + if (!isRootLevel) + { + continue; + } string name = Path.GetFileName(relPath); if (name.StartsWith(libName + ".", s_pathComparison) || name.Equals(aliasFileName, s_pathComparison)) { - libNameAlreadyPresent = true; + libNameAlreadyPresentAtRoot = true; break; } } - if (!libNameAlreadyPresent) + if (!libNameAlreadyPresentAtRoot) { foreach (string relPath in extractedFiles) { @@ -869,6 +923,11 @@ public delegate short UninstallExternalLibraryDelegate( /// This method implements UninstallExternalLibrary API. /// Uninstalls an external library from the specified directory. /// + /// + /// See InstallExternalLibrary remarks: errors are reported via the + /// libraryError out-parameter rather than ExceptionUtils.WrapError, + /// per the sqlexternallibrary.h contract. + /// /// /// SQL_SUCCESS(0), SQL_ERROR(-1) /// @@ -937,6 +996,8 @@ private static void SetLibraryError(string errorMessage, char **libraryError, in Marshal.Copy(errorBytes, 0, errorPtr, errorBytes.Length); ((byte*)errorPtr)[errorBytes.Length] = 0; *libraryError = (char*)errorPtr; + // Length excludes the null terminator -- ExtHost adds +1 when + // copying via Utf8ToNullTerminatedUtf16Le / strcpy_s. *libraryErrorLength = errorBytes.Length; } } @@ -950,6 +1011,17 @@ private static void CopyDirectory(string sourceDir, string destDir) foreach (string file in Directory.GetFiles(sourceDir)) { + // Skip reparse points (symbolic links, junctions). On Linux a + // ZIP archive can carry symlinks that survive extraction; left + // unchecked, File.Copy would follow the symlink and either + // copy a file from outside installDir into it (information + // leak) or, when paired with the directory recursion below, + // open up out-of-tree writes. + FileAttributes fileAttrs = File.GetAttributes(file); + if ((fileAttrs & FileAttributes.ReparsePoint) != 0) + { + continue; + } string destFile = Path.Combine(destDir, Path.GetFileName(file)); // overwrite: false so that if filesystem state changed between the // conflict check and here (TOCTOU), we fail loud rather than silently @@ -959,6 +1031,14 @@ private static void CopyDirectory(string sourceDir, string destDir) foreach (string dir in Directory.GetDirectories(sourceDir)) { + // Skip reparse-point directories for the same reason as above: + // following a junction/symlink could cause us to recurse out + // of the staged temp folder. + FileAttributes dirAttrs = File.GetAttributes(dir); + if ((dirAttrs & FileAttributes.ReparsePoint) != 0) + { + continue; + } string destSubDir = Path.Combine(destDir, Path.GetFileName(dir)); CopyDirectory(dir, destSubDir); } @@ -1027,7 +1107,10 @@ private static void ValidateLibraryName(string libName) // A ZIP file of any variant begins with the 'PK' magic bytes (0x50 0x4B). private static bool IsZipFile(string path) { - using (var fs = new FileStream(path, FileMode.Open, FileAccess.Read)) + // FileShare.Read so we don't fight with anti-virus scanners, + // backup agents, or another concurrent SQL session that may have + // the source file open for read. + using (var fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read)) { byte[] magic = new byte[2]; return fs.Read(magic, 0, 2) == 2 && magic[0] == 0x50 && magic[1] == 0x4B; @@ -1116,8 +1199,13 @@ private static void CleanupManifest(string manifestPath, string installDir) { fullPath = Path.GetFullPath(Path.Combine(fullInstall, relPath)); } - catch + catch (Exception ex) { + // A malformed manifest entry shouldn't abort the rest of + // the cleanup, but it must leave a diagnostic trail so + // orphaned files don't disappear silently. + Logging.Trace( + $"CleanupManifest: skipping manifest entry '{relPath}': {ex.Message}"); continue; } diff --git a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs index fd9ae68f..110b96b4 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs @@ -128,7 +128,13 @@ private static void AddMatches(string searchPath, string userLibName, List f.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))); } /// diff --git a/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp b/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp index 2c86af43..20e09d37 100644 --- a/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp +++ b/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp @@ -353,13 +353,31 @@ static void SetLibraryError( SQLCHAR **libraryError, SQLINTEGER *libraryErrorLength) { + // Guard against null out-parameters. The managed SetLibraryError + // (CSharpExtension.cs) does the same null-check; without it, a caller + // passing null libraryError / libraryErrorLength would dereference null + // and crash the host process. + if (libraryError == nullptr || libraryErrorLength == nullptr) + { + return; + } + if (!errorString.empty()) { + // Length excludes null terminator -- ExtHost adds +1 when copying + // (see Utf8ToNullTerminatedUtf16Le / strcpy_s in the host). *libraryErrorLength = static_cast(errorString.length()); std::string *pError = new std::string(errorString); *libraryError = const_cast( reinterpret_cast(pError->c_str())); } + else + { + // Explicitly clear the out-parameters so callers that don't + // pre-initialize them see a well-defined "no error" state. + *libraryError = nullptr; + *libraryErrorLength = 0; + } } //-------------------------------------------------------------------------------------------------- diff --git a/language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h b/language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h index 1e450a9c..c99e537c 100644 --- a/language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h +++ b/language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h @@ -361,7 +361,7 @@ namespace ExtensionApiTest // User library name and class full name // The name of the library is same as the dll file name. // - const std::string m_UserLibName = "Microsoft.SqlServer.CSharpExtensionTest.dll";; + const std::string m_UserLibName = "Microsoft.SqlServer.CSharpExtensionTest.dll"; const std::string m_UserClassFullName = "Microsoft.SqlServer.CSharpExtensionTest.CSharpTestExecutor"; const std::string m_Separator = ";"; diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index b3b679a1..247d7032 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -215,8 +215,14 @@ namespace ExtensionApiTest "testpackageA", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - EXPECT_TRUE(DirectoryHasFiles(installDir)) - << "No files found in install directory after installation"; + // Assert on the specific files we expect from the inner-zip + // contents rather than just "directory is non-empty" -- the weaker + // form passes even if the install extracts the wrong package or + // leaves stale files behind from a prior test run. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageA.dll")) + << "Expected testpackageA.dll not extracted from inner zip"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageA.txt")) + << "Expected testpackageA.txt not extracted from inner zip"; CleanupInstallDir(installDir); } @@ -372,6 +378,17 @@ namespace ExtensionApiTest } EXPECT_TRUE(hasDll) << "DLL from second package not found after reinstall"; + // Also verify the FIRST package's unique files were removed by the + // ALTER cleanup. Without these checks the test passes even if the + // manifest-based cleanup is broken (the second install just adds + // its own files alongside the stale ones). + EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.dll")) + << "Stale testpackageA.dll left behind after reinstall"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.txt")) + << "Stale testpackageA.txt left behind after reinstall"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")) + << "Expected testpackageB.dll not present after reinstall"; + CleanupInstallDir(installDir); } @@ -987,9 +1004,9 @@ namespace ExtensionApiTest // Name: UninstallNonZipLibraryTest // // Description: - // When a raw DLL (non-ZIP) is installed, no manifest is written. Uninstall - // must still remove the library file by its libName.dll path. - // Exercises the File.Delete(libraryFile) branch in UninstallExternalLibrary. + // Raw-DLL (non-ZIP) installs write a single-entry manifest tracking + // "{libName}.dll" so that ALTER from raw->ZIP can clean up the prior + // install. Uninstall must remove both the file and its manifest. // TEST_F(CSharpExtensionApiTests, UninstallNonZipLibraryTest) { @@ -999,19 +1016,22 @@ namespace ExtensionApiTest string installDir = CreateInstallDir(); - // Install the raw DLL as "rawlib" — no manifest should be written. + // Install the raw DLL as "rawlib" — manifest must be written so that + // ALTER scenarios can track ownership of the {libName}.dll file. ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, "rawlib", rawDll, installDir), SQL_SUCCESS); ASSERT_TRUE(fs::exists(fs::path(installDir) / "rawlib.dll")); - ASSERT_FALSE(fs::exists(fs::path(installDir) / "rawlib.manifest")) - << "Raw-DLL install should not write a manifest"; + ASSERT_TRUE(fs::exists(fs::path(installDir) / "rawlib.manifest")) + << "Raw-DLL install should write a single-entry manifest"; - // Uninstall must still delete the libName.dll file. + // Uninstall must delete both the file and the manifest. SQLRETURN r = CallUninstall(sm_uninstallExternalLibraryFuncPtr, "rawlib", installDir); EXPECT_EQ(r, SQL_SUCCESS); EXPECT_FALSE(fs::exists(fs::path(installDir) / "rawlib.dll")) << "Raw library file not removed by uninstall"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / "rawlib.manifest")) + << "Raw-DLL manifest file not removed by uninstall"; CleanupInstallDir(installDir); } @@ -1095,10 +1115,12 @@ namespace ExtensionApiTest // Name: AlterFromNonZipToZipTest // // Description: - // ALTER EXTERNAL LIBRARY scenario where v1 was a raw DLL (no manifest) - // and v2 is a ZIP package. Install code must handle the missing-manifest - // case gracefully and complete the new install. The old lib file remains - // until explicit uninstall — that matches sicongliu's baseline semantics. + // ALTER EXTERNAL LIBRARY scenario where v1 was a raw DLL and v2 is a + // ZIP package. Raw-DLL installs now write a manifest with one entry + // ("{libName}.dll"), so the v2 ZIP install can detect and clean up the + // v1 file before extracting. Without that manifest tracking, the ZIP + // install would either silently overwrite v1's DLL or trip the alias + // conflict check on the pre-existing "{libName}.dll". // TEST_F(CSharpExtensionApiTests, AlterFromNonZipToZipTest) { @@ -1108,25 +1130,80 @@ namespace ExtensionApiTest string installDir = CreateInstallDir(); - // v1: raw DLL install (no manifest created). + // v1: raw DLL install. The new behavior writes a manifest tracking + // the single "{libName}.dll" entry, so ALTER can clean it up below. ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, "myLib", rawDll, installDir), SQL_SUCCESS); ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.dll")); - ASSERT_FALSE(fs::exists(fs::path(installDir) / "myLib.manifest")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")) + << "Raw-DLL install should write a manifest so ALTER can track it"; - // v2: ZIP install of the same libName — must NOT crash on missing manifest. + // v2: ZIP install of the same libName. The pre-existing myLib.dll + // must NOT trip the alias conflict check -- it is tracked in v1's + // manifest as owned-by-previous and gets cleaned up first. SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, "myLib", pkgB, installDir); EXPECT_EQ(r, SQL_SUCCESS) - << "ALTER from non-ZIP to ZIP should succeed even without prior manifest"; + << "ALTER from non-ZIP to ZIP should succeed"; - // v2's files must be present + v2 manifest must exist. + // v1's raw DLL should be gone (the alias for v2 may or may not be + // named myLib.dll depending on whether v2 contains testpackageB.dll + // and needs an alias). v2's content must be present. EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); EXPECT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); CleanupInstallDir(installDir); } + //---------------------------------------------------------------------------------------------- + // Name: AlterFromZipToNonZipTest + // + // Description: + // ALTER EXTERNAL LIBRARY scenario where v1 was a ZIP package and v2 is + // a raw DLL with the same libName. The v1 manifest must be cleaned up + // before v2 writes "{libName}.dll", and v2 must end up with its own + // one-entry manifest. This is the inverse of AlterFromNonZipToZipTest. + // + TEST_F(CSharpExtensionApiTests, AlterFromZipToNonZipTest) + { + string packagesPath = GetPackagesPath(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + string rawDll = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + + string installDir = CreateInstallDir(); + + // v1: install ZIP package B as "myLib". This drops testpackageB.dll, + // testpackageB.deps.json, an alias myLib.dll, and a myLib.manifest. + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", pkgB, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); + + // v2: raw-DLL install of the same libName. + SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", rawDll, installDir); + EXPECT_EQ(r, SQL_SUCCESS) + << "ALTER from ZIP to non-ZIP should succeed"; + + // v1's ZIP-only files must be gone (cleaned up via v1's manifest + // before v2 was written). + EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageB.deps.json")) + << "Stale v1 deps.json left behind after ALTER zip->raw"; + + // v2's file must be present and its manifest must list exactly that file. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "myLib.dll")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); + vector entries = ReadManifest( + (fs::path(installDir) / "myLib.manifest").string()); + EXPECT_EQ(entries.size(), 1u) << "Raw-DLL manifest should list exactly one entry"; + if (!entries.empty()) + { + EXPECT_EQ(entries[0], "myLib.dll"); + } + + CleanupInstallDir(installDir); + } + //---------------------------------------------------------------------------------------------- // Name: AliasFileRemovedOnUninstallTest // @@ -1296,4 +1373,51 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + + //---------------------------------------------------------------------------------------------- + // Name: RawDllInstallFailsIfForeignFileExistsTest + // + // Description: + // Restores the pre-PR ExtHost CopyFileW(..., bFailIfExists=TRUE) + // contract for raw-DLL installs. If "{libName}.dll" already exists in + // installDir AND we cannot prove ownership via this library's manifest + // (e.g. another library or an external process planted the file), + // install must FAIL rather than silently overwrite. This prevents one + // library from clobbering another's content in a shared install dir. + // + TEST_F(CSharpExtensionApiTests, RawDllInstallFailsIfForeignFileExistsTest) + { + string packagesPath = GetPackagesPath(); + string rawDll = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + ASSERT_TRUE(fs::exists(rawDll)); + + string installDir = CreateInstallDir(); + + // Plant a foreign file at the target path with no matching manifest + // -- simulates ownership by another library or external tooling. + fs::path foreign = fs::path(installDir) / "owned.dll"; + { std::ofstream os(foreign.string()); os << "FOREIGN-CONTENT"; } + ASSERT_TRUE(fs::exists(foreign)); + + // Install must FAIL rather than overwrite the foreign file. + SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, + "owned", rawDll, installDir); + EXPECT_EQ(r, SQL_ERROR) + << "Raw-DLL install must fail when {libName}.dll already exists " + "and is not owned by this library"; + + // Foreign file content must be byte-for-byte unchanged. + ASSERT_TRUE(fs::exists(foreign)); + std::ifstream ifs(foreign.string()); + std::string contents((std::istreambuf_iterator(ifs)), + std::istreambuf_iterator()); + EXPECT_EQ(contents, "FOREIGN-CONTENT") + << "Foreign file was overwritten by failed raw-DLL install"; + + // No manifest should have been written for the failed install. + EXPECT_FALSE(fs::exists(fs::path(installDir) / "owned.manifest")) + << "Manifest leaked from a failed raw-DLL install"; + + CleanupInstallDir(installDir); + } } From 38c553d1fc0f92de49acfc4b42658e1d359e0a24 Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Tue, 28 Apr 2026 08:54:03 -0700 Subject: [PATCH 12/18] Address PR #85 yaelh review: concurrent-install lock, empty-dirs ZIP guard, reparse-point sweep, libName extension-only rejection, refactor InstallExternalLibrary into helpers, doc + style sweep, 7 new tests Block 1 substantive fixes: - C1: AcquireInstallLock serializes Install + Uninstall on the same installDir via {installDir}/.install.lock (FileShare.None, DeleteOnClose, 100ms retry, blocks forever) - C3: ValidateLibraryName rejects extension-only names (.dll, .txt, etc.) via Path.GetFileNameWithoutExtension - E1: extractedFiles.Count==0 guard after collection prevents silent ALTER data loss when ZIP contains only directory entries - E2: SAFETY comment on inner-zip ExtractToDirectory documenting why direct-to-installDir is safe today + tripwire for future .NET versions - E3: New IsReparsePoint helper; root-level loops in Install + CollectRelativeFiles + CopyDirectory all guarded Block 2 test fixes: - E4: 5 new tests (AlterFromNonZipToNonZipTest, InstallRejectsInvalidLibNameTest, InstallZipWithDllSuffixedLibNameTest, UninstallWithMissingInstallDirTest, UninstallPreservesSharedNestedDirsTest) + new fixture testpackageJ-NESTED2.zip - E5/E6/E7/E8/E10/E11: stronger assertions, removed redundant loops, exact equality, byte-for-byte content checks - E12: surfaced + fixed latent test bug (dllCount expected 50 but real value with alias is 51) - E13: InstallInvalidZipTest renamed to InstallNonZipFileAsRawDllTest Block 3 refactor + style + docs: - C6: InstallExternalLibrary split into orchestrator + 6 helpers (InstallRawDll, InstallZipPackage, FindInnerZip, CollectStagedFiles, DetermineAliasSource, ExtractContentToInstallDir, CreateAlias) - A1-A8: empty lines before comments, single return, no var, brace style, scoped blocks unwrapped - A5: s_pathComparer/s_pathComparison/s_lockRetryDelayMs moved to top class members - B1-B5: full XML doc comments with blocks; expanded IsZipFile + ValidateRelativePath comments - C4: Install summary clarifies ZIP is allowed (not expected); raw DLL still supported - C5: Logging.Trace -> Logging.Error in CleanupManifest skip path - D1: -> - D2: DirectoryHasFiles -> DoesDirectoryHaveFiles - D4: Logging.Trace after manifest write (both raw-DLL and ZIP paths) --- .../src/managed/CSharpExtension.cs | 893 +++++++++++++----- .../src/managed/utils/DllUtils.cs | 14 + .../test/src/native/CSharpLibraryTests.cpp | 546 +++++++++-- .../test_packages/testpackageI-EMPTYDIRS.zip | Bin 0 -> 408 bytes .../test_packages/testpackageJ-NESTED2.zip | Bin 0 -> 331 bytes 5 files changed, 1166 insertions(+), 287 deletions(-) create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageI-EMPTYDIRS.zip create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageJ-NESTED2.zip diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 21994217..571db929 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -15,6 +15,7 @@ using System.Runtime.CompilerServices; using System.Collections.Generic; using System.Runtime.InteropServices; +using System.Threading; using static Microsoft.SqlServer.CSharpExtension.Sql; namespace Microsoft.SqlServer.CSharpExtension @@ -49,6 +50,28 @@ public static unsafe class CSharpExtension /// private static string _languageParams; + /// + /// Case-sensitivity comparer matching the host OS's filesystem + /// semantics. Used for set keys that contain on-disk file paths. + /// + private static readonly StringComparer s_pathComparer = + OperatingSystem.IsWindows() ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal; + + /// + /// Case-sensitivity rule matching the host OS's filesystem semantics. + /// Used for string-comparison APIs (StartsWith / EndsWith / Equals) + /// that operate on on-disk file paths. + /// + private static readonly StringComparison s_pathComparison = + OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + + /// + /// Sleep interval (in milliseconds) between attempts to acquire the + /// per-installDir install lock when another process holds it. See + /// . + /// + private const int s_lockRetryDelayMs = 100; + /// /// This delegate declares the delegate type of Init. /// @@ -657,9 +680,12 @@ public delegate short InstallExternalLibraryDelegate( /// /// This method implements InstallExternalLibrary API. /// Installs an external library to the specified directory. - /// The library file is expected to be a zip. If it contains an inner zip, - /// that zip is extracted to the install directory. Otherwise, all files - /// are copied directly. + /// The library file MAY be a ZIP archive (with optional inner ZIP and + /// arbitrary nested file tree) OR a raw DLL. Raw-DLL support matches + /// the pre-PR ExtHost behavior: the file is copied as "{libName}.dll". + /// ZIP support is the new capability added by this PR; it allows + /// libraries to ship multiple DLLs, runtime config, or supporting + /// files in a single archive. /// /// /// NOTE: Unlike most other extension APIs, Install/Uninstall report @@ -669,6 +695,43 @@ public delegate short InstallExternalLibraryDelegate( /// libraryError / libraryErrorLength so it can surface it as part of /// CREATE/ALTER/DROP EXTERNAL LIBRARY diagnostics. /// + /// + /// Session GUID supplied by ExtHost. Currently unused by the CSharp + /// extension; logged downstream for trace correlation only. + /// + /// + /// UTF-8 buffer holding the library name from CREATE EXTERNAL LIBRARY. + /// NOT null-terminated -- read exactly libraryNameLength bytes. + /// + /// + /// Byte length of . + /// + /// + /// UTF-8 buffer holding the absolute path to the library content file + /// (a raw DLL or a ZIP archive). NOT null-terminated -- read exactly + /// libraryFileLength bytes. + /// + /// + /// Byte length of . + /// + /// + /// UTF-8 buffer holding the absolute path to the install directory. + /// May be the public or private external library path. Created if + /// it doesn't exist. NOT null-terminated. + /// + /// + /// Byte length of . + /// + /// + /// On failure, set to a freshly-allocated UTF-8 error string for + /// ExtHost to surface to the user. ExtHost takes ownership of the + /// allocation. On success, set to nullptr. + /// + /// + /// On failure, set to the byte length of + /// (excluding the null terminator -- ExtHost adds +1 when copying). + /// On success, set to 0. + /// /// /// SQL_SUCCESS(0), SQL_ERROR(-1) /// @@ -696,215 +759,360 @@ public static short InstallExternalLibrary( ValidateLibraryName(libName); - if (!Directory.Exists(installDir)) + // Serialize against any other concurrent Install/Uninstall on + // the same installDir. See AcquireInstallLock remarks for why: + // without serialization, two installs can both pass + // CheckForConflicts against pre-cleanup state, then one's + // CleanupManifest destroys its previous version while the + // other's File.Copy collides on overwrite:false -- silent + // data loss for one of them. + using (FileStream installLock = AcquireInstallLock(installDir)) { - Directory.CreateDirectory(installDir); - } + if (!Directory.Exists(installDir)) + { + Directory.CreateDirectory(installDir); + } - string manifestPath = Path.Combine(installDir, libName + ".manifest"); - HashSet oldManifestEntries = ReadManifestEntries(manifestPath); + string manifestPath = Path.Combine(installDir, libName + ".manifest"); + HashSet oldManifestEntries = ReadManifestEntries(manifestPath); - if (!IsZipFile(libFilePath)) - { - // Raw DLL. Clean up any previous manifest-based install of the same - // library, then copy the file as "{libName}.dll" so DllUtils can find it. - // Match the pre-PR ExtHost CopyFileW(..., bFailIfExists=TRUE) contract: - // if "{libName}.dll" already exists at the target path AND it is NOT - // owned by this library (i.e. not listed in our manifest), fail rather - // than silently overwrite a file that may belong to another library. - string dllFileName = DllFileNameFor(libName); - string installedDllPath = Path.Combine(installDir, dllFileName); - - if (File.Exists(manifestPath)) + if (IsZipFile(libFilePath)) { - // Prior manifest-based install of the SAME library: clean it up - // first so the upcoming copy has a free slot. - CleanupManifest(manifestPath, installDir); + tempFolder = InstallZipPackage(libFilePath, installDir, libName, + manifestPath, oldManifestEntries); } - else if (File.Exists(installedDllPath)) + else { - // No manifest: we don't own this file. Fail rather than overwrite. - throw new IOException( - $"Cannot install library '{libName}': file '{dllFileName}' " + - "already exists in the install directory and is not owned by this library."); + InstallRawDll(libFilePath, installDir, libName, manifestPath); } + } + } + catch (Exception e) + { + string stackTracePart = string.IsNullOrEmpty(e.StackTrace) ? string.Empty : e.StackTrace + Environment.NewLine; + Logging.Error(stackTracePart + "Error: " + e.Message); + SetLibraryError(e.Message, libraryError, libraryErrorLength); + result = SQL_ERROR; + } + finally + { + if (tempFolder != null && Directory.Exists(tempFolder)) + { + try { Directory.Delete(tempFolder, true); } + catch { /* best-effort */ } + } + } - File.Copy(libFilePath, installedDllPath, false); + return result; + } - // Track the raw-DLL install in a manifest too. This is what makes - // ALTER from raw-DLL to ZIP work: the ZIP path's CheckForConflicts - // sees "{libName}.dll" in oldManifestEntries and treats it as - // owned-by-previous, allowing the upgrade to proceed cleanly. - File.WriteAllLines(manifestPath, new[] { dllFileName }); + /// + /// Installs a raw DLL (non-ZIP) library file. Copies the file as + /// "{libName}.dll" into and writes a + /// single-entry manifest so future ALTER calls can clean it up. + /// + /// + /// Matches the pre-PR ExtHost CopyFileW(..., bFailIfExists=TRUE) + /// contract: if "{libName}.dll" already exists at the target path AND + /// it is NOT owned by this library (i.e. not listed in our manifest), + /// throws rather than silently overwriting a file that may belong to + /// another library. + /// + private static void InstallRawDll( + string libFilePath, + string installDir, + string libName, + string manifestPath) + { + string dllFileName = DllFileNameFor(libName); + string installedDllPath = Path.Combine(installDir, dllFileName); - return SQL_SUCCESS; - } + if (File.Exists(manifestPath)) + { + // Prior manifest-based install of the SAME library: clean it up + // first so the upcoming copy has a free slot. + CleanupManifest(manifestPath, installDir); + } + else if (File.Exists(installedDllPath)) + { + // No manifest: we don't own this file. Fail rather than overwrite. + throw new IOException( + $"Cannot install library '{libName}': file '{dllFileName}' " + + "already exists in the install directory and is not owned by this library."); + } - // ZIP path. Stage the new content to a temp folder, validate it, then - // clean up the old version and move the new files into place. A corrupt - // ZIP leaves the existing install intact (validation runs against the - // staged copy in tempFolder, before we touch installDir). - // - // NOTE: On-disk replacement is NOT atomic at the per-file level. A - // crash between CleanupManifest and the CopyDirectory loop can leave - // the install directory inconsistent. We rely on SQL Server's catalog- - // based recovery: the next session re-installs from the catalog. - tempFolder = Path.Combine(installDir, Guid.NewGuid().ToString()); - ZipFile.ExtractToDirectory(libFilePath, tempFolder); - - if (Directory.GetFiles(tempFolder).Length == 0 && - Directory.GetDirectories(tempFolder).Length == 0) - { - throw new InvalidOperationException( - "The library archive contains no entries."); - } + File.Copy(libFilePath, installedDllPath, false); - // If the outer ZIP contains exactly one inner .zip at its top level, - // treat it as the real package and extract that instead. This matches - // the way the SQL Server engine wraps user-supplied archives. We pick - // the FIRST .zip found; multiple inner zips are unsupported and the - // remainder are extracted as opaque files. (Callers that need to ship - // multiple zips should pack them inside subdirectories.) - string innerZipPath = null; - foreach (string file in Directory.GetFiles(tempFolder)) - { - if (Path.GetExtension(file).Equals(".zip", StringComparison.OrdinalIgnoreCase)) - { - innerZipPath = file; - break; - } - } + // Track the raw-DLL install in a manifest too. This is what makes + // ALTER from raw-DLL to ZIP work: the ZIP path's CheckForConflicts + // sees "{libName}.dll" in oldManifestEntries and treats it as + // owned-by-previous, allowing the upgrade to proceed cleanly. + File.WriteAllLines(manifestPath, new[] { dllFileName }); + Logging.Trace( + $"Wrote manifest: {manifestPath} with 1 entry (raw-DLL install)"); + } - // Collect relative paths we're about to install, validating each path - // stays under installDir (defense-in-depth zip-slip check on top of - // ZipFile.ExtractToDirectory's own validation). - var extractedFiles = new List(); - if (innerZipPath != null) - { - using (var innerArchive = ZipFile.OpenRead(innerZipPath)) - { - foreach (var entry in innerArchive.Entries) - { - if (string.IsNullOrEmpty(entry.Name)) - continue; + /// + /// Installs a ZIP-archive library. Stages the new content to a temp + /// folder, validates it (zip-slip, empty-archive, conflict checks), + /// then cleans up the previous version and copies the new content + /// into . + /// + /// + /// The path of the temp folder used for staging, so the caller can + /// clean it up in its outer finally regardless of whether the + /// install succeeded or threw. + /// + /// + /// A corrupt ZIP leaves the existing install intact (validation runs + /// against the staged copy in tempFolder, before we touch installDir). + /// On-disk replacement is NOT atomic at the per-file level. A crash + /// between CleanupManifest and the copy phase can leave the install + /// directory inconsistent; SQL Server's catalog-based recovery + /// re-installs from the catalog on the next session. + /// + private static string InstallZipPackage( + string libFilePath, + string installDir, + string libName, + string manifestPath, + HashSet oldManifestEntries) + { + string tempFolder = Path.Combine(installDir, Guid.NewGuid().ToString()); + ZipFile.ExtractToDirectory(libFilePath, tempFolder); - extractedFiles.Add(ValidateRelativePath(installDir, entry.FullName)); - } - } - } - else - { - CollectRelativeFiles(tempFolder, "", extractedFiles); - } + if (Directory.GetFiles(tempFolder).Length == 0 && + Directory.GetDirectories(tempFolder).Length == 0) + { + throw new InvalidOperationException( + "The library archive contains no entries."); + } - // Determine whether we need a "{libName}.dll" alias so - // DllUtils.CreateDllList can discover the library. - // - // DllUtils.CreateDllList searches only the TOP LEVEL of the - // library path (non-recursive). So a deeply-nested DLL like - // "lib/net8.0/{libName}.dll" does NOT make the library - // discoverable, and we still need to create a root-level alias. - // Only a root-level "{libName}.*" suppresses alias creation. - // - // If an alias IS needed, append it to extractedFiles BEFORE - // CheckForConflicts runs so any "{libName}.dll" collision with - // another library fails fast with no content written to - // installDir. - string aliasFileName = DllFileNameFor(libName); - string aliasSourceRelPath = null; - bool libNameAlreadyPresentAtRoot = false; - foreach (string relPath in extractedFiles) + string innerZipPath = FindInnerZip(tempFolder); + List extractedFiles = CollectStagedFiles(installDir, tempFolder, innerZipPath); + + // Reject archives that contain only empty directories (no files). + // The earlier "no entries" guard checks for an entirely-empty + // tempFolder, but a ZIP whose entries are all directory markers + // (e.g. "lib/", "lib/net8.0/") passes that check while leaving + // CollectStagedFiles with zero file entries. Without this guard, + // ALTER would silently destroy the previous version: CleanupManifest + // deletes its content, nothing is copied (extractedFiles is empty), + // and the manifest write is skipped. The library would end up GONE + // with no replacement and no manifest tracking what was lost. + if (extractedFiles.Count == 0) + { + throw new InvalidOperationException( + "The library archive contains no files."); + } + + string aliasFileName = DllFileNameFor(libName); + string aliasSourceRelPath = DetermineAliasSource(libName, aliasFileName, extractedFiles); + if (aliasSourceRelPath != null) + { + // Append the alias to extractedFiles BEFORE CheckForConflicts + // so any "{libName}.dll" collision with another library fails + // fast with no content written to installDir. + extractedFiles.Add(aliasFileName); + } + + CheckForConflicts(installDir, libName, extractedFiles, oldManifestEntries); + + // All checks passed. Remove the previous version's files (if any), + // then extract / copy the new content into installDir. + if (File.Exists(manifestPath)) + { + CleanupManifest(manifestPath, installDir); + } + + ExtractContentToInstallDir(installDir, tempFolder, innerZipPath); + + if (aliasSourceRelPath != null) + { + CreateAlias(installDir, aliasSourceRelPath, aliasFileName); + } + + File.WriteAllLines(manifestPath, extractedFiles); + Logging.Trace( + $"Wrote manifest: {manifestPath} with {extractedFiles.Count} entries"); + + return tempFolder; + } + + /// + /// Finds the FIRST top-level ".zip" entry inside the staged + /// , or null if none is present. + /// + /// + /// If the outer ZIP contains exactly one inner .zip at its top level, + /// it is treated as the real package and extracted in place of the + /// outer. This matches the way the SQL Server engine wraps + /// user-supplied archives. Multiple inner zips are unsupported; any + /// after the first are extracted as opaque files (callers that need + /// to ship multiple zips should pack them inside subdirectories). + /// + private static string FindInnerZip(string tempFolder) + { + foreach (string file in Directory.GetFiles(tempFolder)) + { + if (Path.GetExtension(file).Equals(".zip", StringComparison.OrdinalIgnoreCase)) { - bool isRootLevel = - relPath.IndexOf('/') < 0 && - relPath.IndexOf('\\') < 0; - if (!isRootLevel) - { - continue; - } - string name = Path.GetFileName(relPath); - if (name.StartsWith(libName + ".", s_pathComparison) || - name.Equals(aliasFileName, s_pathComparison)) - { - libNameAlreadyPresentAtRoot = true; - break; - } + return file; } - if (!libNameAlreadyPresentAtRoot) + } + return null; + } + + /// + /// Builds the list of relative paths that will be installed into + /// , validating each path stays under + /// installDir (defense-in-depth zip-slip check on top of + /// ZipFile.ExtractToDirectory's own validation). + /// + private static List CollectStagedFiles( + string installDir, + string tempFolder, + string innerZipPath) + { + List extractedFiles = new List(); + if (innerZipPath != null) + { + using (ZipArchive innerArchive = ZipFile.OpenRead(innerZipPath)) { - foreach (string relPath in extractedFiles) + foreach (ZipArchiveEntry entry in innerArchive.Entries) { - if (Path.GetExtension(relPath).Equals(".dll", s_pathComparison)) + if (string.IsNullOrEmpty(entry.Name)) { - aliasSourceRelPath = relPath; - break; + // Directory entry (no filename portion). Skip. + continue; } - } - if (aliasSourceRelPath != null) - { - extractedFiles.Add(aliasFileName); + extractedFiles.Add(ValidateRelativePath(installDir, entry.FullName)); } } + } + else + { + CollectRelativeFiles(tempFolder, "", extractedFiles); + } + return extractedFiles; + } - CheckForConflicts(installDir, libName, extractedFiles, oldManifestEntries); - - // All checks passed. Remove the previous version's files (if any), then - // extract/copy the new content into installDir. - if (File.Exists(manifestPath)) + /// + /// Decides which extracted file (if any) should be cloned as the + /// "{libName}.dll" alias so DllUtils.CreateDllList can discover the + /// library. Returns the relative path of the source DLL to clone, or + /// null if no alias is needed (a root-level "{libName}.*" is already + /// present, or no DLL exists to clone from). + /// + /// + /// DllUtils.CreateDllList searches only the TOP LEVEL of the library + /// path (non-recursive). So a deeply-nested DLL like + /// "lib/net8.0/{libName}.dll" does NOT make the library discoverable, + /// and we still need to create a root-level alias. Only a root-level + /// "{libName}.*" suppresses alias creation. + /// + private static string DetermineAliasSource( + string libName, + string aliasFileName, + List extractedFiles) + { + foreach (string relPath in extractedFiles) + { + bool isRootLevel = + relPath.IndexOf('/') < 0 && + relPath.IndexOf('\\') < 0; + if (!isRootLevel) { - CleanupManifest(manifestPath, installDir); + continue; } - - if (innerZipPath != null) + string name = Path.GetFileName(relPath); + if (name.StartsWith(libName + ".", s_pathComparison) || + name.Equals(aliasFileName, s_pathComparison)) { - ZipFile.ExtractToDirectory(innerZipPath, installDir, false); + // A root-level file matches "{libName}.*" -- already discoverable. + return null; } - else + } + foreach (string relPath in extractedFiles) + { + if (Path.GetExtension(relPath).Equals(".dll", s_pathComparison)) { - foreach (string file in Directory.GetFiles(tempFolder)) - { - File.Copy(file, Path.Combine(installDir, Path.GetFileName(file)), false); - } - foreach (string dir in Directory.GetDirectories(tempFolder)) - { - CopyDirectory(dir, Path.Combine(installDir, Path.GetFileName(dir))); - } + return relPath; } + } + return null; + } - // Create the alias file now that content is in place. The source - // was chosen and conflict-checked above. - if (aliasSourceRelPath != null) + /// + /// Extracts the staged content from (or + /// directly from the inner zip if is + /// non-null) into the live . + /// + private static void ExtractContentToInstallDir( + string installDir, + string tempFolder, + string innerZipPath) + { + if (innerZipPath != null) + { + // SAFETY: Extracting directly to the live installDir is currently + // safe because ZipFile.ExtractToDirectory does not restore symlink + // entries as actual symlinks on any platform -- they are written + // as regular files containing the link target text. So even on + // Linux, an attacker-controlled inner ZIP cannot smuggle a symlink + // into installDir via this path today. + // + // If a future .NET version changes this (e.g. honors symlink + // entries on Linux), or if we switch to a different extraction + // library, this path MUST be re-routed through a separate temp + // folder followed by an IsReparsePoint-guarded copy -- mirror + // the non-inner-zip branch below for the pattern. + ZipFile.ExtractToDirectory(innerZipPath, installDir, false); + } + else + { + // Skip reparse points at the root of tempFolder for the same + // reason CopyDirectory does inside subdirectories: a root-level + // symlink could cause File.Copy to copy data from outside the + // staged tree, and a root-level reparse-point directory could + // make CopyDirectory recurse out of tempFolder. CopyDirectory + // only checks the entries it iterates, not its top-level + // sourceDir argument, so the guards live here at the call site. + foreach (string file in Directory.GetFiles(tempFolder)) { - string aliasSrc = Path.Combine(installDir, aliasSourceRelPath); - string alias = Path.Combine(installDir, aliasFileName); - if (File.Exists(aliasSrc)) + if (IsReparsePoint(file)) { - File.Copy(aliasSrc, alias, false); + continue; } + File.Copy(file, Path.Combine(installDir, Path.GetFileName(file)), false); } - - if (extractedFiles.Count > 0) + foreach (string dir in Directory.GetDirectories(tempFolder)) { - File.WriteAllLines(manifestPath, extractedFiles); + if (IsReparsePoint(dir)) + { + continue; + } + CopyDirectory(dir, Path.Combine(installDir, Path.GetFileName(dir))); } } - catch (Exception e) - { - string stackTracePart = string.IsNullOrEmpty(e.StackTrace) ? string.Empty : e.StackTrace + Environment.NewLine; - Logging.Error(stackTracePart + "Error: " + e.Message); - SetLibraryError(e.Message, libraryError, libraryErrorLength); - result = SQL_ERROR; - } - finally + } + + /// + /// Creates the "{libName}.dll" alias by cloning the source DLL chosen + /// by . Caller must have verified + /// the alias is needed and conflict-checked before this is invoked. + /// + private static void CreateAlias( + string installDir, + string aliasSourceRelPath, + string aliasFileName) + { + string aliasSrc = Path.Combine(installDir, aliasSourceRelPath); + string alias = Path.Combine(installDir, aliasFileName); + if (File.Exists(aliasSrc)) { - if (tempFolder != null && Directory.Exists(tempFolder)) - { - try { Directory.Delete(tempFolder, true); } - catch { /* best-effort */ } - } + File.Copy(aliasSrc, alias, false); } - - return result; } /// @@ -921,13 +1129,45 @@ public delegate short UninstallExternalLibraryDelegate( /// /// This method implements UninstallExternalLibrary API. - /// Uninstalls an external library from the specified directory. + /// Uninstalls an external library from the specified directory by + /// reading "{libName}.manifest" and deleting each listed file, then + /// pruning newly-empty subdirectories. Files belonging to other + /// libraries (not listed in this manifest) are left intact. /// /// /// See InstallExternalLibrary remarks: errors are reported via the /// libraryError out-parameter rather than ExceptionUtils.WrapError, - /// per the sqlexternallibrary.h contract. + /// per the sqlexternallibrary.h contract. A missing installDir or + /// missing manifest is treated as a no-op success (the library is + /// already in the desired state). /// + /// + /// Session GUID supplied by ExtHost. Currently unused. + /// + /// + /// UTF-8 buffer holding the library name from DROP EXTERNAL LIBRARY. + /// NOT null-terminated -- read exactly libraryNameLength bytes. + /// + /// + /// Byte length of . + /// + /// + /// UTF-8 buffer holding the absolute path to the install directory. + /// May be the public or private external library path. NOT + /// null-terminated. + /// + /// + /// Byte length of . + /// + /// + /// On failure, set to a freshly-allocated UTF-8 error string for + /// ExtHost to surface to the user. ExtHost takes ownership of the + /// allocation. On success, set to nullptr. + /// + /// + /// On failure, set to the byte length of + /// (excluding the null terminator). On success, set to 0. + /// /// /// SQL_SUCCESS(0), SQL_ERROR(-1) /// @@ -956,20 +1196,30 @@ public static short UninstallExternalLibrary( if (Directory.Exists(installDir)) { - // Check for a manifest written during install that lists - // all files extracted from the library's ZIP content. - string manifestPath = Path.Combine(installDir, libName + ".manifest"); - if (File.Exists(manifestPath)) + // Serialize against any other concurrent Install/Uninstall + // on the same installDir. An uninstall that races an install + // of a different library can otherwise see CleanupManifest + // delete its own files between the install's CheckForConflicts + // and File.Copy, leaving the install with a stale view of disk + // state. See AcquireInstallLock remarks for the full threat + // model. + using (FileStream installLock = AcquireInstallLock(installDir)) { - CleanupManifest(manifestPath, installDir); - } + // Check for a manifest written during install that lists + // all files extracted from the library's ZIP content. + string manifestPath = Path.Combine(installDir, libName + ".manifest"); + if (File.Exists(manifestPath)) + { + CleanupManifest(manifestPath, installDir); + } - // Non-ZIP installs write a single "{libName}.dll" file and - // no manifest; remove that file directly. - string libraryFile = Path.Combine(installDir, DllFileNameFor(libName)); - if (File.Exists(libraryFile)) - { - File.Delete(libraryFile); + // Non-ZIP installs write a single "{libName}.dll" file and + // no manifest; remove that file directly. + string libraryFile = Path.Combine(installDir, DllFileNameFor(libName)); + if (File.Exists(libraryFile)) + { + File.Delete(libraryFile); + } } } } @@ -1011,14 +1261,7 @@ private static void CopyDirectory(string sourceDir, string destDir) foreach (string file in Directory.GetFiles(sourceDir)) { - // Skip reparse points (symbolic links, junctions). On Linux a - // ZIP archive can carry symlinks that survive extraction; left - // unchecked, File.Copy would follow the symlink and either - // copy a file from outside installDir into it (information - // leak) or, when paired with the directory recursion below, - // open up out-of-tree writes. - FileAttributes fileAttrs = File.GetAttributes(file); - if ((fileAttrs & FileAttributes.ReparsePoint) != 0) + if (IsReparsePoint(file)) { continue; } @@ -1031,11 +1274,7 @@ private static void CopyDirectory(string sourceDir, string destDir) foreach (string dir in Directory.GetDirectories(sourceDir)) { - // Skip reparse-point directories for the same reason as above: - // following a junction/symlink could cause us to recurse out - // of the staged temp folder. - FileAttributes dirAttrs = File.GetAttributes(dir); - if ((dirAttrs & FileAttributes.ReparsePoint) != 0) + if (IsReparsePoint(dir)) { continue; } @@ -1044,13 +1283,100 @@ private static void CopyDirectory(string sourceDir, string destDir) } } + /// + /// Returns true if is a reparse point + /// (symbolic link, junction, mount point). + /// + /// + /// We treat all reparse points discovered in the staged tempFolder as + /// untrusted. On Linux a ZIP archive can carry symlink entries that + /// survive extraction; following them while copying into installDir + /// could (a) copy data from outside the staged tree (information leak) + /// or (b) cause the recursive copy to escape the staged area entirely + /// and write into arbitrary filesystem locations. + /// + private static bool IsReparsePoint(string path) + { + return (File.GetAttributes(path) & FileAttributes.ReparsePoint) != 0; + } + + // Block forever in AcquireInstallLock -- match the engine's + // "this install will eventually finish" contract. SQL Server's outer + // statement-level cancellation is the right place to interrupt a + // pathologically-stuck install, not an arbitrary timeout in here. + // (s_lockRetryDelayMs is declared at the top of the class with the + // other class members.) + + /// + /// Acquires an exclusive cross-process lock for operations on + /// . Returns a disposable handle that + /// releases the lock when disposed. + /// + /// + /// Two concurrent CSharp extension processes (e.g. two SQL sessions + /// each running CREATE/ALTER/DROP EXTERNAL LIBRARY against the same + /// install directory) must not be allowed to interleave the + /// CheckForConflicts / CleanupManifest / copy / WriteAllLines + /// sequence. Without serialization, both can pass CheckForConflicts + /// against pre-cleanup state, then one's CleanupManifest destroys + /// its previous version's content while the other's File.Copy + /// collides on overwrite:false -- leaving the second library GONE + /// with no replacement and no manifest. + /// + /// Implementation: open "{installDir}/.install.lock" with + /// FileShare.None. The OS releases the handle on process crash so + /// there is no stale-lock risk. FileOptions.DeleteOnClose removes + /// the lock file when the holder closes its handle, keeping the + /// install directory clean across runs. + /// + /// Acquisition blocks indefinitely with a 100ms retry interval. + /// In the uncontended common case (single session), acquisition + /// completes on the first attempt with no measurable overhead. + /// + private static FileStream AcquireInstallLock(string installDir) + { + Directory.CreateDirectory(installDir); + string lockPath = Path.Combine(installDir, ".install.lock"); + + while (true) + { + try + { + return new FileStream( + lockPath, + FileMode.OpenOrCreate, + FileAccess.ReadWrite, + FileShare.None, + bufferSize: 1, + FileOptions.DeleteOnClose); + } + catch (IOException) + { + // Another process holds the lock. Wait and retry. + Thread.Sleep(s_lockRetryDelayMs); + } + } + } + /// /// Recursively collects all file paths relative to the root directory. /// + /// + /// Reparse points (symlinks, junctions) are skipped at both the file + /// and directory level. The result of this walk feeds CheckForConflicts + /// and the on-disk manifest, both of which assume every entry will be + /// physically copied by CopyDirectory. Since CopyDirectory skips + /// reparse points, recording them here would create phantom manifest + /// entries that point at nothing on disk. + /// private static void CollectRelativeFiles(string directory, string prefix, List results) { foreach (string file in Directory.GetFiles(directory)) { + if (IsReparsePoint(file)) + { + continue; + } string relPath = string.IsNullOrEmpty(prefix) ? Path.GetFileName(file) : Path.Combine(prefix, Path.GetFileName(file)); @@ -1059,6 +1385,10 @@ private static void CollectRelativeFiles(string directory, string prefix, List + /// Returns the on-disk file name for a raw-DLL install of + /// . + /// + /// + /// If the library was registered with a name that already ends in + /// ".dll" (e.g. CREATE EXTERNAL LIBRARY [Scriptoria.dll]), we must + /// not append a second ".dll" -- that produced "Scriptoria.dll.dll" + /// files that the CLR assembly resolver could not locate. + /// + /// + /// The library name as supplied via libraryName to InstallExternalLibrary. + /// + /// + /// "{libName}.dll" if libName does not already end in ".dll", + /// otherwise libName unchanged. + /// private static string DllFileNameFor(string libName) { + string result; if (!string.IsNullOrEmpty(libName) && libName.EndsWith(".dll", s_pathComparison)) { - return libName; + result = libName; + } + else + { + result = libName + ".dll"; } - return libName + ".dll"; + + return result; } - // Rejects library names that could escape installDir when combined via Path.Combine. + /// + /// Validates that is safe to use in path + /// composition. Throws on rejection. + /// + /// + /// Without this check, a malicious or legacy libName like "../foo" + /// or "/etc/foo" could make + /// Path.Combine(installDir, libName + ".manifest") resolve + /// outside installDir, allowing unintended file reads / writes / + /// deletes. Also rejects names that are only an extension + /// (e.g. ".dll") because the resulting "{libName}.manifest" paths + /// would be hidden dotfiles on Linux and opaque on both platforms. + /// + /// + /// The library name as supplied via libraryName to + /// InstallExternalLibrary or UninstallExternalLibrary. + /// private static void ValidateLibraryName(string libName) { if (string.IsNullOrEmpty(libName)) { throw new ArgumentException("Library name must not be empty."); } + if (libName.IndexOfAny(new[] { '/', '\\', '\0' }) >= 0 || libName.Contains("..") || Path.IsPathRooted(libName)) @@ -1102,25 +1461,71 @@ private static void ValidateLibraryName(string libName) throw new ArgumentException( $"Library name '{libName}' contains invalid characters."); } + + // Reject names that are only an extension (e.g. ".dll", ".txt"). + // Path.GetFileNameWithoutExtension returns "" for these, meaning + // the name has no stem -- DllFileNameFor would return the bare + // extension and the resulting "{libName}.manifest" / "{libName}.dll" + // paths would be hidden dotfiles on Linux and opaque on both + // platforms. + if (string.IsNullOrEmpty(Path.GetFileNameWithoutExtension(libName))) + { + throw new ArgumentException( + $"Library name '{libName}' must not be only an extension."); + } } - // A ZIP file of any variant begins with the 'PK' magic bytes (0x50 0x4B). + /// + /// Returns true if the file at appears to be + /// a ZIP archive based on its leading two "magic bytes". + /// + /// + /// All ZIP variants (.zip, .jar, .war, .docx, etc.) begin with the + /// two-byte signature "PK" (0x50 0x4B), inherited from PKWARE's + /// original 1989 PKZIP format. We sniff content rather than trust + /// the file extension because: + /// + /// + /// Callers can register a library file as e.g. "foo.zip" without + /// the bytes actually being a ZIP archive (the engine doesn't + /// require the registered name to match content). + /// + /// + /// Callers can register a library file with no extension or with a + /// non-".zip" extension (e.g. ".dll", ".bin") whose contents ARE a + /// ZIP archive. + /// + /// + /// The two-byte sniff is intentionally minimal: a file beginning with + /// "PK" but corrupt internally will still reach + /// ZipFile.ExtractToDirectory, which throws a clear exception that + /// the outer catch surfaces via libraryError. The sniff's job is + /// only to dispatch between the raw-DLL and ZIP install paths -- + /// not to validate ZIP integrity. + /// private static bool IsZipFile(string path) { // FileShare.Read so we don't fight with anti-virus scanners, // backup agents, or another concurrent SQL session that may have // the source file open for read. - using (var fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read)) + using (FileStream fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read)) { byte[] magic = new byte[2]; return fs.Read(magic, 0, 2) == 2 && magic[0] == 0x50 && magic[1] == 0x4B; } } - // Reads an existing manifest into a set of relative paths. Empty set if absent. + /// + /// Reads an existing manifest file into a set of relative paths. + /// Returns an empty set if the manifest does not exist. + /// + /// + /// Absolute path to the "{libName}.manifest" file. Safe to pass a + /// non-existent path. + /// private static HashSet ReadManifestEntries(string manifestPath) { - var set = new HashSet(s_pathComparer); + HashSet set = new HashSet(s_pathComparer); if (File.Exists(manifestPath)) { foreach (string line in File.ReadAllLines(manifestPath)) @@ -1131,11 +1536,50 @@ private static HashSet ReadManifestEntries(string manifestPath) } } } + return set; } - // Converts a ZIP entry path to a platform-native relative path and verifies - // it does not escape installDir (defense-in-depth zip-slip check). + /// + /// Converts a ZIP entry path to a platform-native relative path and + /// verifies it does not escape + /// (defense-in-depth zip-slip check). + /// + /// + /// Two transformations + one check: + /// + /// + /// Separator normalization. ZIP entries always use forward + /// slashes per the ZIP spec (PKWARE APPNOTE 4.4.17.1). We rewrite + /// '/' to so subsequent + /// / calls + /// produce native paths on Windows ('\\') and Linux ('/'). We do + /// NOT touch backslashes -- they are illegal in ZIP entry names per + /// spec, and on Linux a backslash is a legitimate filename + /// character that must be preserved as-is. + /// + /// + /// Canonicalization via . + /// Resolves any "..", ".", or symlink-style segments in both + /// installDir and the combined path so the StartsWith check below + /// is performed on absolute, canonical paths. + /// + /// + /// Containment check. The combined path must start with + /// {fullInstall}{DirectorySeparatorChar}. Comparing with the + /// trailing separator is critical: without it, an installDir of + /// "C:\install" would falsely accept entries that resolve to + /// "C:\installEvil\..." (sibling directory with shared prefix). + /// + /// + /// .NET's ZipFile.ExtractToDirectory already performs its own + /// zip-slip check on extraction (since .NET Core 2.1). This + /// function is defense in depth: the manifest we write must list + /// only paths inside installDir, so that uninstall's + /// File.Delete(...) calls cannot be tricked into deleting + /// arbitrary files via a malicious manifest entry that survived + /// extraction. + /// private static string ValidateRelativePath(string installDir, string zipEntryFullName) { string relPath = zipEntryFullName.Replace('/', Path.DirectorySeparatorChar); @@ -1149,6 +1593,7 @@ private static string ValidateRelativePath(string installDir, string zipEntryFul throw new InvalidOperationException( $"Library archive contains entry with invalid path: '{zipEntryFullName}'."); } + return relPath; } @@ -1185,7 +1630,7 @@ private static void CleanupManifest(string manifestPath, string installDir) string prefix = fullInstall.EndsWith(sep) ? fullInstall : fullInstall + sep; string[] entries = File.ReadAllLines(manifestPath); - var parentDirs = new HashSet(s_pathComparer); + HashSet parentDirs = new HashSet(s_pathComparer); foreach (string relPath in entries) { @@ -1203,8 +1648,10 @@ private static void CleanupManifest(string manifestPath, string installDir) { // A malformed manifest entry shouldn't abort the rest of // the cleanup, but it must leave a diagnostic trail so - // orphaned files don't disappear silently. - Logging.Trace( + // orphaned files don't disappear silently. Use Error + // (not Trace) -- the comment promises a trail and Trace + // is typically off in production deployments. + Logging.Error( $"CleanupManifest: skipping manifest entry '{relPath}': {ex.Message}"); continue; } @@ -1230,7 +1677,7 @@ private static void CleanupManifest(string manifestPath, string installDir) } // Remove empty directories deepest first. - var sortedDirs = new List(parentDirs); + List sortedDirs = new List(parentDirs); sortedDirs.Sort((a, b) => SeparatorCount(b).CompareTo(SeparatorCount(a))); foreach (string dir in sortedDirs) { diff --git a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs index 110b96b4..6171f5de 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs @@ -114,6 +114,20 @@ public static List CreateDllList( /// "{name}.*" wildcard for bare names (so callers that pass "Foo" /// still match "Foo.dll", "Foo.runtimeconfig.json", etc.). /// + /// + /// Directory to search. Returns immediately if null/empty or absent; + /// missing public/private library paths are not an error -- the other + /// path may still yield matches. + /// + /// + /// Library name to match. May be a bare stem ("regex") or include a + /// .dll suffix ("Foo.dll"). Wildcards / path separators are not + /// expected -- the caller is responsible for validation. + /// + /// + /// Output list to append discovered .dll paths to. Caller-owned; + /// AddMatches never clears or replaces. + /// private static void AddMatches(string searchPath, string userLibName, List dllList) { if (string.IsNullOrEmpty(searchPath) || !Directory.Exists(searchPath)) diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index 247d7032..4d28d872 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -12,11 +12,12 @@ // //********************************************************************* #include "CSharpExtensionApiTests.h" +#include #include #include using namespace std; -namespace fs = experimental::filesystem; +namespace fs = std::filesystem; namespace ExtensionApiTest { @@ -121,7 +122,7 @@ namespace ExtensionApiTest // Helper: check if directory has any files or subdirectories // - static bool DirectoryHasFiles(const string &dir) + static bool DoesDirectoryHaveFiles(const string &dir) { bool hasFiles = false; @@ -232,7 +233,8 @@ namespace ExtensionApiTest // // Description: // Tests installing an outer zip that contains DLLs directly (no inner zip). - // Verifies that DLLs are copied to the install directory. + // Verifies that the specific expected DLLs are extracted to the install + // directory. // TEST_F(CSharpExtensionApiTests, InstallZipContainingDllsTest) { @@ -246,30 +248,34 @@ namespace ExtensionApiTest "testpackageB", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - // Verify that DLL files were copied to the install directory - bool hasDll = false; - for (const auto &entry : fs::directory_iterator(installDir)) - { - if (entry.path().extension() == ".dll") - { - hasDll = true; - break; - } - } - EXPECT_TRUE(hasDll) << "No DLL files found in install directory after installation"; + // Assert on the specific files we expect from package B's contents. + // "any DLL exists" is too weak: it would pass even if the install + // extracted the wrong package or a stale file from a prior test + // survived. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")) + << "Expected testpackageB.dll not extracted"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.deps.json")) + << "Expected testpackageB.deps.json not extracted"; CleanupInstallDir(installDir); } //---------------------------------------------------------------------------------------------- - // Name: InstallInvalidZipTest + // Name: InstallNonZipFileAsRawDllTest // // Description: - // Tests that installing a file with .zip extension that is not actually - // a valid ZIP (magic bytes are not PK) copies it to the install directory - // as a raw file named after the library and returns SQL_SUCCESS. - // - TEST_F(CSharpExtensionApiTests, InstallInvalidZipTest) + // Tests that installing a file with .zip extension that is NOT actually + // a valid ZIP (magic bytes are not 'PK') falls through to the raw-DLL + // install path: the file is copied to the install directory as + // "{libName}.dll" and SQL_SUCCESS is returned. This pins the contract + // that IsZipFile detects content (not file extension). + // + // NOTE: this does NOT exercise corrupt-ZIP handling. A genuinely + // invalid ZIP would start with 'PK' but be malformed inside, causing + // ZipFile.ExtractToDirectory to throw on the ZIP path. We have no + // fixture for that case today. + // + TEST_F(CSharpExtensionApiTests, InstallNonZipFileAsRawDllTest) { string packagesPath = GetPackagesPath(); string packagePath = (fs::path(packagesPath) / "bad-package-ZIP.zip").string(); @@ -326,7 +332,7 @@ namespace ExtensionApiTest SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, "testpackageB", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - EXPECT_TRUE(DirectoryHasFiles(installDir)) << "No files found after installation"; + EXPECT_TRUE(DoesDirectoryHaveFiles(installDir)) << "No files found after installation"; // Uninstall result = CallUninstall(sm_uninstallExternalLibraryFuncPtr, @@ -334,7 +340,7 @@ namespace ExtensionApiTest EXPECT_EQ(result, SQL_SUCCESS); // Verify directory is empty - EXPECT_FALSE(DirectoryHasFiles(installDir)) << "Files still present after uninstall"; + EXPECT_FALSE(DoesDirectoryHaveFiles(installDir)) << "Files still present after uninstall"; CleanupInstallDir(installDir); } @@ -366,22 +372,9 @@ namespace ExtensionApiTest "testpackage", packagePathB, installDir); EXPECT_EQ(result, SQL_SUCCESS); - // Verify the install directory has files from the second package - bool hasDll = false; - for (const auto &entry : fs::directory_iterator(installDir)) - { - if (entry.path().extension() == ".dll") - { - hasDll = true; - break; - } - } - EXPECT_TRUE(hasDll) << "DLL from second package not found after reinstall"; - - // Also verify the FIRST package's unique files were removed by the - // ALTER cleanup. Without these checks the test passes even if the - // manifest-based cleanup is broken (the second install just adds - // its own files alongside the stale ones). + // Verify v1's unique files were removed by the manifest cleanup, + // and v2's expected file is present. The earlier "any .dll exists" + // loop is redundant given these explicit checks. EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.dll")) << "Stale testpackageA.dll left behind after reinstall"; EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.txt")) @@ -416,6 +409,57 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + //---------------------------------------------------------------------------------------------- + // Name: AlterToEmptyDirsZipPreservesV1Test + // + // Description: + // Regression test for the silent-data-loss scenario on ALTER. A ZIP + // containing only directory entries (no file entries) used to slip + // past the empty-archive guard because Directory.GetDirectories on + // the extracted tempFolder is non-empty. CollectRelativeFiles then + // returns 0 entries; CleanupManifest deletes the previous version's + // content; nothing is copied; and the manifest write is skipped + // because it is gated on extractedFiles.Count > 0. The library would + // end up GONE with no replacement and no manifest. + // + // After the fix, install fails with SQL_ERROR before any cleanup + // runs, so v1 must remain byte-for-byte intact. + // + TEST_F(CSharpExtensionApiTests, AlterToEmptyDirsZipPreservesV1Test) + { + string packagesPath = GetPackagesPath(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + string emptyDirsZip = (fs::path(packagesPath) / "testpackageI-EMPTYDIRS.zip").string(); + ASSERT_TRUE(fs::exists(pkgB)); + ASSERT_TRUE(fs::exists(emptyDirsZip)); + + string installDir = CreateInstallDir(); + + // v1: install a real ZIP package as "myLib". + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", pkgB, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.deps.json")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); + + // v2: attempt ALTER with a ZIP whose only entries are empty + // directories. Must FAIL -- and v1 must survive intact. + SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", emptyDirsZip, installDir); + EXPECT_EQ(r, SQL_ERROR) + << "ALTER to empty-dirs ZIP must fail rather than silently delete v1"; + + // v1's content must be untouched. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")) + << "v1's testpackageB.dll was deleted by failed ALTER"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.deps.json")) + << "v1's testpackageB.deps.json was deleted by failed ALTER"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")) + << "v1's manifest was deleted by failed ALTER"; + + CleanupInstallDir(installDir); + } + //---------------------------------------------------------------------------------------------- // Name: InstallZipWithNestedDirectoriesTest // @@ -503,8 +547,14 @@ namespace ExtensionApiTest // Name: InstallZipWithManyFilesTest // // Description: - // Tests that installing a zip containing many files (50 DLLs) extracts - // all of them correctly. + // Tests that installing a ZIP containing many files extracts all of + // them correctly AND that the alias is created on top. + // + // Package contents: testpackageG-MANYFILES.zip contains exactly 50 + // DLLs (Module1.dll .. Module50.dll) and no other files. None of + // them matches "manyfilespackage.*", so install must clone the first + // DLL as an alias named "manyfilespackage.dll". The DLL count in + // installDir is therefore 50 (extracted) + 1 (alias) = 51. // TEST_F(CSharpExtensionApiTests, InstallZipWithManyFilesTest) { @@ -518,16 +568,17 @@ namespace ExtensionApiTest "manyfilespackage", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - // Count extracted DLL files + // Count DLL files in installDir. 50 from the package + 1 alias = 51. int dllCount = 0; - for (const auto &entry : fs::directory_iterator(installDir)) + for (const fs::directory_entry &entry : fs::directory_iterator(installDir)) { if (entry.path().extension() == ".dll") { dllCount++; } } - EXPECT_EQ(dllCount, 50) << "Expected 50 extracted DLL files, found " << dllCount; + EXPECT_EQ(dllCount, 51) + << "Expected 51 DLL files (50 extracted + 1 alias), found " << dllCount; // Verify the alias exists so DllUtils can discover the library by name. EXPECT_TRUE(fs::exists(fs::path(installDir) / "manyfilespackage.dll")); @@ -661,16 +712,18 @@ namespace ExtensionApiTest vector entries = ReadManifest(manifestPath); EXPECT_GE(entries.size(), 2u) << "Manifest should list at least 2 extracted files"; - // Manifest should contain the actual extracted file names + // Manifest entries are EXACT relative paths -- assert equality, not + // substring containment. Substring match would accept "x.dll", + // "testpackageB.dll.bak", etc. and miss real bugs. bool hasDll = false; bool hasDeps = false; - for (const auto &e : entries) + for (const string &e : entries) { - if (e.find("testpackageB.dll") != string::npos) hasDll = true; - if (e.find("testpackageB.deps.json") != string::npos) hasDeps = true; + if (e == "testpackageB.dll") { hasDll = true; } + if (e == "testpackageB.deps.json") { hasDeps = true; } } - EXPECT_TRUE(hasDll) << "Manifest missing testpackageB.dll"; - EXPECT_TRUE(hasDeps) << "Manifest missing testpackageB.deps.json"; + EXPECT_TRUE(hasDll) << "Manifest missing exact entry 'testpackageB.dll'"; + EXPECT_TRUE(hasDeps) << "Manifest missing exact entry 'testpackageB.deps.json'"; CleanupInstallDir(installDir); } @@ -742,16 +795,42 @@ namespace ExtensionApiTest "myAlias", packagePath, installDir); EXPECT_EQ(result, SQL_SUCCESS); - EXPECT_TRUE(fs::exists(fs::path(installDir) / "myAlias.dll")) + fs::path aliasFile = fs::path(installDir) / "myAlias.dll"; + fs::path sourceDll = fs::path(installDir) / "testpackageB.dll"; + ASSERT_TRUE(fs::exists(aliasFile)) << "Expected alias file 'myAlias.dll' not found"; + ASSERT_TRUE(fs::exists(sourceDll)) + << "Expected source file 'testpackageB.dll' not extracted"; + + // Alias must be a byte-for-byte copy of the source DLL (the first + // .dll discovered in the package). A zero-length alias, a copy of + // the wrong file, or a partial write would all silently pass the + // "file exists" check above without this content check. + EXPECT_EQ(fs::file_size(aliasFile), fs::file_size(sourceDll)) + << "Alias file size differs from source DLL"; + + std::ifstream aliasStream(aliasFile.string(), std::ios::binary); + std::ifstream sourceStream(sourceDll.string(), std::ios::binary); + std::string aliasContents( + (std::istreambuf_iterator(aliasStream)), + std::istreambuf_iterator()); + std::string sourceContents( + (std::istreambuf_iterator(sourceStream)), + std::istreambuf_iterator()); + EXPECT_EQ(aliasContents, sourceContents) + << "Alias file contents differ from source DLL"; // Manifest should include the alias. vector entries = ReadManifest( (fs::path(installDir) / "myAlias.manifest").string()); bool hasAlias = false; - for (const auto &e : entries) + for (const string &e : entries) { - if (e == "myAlias.dll") { hasAlias = true; break; } + if (e == "myAlias.dll") + { + hasAlias = true; + break; + } } EXPECT_TRUE(hasAlias) << "Manifest missing alias entry 'myAlias.dll'"; @@ -946,10 +1025,16 @@ namespace ExtensionApiTest vector entries = ReadManifest( (fs::path(installDir) / "myLib.manifest").string()); bool hasA = false, hasB = false; - for (const auto &e : entries) + for (const string &e : entries) { - if (e.find("testpackageA") != string::npos) hasA = true; - if (e.find("testpackageB") != string::npos) hasB = true; + if (e.find("testpackageA") != string::npos) + { + hasA = true; + } + if (e.find("testpackageB") != string::npos) + { + hasB = true; + } } EXPECT_FALSE(hasA) << "Manifest still references v1 files"; EXPECT_TRUE(hasB) << "Manifest missing v2 files"; @@ -973,10 +1058,15 @@ namespace ExtensionApiTest string msg; // Failure mode 1: non-existent source file + string missingPath = "C:\\does\\not\\exist.zip"; SQLRETURN r = CallInstallCaptureError(sm_installExternalLibraryFuncPtr, - "missing", "C:\\does\\not\\exist.zip", installDir, msg); + "missing", missingPath, installDir, msg); EXPECT_EQ(r, SQL_ERROR); EXPECT_FALSE(msg.empty()) << "No error message populated for missing file"; + // Message should mention the path that was not found, so the user + // can fix the input rather than guess. + EXPECT_NE(msg.find("exist.zip"), string::npos) + << "Missing-file message should reference the path. Got: " << msg; // Failure mode 2: zip-slip attack string zipSlip = (fs::path(packagesPath) / "testpackageH-ZIPSLIP.zip").string(); @@ -984,6 +1074,11 @@ namespace ExtensionApiTest "slip", zipSlip, installDir, msg); EXPECT_EQ(r, SQL_ERROR); EXPECT_FALSE(msg.empty()) << "No error message populated for zip-slip"; + // Message should describe the attack class so the user (or a security + // log scanner) can identify it. ValidateRelativePath throws with + // "contains entry with invalid path" -- match a stable substring. + EXPECT_NE(msg.find("invalid path"), string::npos) + << "Zip-slip message should describe the rejection. Got: " << msg; // Failure mode 3: file-level conflict string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); @@ -1146,12 +1241,34 @@ namespace ExtensionApiTest EXPECT_EQ(r, SQL_SUCCESS) << "ALTER from non-ZIP to ZIP should succeed"; - // v1's raw DLL should be gone (the alias for v2 may or may not be - // named myLib.dll depending on whether v2 contains testpackageB.dll - // and needs an alias). v2's content must be present. + // v2's content must be present + manifest exists. EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.dll")); + EXPECT_TRUE(fs::exists(fs::path(installDir) / "testpackageB.deps.json")); EXPECT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); + // v1's raw DLL bytes must be GONE. Both the file and the alias for + // v2 are named myLib.dll, so a broken cleanup (overwrite-instead-of- + // clean-then-install) would be masked by mere existence of the file. + // Compare bytes against testpackageB.dll: if the alias was created + // correctly from v2's content, it must equal the source DLL. + fs::path myLibDll = fs::path(installDir) / "myLib.dll"; + fs::path sourceDll = fs::path(installDir) / "testpackageB.dll"; + ASSERT_TRUE(fs::exists(myLibDll)) + << "v2's alias myLib.dll missing"; + EXPECT_EQ(fs::file_size(myLibDll), fs::file_size(sourceDll)) + << "myLib.dll size differs from v2's source -- likely still v1's bytes"; + + std::ifstream myLibStream(myLibDll.string(), std::ios::binary); + std::ifstream sourceStream(sourceDll.string(), std::ios::binary); + std::string myLibBytes( + (std::istreambuf_iterator(myLibStream)), + std::istreambuf_iterator()); + std::string sourceBytes( + (std::istreambuf_iterator(sourceStream)), + std::istreambuf_iterator()); + EXPECT_EQ(myLibBytes, sourceBytes) + << "myLib.dll content differs from v2's source -- v1's raw DLL was not cleaned up"; + CleanupInstallDir(installDir); } @@ -1314,7 +1431,9 @@ namespace ExtensionApiTest // Create a sentinel file OUTSIDE installDir that uninstall must not touch. fs::path sentinelDir = fs::path(installDir).parent_path(); fs::path sentinel = sentinelDir / "do-not-delete.manifest"; - { std::ofstream os(sentinel.string()); os << "sentinel"; } + std::ofstream sentinelStream(sentinel.string()); + sentinelStream << "sentinel"; + sentinelStream.close(); ASSERT_TRUE(fs::exists(sentinel)); // Attempt uninstall with a traversal libName. @@ -1327,6 +1446,39 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + //---------------------------------------------------------------------------------------------- + // Name: InstallRejectsExtensionOnlyLibNameTest + // + // Description: + // ValidateLibraryName must reject libNames that are only an extension + // (e.g. ".dll"). Without this check, DllFileNameFor(".dll") returns + // ".dll", producing hidden dotfiles like "{installDir}/.dll" and + // "{installDir}/.dll.manifest" that are opaque on Windows and hidden + // on Linux. + // + TEST_F(CSharpExtensionApiTests, InstallRejectsExtensionOnlyLibNameTest) + { + string packagesPath = GetPackagesPath(); + string rawDll = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + ASSERT_TRUE(fs::exists(rawDll)); + + string installDir = CreateInstallDir(); + + // libName is just an extension -- no stem. Must be rejected before + // any filesystem operation. + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + ".dll", rawDll, installDir); + EXPECT_EQ(result, SQL_ERROR) << "Install must reject extension-only libName"; + + // No file should have been created at "installDir/.dll" or its manifest. + EXPECT_FALSE(fs::exists(fs::path(installDir) / ".dll")) + << "Hidden dotfile created from extension-only libName"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / ".dll.manifest")) + << "Hidden dotfile manifest created from extension-only libName"; + + CleanupInstallDir(installDir); + } + //---------------------------------------------------------------------------------------------- // Name: AliasConflictDetectedBeforeExtractionTest // @@ -1352,12 +1504,20 @@ namespace ExtensionApiTest // Plant a "shared.dll" file in installDir (simulates ownership by another library). fs::path squatter = fs::path(installDir) / "shared.dll"; - { std::ofstream os(squatter.string()); os << "squatter"; } + std::ofstream squatterStream(squatter.string()); + squatterStream << "squatter"; + squatterStream.close(); ASSERT_TRUE(fs::exists(squatter)); // Count files currently in installDir; install of B must not add any. size_t fileCountBefore = 0; - for (auto &p : fs::recursive_directory_iterator(installDir)) if (fs::is_regular_file(p)) ++fileCountBefore; + for (const fs::directory_entry &p : fs::recursive_directory_iterator(installDir)) + { + if (fs::is_regular_file(p)) + { + ++fileCountBefore; + } + } // Install B with libName "shared" - it has no shared.* file, so install // would create a "shared.dll" alias, which collides with squatter. @@ -1367,7 +1527,13 @@ namespace ExtensionApiTest // No B content should have been written. size_t fileCountAfter = 0; - for (auto &p : fs::recursive_directory_iterator(installDir)) if (fs::is_regular_file(p)) ++fileCountAfter; + for (const fs::directory_entry &p : fs::recursive_directory_iterator(installDir)) + { + if (fs::is_regular_file(p)) + { + ++fileCountAfter; + } + } EXPECT_EQ(fileCountBefore, fileCountAfter) << "Failed install left partial state in installDir"; @@ -1396,7 +1562,9 @@ namespace ExtensionApiTest // Plant a foreign file at the target path with no matching manifest // -- simulates ownership by another library or external tooling. fs::path foreign = fs::path(installDir) / "owned.dll"; - { std::ofstream os(foreign.string()); os << "FOREIGN-CONTENT"; } + std::ofstream foreignStream(foreign.string()); + foreignStream << "FOREIGN-CONTENT"; + foreignStream.close(); ASSERT_TRUE(fs::exists(foreign)); // Install must FAIL rather than overwrite the foreign file. @@ -1420,4 +1588,254 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + + //---------------------------------------------------------------------------------------------- + // Name: AlterFromNonZipToNonZipTest + // + // Description: + // ALTER EXTERNAL LIBRARY scenario where both v1 and v2 are raw DLLs + // with the same libName. v1 writes a single-entry manifest; v2 must + // see that manifest, treat the existing "{libName}.dll" as + // owned-by-previous, clean it up, then copy v2's bytes into place. + // Completes the ALTER coverage matrix (ZIP->ZIP, raw->ZIP, ZIP->raw, + // and now raw->raw). + // + TEST_F(CSharpExtensionApiTests, AlterFromNonZipToNonZipTest) + { + string packagesPath = GetPackagesPath(); + string rawDll = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + ASSERT_TRUE(fs::exists(rawDll)); + + string installDir = CreateInstallDir(); + + // v1: raw DLL install. Writes myLib.dll plus myLib.manifest. + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", rawDll, installDir), SQL_SUCCESS); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.dll")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); + + // v2: raw DLL install of the same libName (same source bytes is fine + // -- the test exercises the manifest cleanup + re-install path). + SQLRETURN r = CallInstall(sm_installExternalLibraryFuncPtr, + "myLib", rawDll, installDir); + EXPECT_EQ(r, SQL_SUCCESS) << "ALTER raw->raw should succeed"; + + // The file and manifest must still be present, and the manifest + // should still contain exactly one entry (no duplication from a + // missed cleanup of the previous manifest). + EXPECT_TRUE(fs::exists(fs::path(installDir) / "myLib.dll")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "myLib.manifest")); + vector entries = ReadManifest( + (fs::path(installDir) / "myLib.manifest").string()); + EXPECT_EQ(entries.size(), 1u) + << "Raw->raw ALTER must not duplicate manifest entries"; + if (!entries.empty()) + { + EXPECT_EQ(entries[0], "myLib.dll"); + } + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallRejectsInvalidLibNameTest + // + // Description: + // ValidateLibraryName must reject all of the following on the install + // side (uninstall is covered separately by + // UninstallRejectsPathTraversalLibNameTest): + // - empty string + // - path-traversal segment (".." resolved against installDir) + // - embedded null character + // - absolute path + // - extension-only (covered also by InstallRejectsExtensionOnlyLibNameTest) + // A regression in ValidateLibraryName must trip at least one of these + // cases. + // + TEST_F(CSharpExtensionApiTests, InstallRejectsInvalidLibNameTest) + { + string packagesPath = GetPackagesPath(); + string rawDll = (fs::path(packagesPath) / "testpackageE-RAWDLL.dll").string(); + ASSERT_TRUE(fs::exists(rawDll)); + + struct Case + { + string libName; + const char *label; + }; + // Note: "foo\0bar" must be constructed via the (data, length) ctor + // so the embedded NUL survives. CallInstall forwards libName.length(). + Case cases[] = { + { string(""), "empty" }, + { string("../escape"), "path-traversal" }, + { string("foo\0bar", 7), "null-character" }, +#ifdef _WIN32 + { string("C:\\Windows\\foo"), "absolute-path-windows" }, +#else + { string("/etc/foo"), "absolute-path-posix" }, +#endif + { string(".dll"), "extension-only" }, + }; + + for (const Case &c : cases) + { + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + c.libName, rawDll, installDir); + EXPECT_EQ(result, SQL_ERROR) + << "ValidateLibraryName should reject case: " << c.label; + + CleanupInstallDir(installDir); + } + } + + //---------------------------------------------------------------------------------------------- + // Name: InstallZipWithDllSuffixedLibNameTest + // + // Description: + // Symmetric coverage with InstallRawDllWithDllSuffixedLibNameTest. When + // CREATE EXTERNAL LIBRARY [foo.dll] is paired with a ZIP package whose + // contents do NOT include a "foo.dll" file at the root, install must + // create a single "foo.dll" alias (NOT "foo.dll.dll") and the alias + // must be tracked in the manifest as "foo.dll". + // + TEST_F(CSharpExtensionApiTests, InstallZipWithDllSuffixedLibNameTest) + { + string packagesPath = GetPackagesPath(); + string pkgB = (fs::path(packagesPath) / "testpackageB-DLL.zip").string(); + ASSERT_TRUE(fs::exists(pkgB)); + + string installDir = CreateInstallDir(); + + // libName ends in .dll. The package contains testpackageB.dll + + // testpackageB.deps.json -- nothing matching "foo.*" -- so the + // install code must create an alias. + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "foo.dll", pkgB, installDir), SQL_SUCCESS); + + // Single .dll suffix -- not "foo.dll.dll". + EXPECT_TRUE(fs::exists(fs::path(installDir) / "foo.dll")) + << "Alias not created at expected single-.dll path"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / "foo.dll.dll")) + << "Alias incorrectly written with double-.dll extension"; + + // Manifest tracks the alias under its single-.dll name. + ASSERT_TRUE(fs::exists(fs::path(installDir) / "foo.dll.manifest")); + vector entries = ReadManifest( + (fs::path(installDir) / "foo.dll.manifest").string()); + bool aliasInManifest = false; + for (const string &e : entries) + { + if (e == "foo.dll") + { + aliasInManifest = true; + break; + } + } + EXPECT_TRUE(aliasInManifest) + << "Manifest must list the alias under its single-.dll name"; + + CleanupInstallDir(installDir); + } + + //---------------------------------------------------------------------------------------------- + // Name: UninstallWithMissingInstallDirTest + // + // Description: + // Uninstall must succeed (no-op) when installDir does not exist on + // disk at all. The Directory.Exists guard in UninstallExternalLibrary + // short-circuits manifest cleanup; this test pins that contract so a + // future change can't accidentally throw on a missing directory. + // Distinct from UninstallNonExistentLibraryTest which creates the + // installDir first. + // + TEST_F(CSharpExtensionApiTests, UninstallWithMissingInstallDirTest) + { + // Construct a path that does NOT exist. Use a sibling of the + // standard testInstallLibs path so we know the parent directory + // is writable but the target itself is absent. + char path[MAX_PATH + 1] = { 0 }; + GetModuleFileName(NULL, path, MAX_PATH); + fs::path missing = fs::path(path).parent_path() / "testInstallLibs-missing"; + if (fs::exists(missing)) + { + fs::remove_all(missing); + } + ASSERT_FALSE(fs::exists(missing)) + << "Test setup error: chosen installDir must not exist"; + + SQLRETURN r = CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "anything", missing.string()); + EXPECT_EQ(r, SQL_SUCCESS) + << "Uninstall against a missing installDir must succeed (no-op)"; + + // Should NOT have created the directory as a side effect. + EXPECT_FALSE(fs::exists(missing)) + << "Uninstall must not create installDir"; + } + + //---------------------------------------------------------------------------------------------- + // Name: UninstallPreservesSharedNestedDirsTest + // + // Description: + // Two libraries can share a nested parent directory (e.g. both contribute + // files under "lib/net8.0/"). Uninstalling one must leave the other's + // files AND the shared parent directory itself intact. + // + // Library 1 = testpackageD-NESTED.zip -> lib/net8.0/MyLib.dll, runtimes/win-x64/native.dll, MyLib.deps.json + // Library 2 = testpackageJ-NESTED2.zip -> lib/net8.0/Other.dll, Other.deps.json + // + // After installing both: lib/net8.0/ contains MyLib.dll AND Other.dll. + // After uninstalling library 1: lib/net8.0/Other.dll AND lib/net8.0/ + // AND lib/ must all still exist. + // + TEST_F(CSharpExtensionApiTests, UninstallPreservesSharedNestedDirsTest) + { + string packagesPath = GetPackagesPath(); + string pkgD = (fs::path(packagesPath) / "testpackageD-NESTED.zip").string(); + string pkgJ = (fs::path(packagesPath) / "testpackageJ-NESTED2.zip").string(); + ASSERT_TRUE(fs::exists(pkgD)); + ASSERT_TRUE(fs::exists(pkgJ)); + + string installDir = CreateInstallDir(); + + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "lib1", pkgD, installDir), SQL_SUCCESS); + ASSERT_EQ(CallInstall(sm_installExternalLibraryFuncPtr, + "lib2", pkgJ, installDir), SQL_SUCCESS); + + // Pre-uninstall sanity: both libraries' files coexist in lib/net8.0/. + ASSERT_TRUE(fs::exists(fs::path(installDir) / "lib" / "net8.0" / "MyLib.dll")); + ASSERT_TRUE(fs::exists(fs::path(installDir) / "lib" / "net8.0" / "Other.dll")); + + // Uninstall library 1. + ASSERT_EQ(CallUninstall(sm_uninstallExternalLibraryFuncPtr, + "lib1", installDir), SQL_SUCCESS); + + // Library 1's unique files must be gone. + EXPECT_FALSE(fs::exists(fs::path(installDir) / "lib" / "net8.0" / "MyLib.dll")) + << "lib1's MyLib.dll leaked after uninstall"; + EXPECT_FALSE(fs::exists(fs::path(installDir) / "MyLib.deps.json")) + << "lib1's MyLib.deps.json leaked after uninstall"; + + // Library 2's files must be untouched. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib" / "net8.0" / "Other.dll")) + << "lib2's Other.dll was wrongly removed by lib1's uninstall"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "Other.deps.json")) + << "lib2's Other.deps.json was wrongly removed by lib1's uninstall"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib2.manifest")) + << "lib2's manifest was wrongly removed by lib1's uninstall"; + + // The shared parent directory MUST still exist (lib2 still has + // content there). Empty-dir cleanup must only fire on dirs that + // become empty, not on dirs that still have content from another + // library. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib" / "net8.0")) + << "Shared parent directory lib/net8.0/ wrongly removed by lib1's uninstall"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "lib")) + << "Shared parent directory lib/ wrongly removed by lib1's uninstall"; + + CleanupInstallDir(installDir); + } } diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageI-EMPTYDIRS.zip b/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageI-EMPTYDIRS.zip new file mode 100644 index 0000000000000000000000000000000000000000..5ad591fc858d3cd0354c926651b019604c71aed8 GIT binary patch literal 408 zcmWIWW@Zs#0D(y3IWb@clwbkUIhjfN0XS7~LsjIZmRRT+;8Dm4R9IA+SCW~VT8u}T zAhNRZ%skx+GZUb4MkWyk+)f1QK>`p5Q1DQx3!mSPD bB?Kq}GU3_~9%N+$DPaM^hd_EEh{FH?fl4yc literal 0 HcmV?d00001 diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageJ-NESTED2.zip b/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageJ-NESTED2.zip new file mode 100644 index 0000000000000000000000000000000000000000..38cf0cbe5eea10f2f0f67459e2b32c5f1824d6ac GIT binary patch literal 331 zcmWIWW@Zs#U|`^2P&S(r(|@)gLJ!E(0b-D7PG*vRUTTSjo`Jr9Nk(dsUP?}m_6biP zzq2RKdwS{yd7L}1?Ri!^(Brgz(20{im(F|ZZoFW8;k*7x58X@NCr>gm1fZDTH!lqmckMU!a?S c?lOe-1RxX5odMpgY#;?pKv)l?w}Che0CdMzUjP6A literal 0 HcmV?d00001 From 9e6d5fb5c96b140a1e5908697eeac0448e9f2ab9 Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Tue, 28 Apr 2026 09:47:59 -0700 Subject: [PATCH 13/18] Fix gtest suite regressions discovered by local test run (38c553d -> 111/111) Found and fixed 4 issues introduced by the v3.9.0 changeset: 1. Lock file in installDir interfered with tests enumerating installDir contents and racing fs::remove_all on DeleteOnClose. Moved lock to a sibling path '{installDir}.install.lock' (concatenated, NOT a child of installDir). 2. C6 refactor lost a temp-folder cleanup invariant: 'tempFolder = InstallZipPackage(...)' only published the path on successful return, so a throw inside InstallZipPackage left an unreachable tempFolder leaking inside installDir. Converted to 'out string tempFolder' assigned BEFORE any work that can throw. 3. Three test bodies read files via std::ifstream without explicit close(); on Windows this raced CleanupInstallDir's fs::remove_all because the destructor runs lexically late. Added explicit .close() calls (RawDllInstallFailsIfForeignFileExists, InstallLibNameAlias, AlterFromNonZipToZip). 4. ErrorMessagePopulatedOnFailureTest mode 2 was checking for our ValidateRelativePath exception text, but .NET's ZipFile.ExtractToDirectory built-in zip-slip guard fires first with its own message ('outside the specified destination directory'). Updated assertion to match what actually surfaces. Also: CMakeLists.txt was passing '--std=c++17' (a GCC/Clang flag, silently ignored by MSVC), so the test suite was actually building at MSVC's default C++14 the whole time. Switched to a generator expression that emits '/std:c++17' for MSVC and '--std=c++17' for non-MSVC compilers. Required to make D1 (std::filesystem) work. Local test run: 111 passed, 0 failed (was 75 passed / 36 failed pre-fix). --- .../src/managed/CSharpExtension.cs | 55 +++++++++++++------ .../test/src/native/CMakeLists.txt | 5 +- .../test/src/native/CSharpLibraryTests.cpp | 16 ++++-- 3 files changed, 53 insertions(+), 23 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 571db929..4810e141 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -778,8 +778,8 @@ public static short InstallExternalLibrary( if (IsZipFile(libFilePath)) { - tempFolder = InstallZipPackage(libFilePath, installDir, libName, - manifestPath, oldManifestEntries); + InstallZipPackage(libFilePath, installDir, libName, + manifestPath, oldManifestEntries, out tempFolder); } else { @@ -858,11 +858,13 @@ private static void InstallRawDll( /// then cleans up the previous version and copies the new content /// into . /// - /// - /// The path of the temp folder used for staging, so the caller can - /// clean it up in its outer finally regardless of whether the - /// install succeeded or threw. - /// + /// + /// Out-parameter set to the staging tempFolder path AS SOON AS it is + /// chosen, BEFORE any extraction is attempted. This lets the caller + /// clean it up in its outer finally even if the extraction or + /// any subsequent step throws -- otherwise a half-extracted tempFolder + /// would leak inside installDir. + /// /// /// A corrupt ZIP leaves the existing install intact (validation runs /// against the staged copy in tempFolder, before we touch installDir). @@ -871,14 +873,18 @@ private static void InstallRawDll( /// directory inconsistent; SQL Server's catalog-based recovery /// re-installs from the catalog on the next session. /// - private static string InstallZipPackage( + private static void InstallZipPackage( string libFilePath, string installDir, string libName, string manifestPath, - HashSet oldManifestEntries) + HashSet oldManifestEntries, + out string tempFolder) { - string tempFolder = Path.Combine(installDir, Guid.NewGuid().ToString()); + // Publish tempFolder to the caller BEFORE doing any work that + // could throw, so the caller's finally can clean it up regardless + // of where we fail (extract throw, conflict throw, copy throw). + tempFolder = Path.Combine(installDir, Guid.NewGuid().ToString()); ZipFile.ExtractToDirectory(libFilePath, tempFolder); if (Directory.GetFiles(tempFolder).Length == 0 && @@ -935,8 +941,6 @@ private static string InstallZipPackage( File.WriteAllLines(manifestPath, extractedFiles); Logging.Trace( $"Wrote manifest: {manifestPath} with {extractedFiles.Count} entries"); - - return tempFolder; } /// @@ -1323,11 +1327,19 @@ private static bool IsReparsePoint(string path) /// collides on overwrite:false -- leaving the second library GONE /// with no replacement and no manifest. /// - /// Implementation: open "{installDir}/.install.lock" with - /// FileShare.None. The OS releases the handle on process crash so - /// there is no stale-lock risk. FileOptions.DeleteOnClose removes - /// the lock file when the holder closes its handle, keeping the - /// install directory clean across runs. + /// Implementation: open a sibling file + /// "{installDir}.install.lock" (NOT inside installDir) with + /// FileShare.None. Sibling placement avoids two issues: + /// (a) tests / callers that enumerate installDir don't see a + /// mystery dotfile; + /// (b) `Directory.Delete(installDir, recursive:true)` and + /// fs::remove_all don't race the OS-level DeleteOnClose + /// unlinking of the lock file inside installDir. + /// + /// The OS releases the handle on process crash so there is no + /// stale-lock risk. FileOptions.DeleteOnClose removes the lock + /// file when the holder closes its handle, keeping the install + /// area clean across runs. /// /// Acquisition blocks indefinitely with a 100ms retry interval. /// In the uncontended common case (single session), acquisition @@ -1335,8 +1347,15 @@ private static bool IsReparsePoint(string path) /// private static FileStream AcquireInstallLock(string installDir) { + // Ensure installDir exists so that its parent exists too (the + // lock lives in the parent, not in installDir). Directory.CreateDirectory(installDir); - string lockPath = Path.Combine(installDir, ".install.lock"); + + // Lock file lives ALONGSIDE installDir, not inside it. This keeps + // the lock invisible to enumerations of installDir contents and + // avoids racing fs::remove_all(installDir). + string lockPath = installDir.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + + ".install.lock"; while (true) { diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CMakeLists.txt b/language-extensions/dotnet-core-CSharp/test/src/native/CMakeLists.txt index 4a10e9db..6641c2d3 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CMakeLists.txt +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CMakeLists.txt @@ -23,7 +23,10 @@ add_executable(dotnet-core-CSharp-extension-test ${DOTNETCORE_CSHARP_EXTENSION_TEST_SOURCE_FILES} ) -target_compile_options(dotnet-core-CSharp-extension-test PRIVATE --std=c++17) +target_compile_options(dotnet-core-CSharp-extension-test PRIVATE + "$<$:/std:c++17>" + "$<$>:--std=c++17>" +) # Set the DLLEXPORT variable to export symbols # diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index 4d28d872..e17670d2 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -817,6 +817,8 @@ namespace ExtensionApiTest std::string sourceContents( (std::istreambuf_iterator(sourceStream)), std::istreambuf_iterator()); + aliasStream.close(); + sourceStream.close(); EXPECT_EQ(aliasContents, sourceContents) << "Alias file contents differ from source DLL"; @@ -1074,10 +1076,13 @@ namespace ExtensionApiTest "slip", zipSlip, installDir, msg); EXPECT_EQ(r, SQL_ERROR); EXPECT_FALSE(msg.empty()) << "No error message populated for zip-slip"; - // Message should describe the attack class so the user (or a security - // log scanner) can identify it. ValidateRelativePath throws with - // "contains entry with invalid path" -- match a stable substring. - EXPECT_NE(msg.find("invalid path"), string::npos) + // Message should describe the rejection. The exception comes from + // .NET's ZipFile.ExtractToDirectory built-in zip-slip guard, which + // throws with "outside the specified destination directory". Our + // own ValidateRelativePath defense-in-depth check ("invalid path") + // is never reached because the .NET extractor catches it first. + // Either substring is acceptable -- assert the .NET form. + EXPECT_NE(msg.find("outside"), string::npos) << "Zip-slip message should describe the rejection. Got: " << msg; // Failure mode 3: file-level conflict @@ -1266,6 +1271,8 @@ namespace ExtensionApiTest std::string sourceBytes( (std::istreambuf_iterator(sourceStream)), std::istreambuf_iterator()); + myLibStream.close(); + sourceStream.close(); EXPECT_EQ(myLibBytes, sourceBytes) << "myLib.dll content differs from v2's source -- v1's raw DLL was not cleaned up"; @@ -1579,6 +1586,7 @@ namespace ExtensionApiTest std::ifstream ifs(foreign.string()); std::string contents((std::istreambuf_iterator(ifs)), std::istreambuf_iterator()); + ifs.close(); EXPECT_EQ(contents, "FOREIGN-CONTENT") << "Foreign file was overwritten by failed raw-DLL install"; From 1c40a611d97d7629510438db62a74769f06d7bf9 Mon Sep 17 00:00:00 2001 From: Justin M Date: Tue, 28 Apr 2026 09:55:50 -0700 Subject: [PATCH 14/18] Bump System.Text.Json to 10.0.4; fix stale native comment Pin System.Text.Json to 10.0.4 explicitly in both csprojs so the extension's published output ships the 10.0.4 assembly (overriding the in-box net8 STJ 8.x in the extension's own AssemblyLoadContext). * Microsoft.SqlServer.CSharpExtension.csproj * Microsoft.SqlServer.CSharpExtensionTest.csproj NuGet automatically bumped two companion packages to match: * System.IO.Pipelines -> 10.0.4 * System.Text.Encodings.Web -> 10.0.4 No other transitive bumps were needed. TFM stays net8.0. Also fix stale comment in nativecsharpextension.cpp: "expected to be a zip" -> "may be a ZIP archive or a raw DLL" to match the header and managed code. Verified locally: * dotnet build (production csproj): 0 warnings, 0 errors. * dotnet build (test csproj): 0 warnings, 0 errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/managed/Microsoft.SqlServer.CSharpExtension.csproj | 1 + .../dotnet-core-CSharp/src/native/nativecsharpextension.cpp | 6 +++--- .../managed/Microsoft.SqlServer.CSharpExtensionTest.csproj | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj b/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj index f503877d..78a8d5eb 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj +++ b/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj @@ -13,5 +13,6 @@ + \ No newline at end of file diff --git a/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp b/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp index 20e09d37..4cf117d1 100644 --- a/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp +++ b/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp @@ -385,9 +385,9 @@ static void SetLibraryError( // // Description: // Installs an external library to the specified directory. -// The library file is expected to be a zip containing the library files. -// If it contains an inner zip, that zip is extracted to the install directory. -// Otherwise, all files are copied directly. +// The library file may be a ZIP archive or a raw DLL. +// If a ZIP, and it contains an inner zip, that inner zip is extracted to the +// install directory. Otherwise, all files are copied directly. // // Returns: // SQL_SUCCESS on success, else SQL_ERROR diff --git a/language-extensions/dotnet-core-CSharp/test/src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj b/language-extensions/dotnet-core-CSharp/test/src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj index d0f49b7d..b525f962 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj +++ b/language-extensions/dotnet-core-CSharp/test/src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj @@ -13,6 +13,7 @@ + From a0d7aae51e7d3de46dd6c2aaab027c0205ff8fd8 Mon Sep 17 00:00:00 2001 From: Justin M Date: Tue, 28 Apr 2026 13:16:53 -0700 Subject: [PATCH 15/18] Revert System.Text.Json 10.0.4 pin (keep native comment fix) The STJ 10.0.4 pin causes a runtime version mismatch: the extension loads into the default AssemblyLoadContext via hostfxr, where the shared framework (Microsoft.NETCore.App 8.0.x) already provides STJ 8.0.0.0. A local STJ 10.0.0.0 cannot coexist in the same ALC. Revert the PackageReference additions from both csprojs. Retain the stale-comment fix in nativecsharpextension.cpp ("expected to be a zip" -> "may be a ZIP archive or a raw DLL"). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/managed/Microsoft.SqlServer.CSharpExtension.csproj | 1 - .../src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj | 1 - 2 files changed, 2 deletions(-) diff --git a/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj b/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj index 78a8d5eb..f503877d 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj +++ b/language-extensions/dotnet-core-CSharp/src/managed/Microsoft.SqlServer.CSharpExtension.csproj @@ -13,6 +13,5 @@ - \ No newline at end of file diff --git a/language-extensions/dotnet-core-CSharp/test/src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj b/language-extensions/dotnet-core-CSharp/test/src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj index b525f962..d0f49b7d 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj +++ b/language-extensions/dotnet-core-CSharp/test/src/managed/Microsoft.SqlServer.CSharpExtensionTest.csproj @@ -13,7 +13,6 @@ - From 55d15e34a4c7826ced96cb929cf9d013f9c20998 Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Thu, 30 Apr 2026 07:29:28 -0700 Subject: [PATCH 16/18] Address PR #85 third review pass: A1 native buffer ownership, A2-A7 correctness/robustness, B1 symlink regression test, doc + test cleanups Bug / correctness fixes (Justin + yaelh review): A1 nativecsharpextension.cpp -- replace `new std::string(...)` + `c_str()` with `malloc(len+1)` + `memcpy` so the returned pointer owns its own buffer. Matches the managed `Marshal.AllocHGlobal` contract that ExtHost expects on the other side. Fixes UB where ExtHost would dereference a pointer into a freed `std::string` internal buffer. A2 CSharpExtension.cs -- narrow the `IOException` catch in `EnsureDir` via `when (IsSharingViolation(ex))` filter that inspects HResult for ERROR_SHARING_VIOLATION (32) / ERROR_LOCK_VIOLATION (33). Other IOException variants (DirectoryNotFoundException, PathTooLongException, mid-creation failures) and non-IOException exceptions (UnauthorizedAccess, Argument, Security) now propagate fast instead of being swallowed. A3 DllUtils.cs -- replace `Directory.GetFiles(searchPath, "{name}.*")` + `.Where(...EndsWith(".dll"))` with `Directory.EnumerateFiles(searchPath)` + explicit `Equals(".dll", OrdinalIgnoreCase)` on extension and `Equals(userLibName, OrdinalIgnoreCase)` on stem. No more reliance on Win32 `FindFirstFile` wildcard semantics; eliminates the "*.dll matches foo.dllx" and "Foo.* matches short-name 8.3 alias" over-match quirks. Remarks block documents both quirks for the next reader. A4 + B1 CSharpExtension.cs + CSharpLibraryTests.cpp + test_packages/testpackageK-SYMLINK.zip + test_packages/build-symlink-fixture.ps1 -- collapse the inner-zip and outer-zip install paths through a single `contentRoot` + `IsReparsePoint`-guarded `ExtractContentToInstallDir` helper. Both paths now extract the payload into `tempFolder/inner-content/` first and walk it on disk, instead of one path calling `ZipFile.ExtractToDirectory(innerZip, installDir)` directly. `CollectStagedFiles` simplified to a single on-disk walk; `ExtractContentToInstallDir` simplified to a single code path. B1 regression test `InnerZipFutureSymlinkRejectedTest` uses the new fixture (262 bytes, generated by `build-symlink-fixture.ps1`). Inner zip contains `legitfile.dll` (regular file) + `evil-symlink.dll` (Unix mode `0o120755`, content `/etc/passwd`). Today's .NET ZipFile ignores Unix mode bits and materializes the entry as a regular file, but a future runtime might honor them. Test asserts the future-proofing invariant: install succeeds, `legitfile.dll` lands in installDir, and installDir contains zero reparse points. A5 CSharpExtension.cs -- wrap `File.Delete(libraryFile)` in an `else` branch so the manifest path exclusively owns cleanup for current-version installs. Direct `File.Delete` only runs as a legacy-compat path for libraries installed by pre-PR builds that have no manifest. Comments explain the split. A6 CSharpExtension.cs -- add `s_reservedDeviceNames` HashSet (OrdinalIgnoreCase) at the top of the class with all 22 Windows reserved DOS device names (CON, NUL, AUX, PRN, COM1-COM9, LPT1-LPT9, plus the COM0/LPT0 historical aliases). `ValidateLibraryName` now rejects any libName whose stem (`Path.GetFileNameWithoutExtension`) matches. New rows in `InstallRejectsInvalidLibNameTest`: CON, nul, Aux, PRN, COM1, LPT9, CON.dll, nul.manifest. Rejection is enforced on every OS so behavior stays consistent for libraries moved between hosts. A7 CSharpExtension.cs -- `DetermineAliasSource` now picks the lexicographically-first `.dll` candidate (`string.CompareOrdinal`) instead of the first one returned by `Directory.GetFiles`. Stable across NTFS / ext4 / XFS and across re-installs. B6 CSharpExtension.cs + CSharpLibraryTests.cpp -- replace content-sniff `IsZipFile` with extension-based `HasZipExtension`. `InstallNonZipFileAsRawDllTest` (which asserted that a `.zip` file with non-PK bytes was silently rewritten as `{libName}.dll`) becomes `InstallZipExtensionWithBadContentFailsLoudlyTest` which asserts `SQL_ERROR` + that the user's file is NOT silently installed under a `.dll` rename. Pre-fix behavior would copy `bad-package-ZIP.zip` to `bad-package.dll`; post-fix the user gets a clear error. B12 CSharpExtension.cs -- first check in `ValidateLibraryName` is now `string.IsNullOrWhiteSpace(libName)`. New " " row in `InstallRejectsInvalidLibNameTest`. B13 CSharpExtension.cs -- remove `ValidateRelativePath` entirely. After A4 collapsed both code paths through `ZipFile.ExtractToDirectory` + on-disk walk it had zero callers; the zip-slip defense it provided is now covered by (a) `ZipFile.ExtractToDirectory`'s built-in zip-slip check and (b) the on-disk walk being unable to see entries that escaped the staged tree. Test additions / hardenings: B3 CSharpLibraryTests.cpp -- rename `DirectoryOverlapAllowedTest` -> `NonConflictingFlatFilesCoexistTest`. Comment block tightened to state "Both packages used here are flat (no nested directories), so the test exercises the flat-file coexistence case only -- nested-directory overlap is covered separately by `ManifestListsNestedFilesTest` + `InnerZipFileConflictFailsTest`." B4 CSharpLibraryTests.cpp -- harden `InstallZipWithManyFilesTest` with a per-module existence loop (`Module1.dll` ... `Module50.dll`) plus a comment block with the historical context (the original `EXPECT_EQ(dllCount, 50)` passed legitimately because the old install code created the alias as `{libName}` with no extension -- test asserted what the code did, not what it should do). Documentation: B2 CSharpExtension.cs -- expand `IsReparsePoint` `` block with the concrete `sneaky.dll` -> `/etc/shadow` scenario and the directory-level reparse-point variant. Notes the guard is theoretical against today's .NET (which writes symlink-mode entries as regular files) and links to `InnerZipFutureSymlinkRejectedTest`. B7 CSharpExtension.cs -- trim the `CleanupManifest` catch comment to stop at the diagnostic-trail justification. Style: B5 / B9 / B10 -- blank line between adjacent independent constructs: (1) file-loop / dir-loop split in `ExtractContentToInstallDir`, (2) consecutive `if (e.find("MyLib.dll"))` / `if (e.find("native.dll"))` flag-setters in `ManifestListsNestedFilesTest`, (3) consecutive `if (e.find("testpackageA"))` / `if (e.find("testpackageB"))` flag-setters in `AlterFromZipToZipTest`. `}` followed by `continue;` guards or `return X;` function-tails left alone -- those are idiomatic and adding blank lines there would only add noise. B11 CSharpLibraryTests.cpp -- one-line rationale comments at the first write-site (`sentinelStream`) and the first read-site cluster (`aliasStream` / `sourceStream`) explaining the pattern of explicit `.close()` before `EXPECT_EQ` / `fs::exists` (gtest macro failure paths can run arbitrary code; known-closed streams are easier to debug). Same pattern is used at every other site for consistency. Test results: 112/112 unit tests pass on Windows release config. --- .../src/managed/CSharpExtension.cs | 400 ++++++++++-------- .../src/managed/utils/DllUtils.cs | 33 +- .../src/native/nativecsharpextension.cpp | 24 +- .../test/src/native/CSharpLibraryTests.cpp | 172 +++++++- .../test_packages/build-symlink-fixture.ps1 | 92 ++++ .../test_packages/testpackageK-SYMLINK.zip | Bin 0 -> 262 bytes 6 files changed, 508 insertions(+), 213 deletions(-) create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/build-symlink-fixture.ps1 create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageK-SYMLINK.zip diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 4810e141..2fd93232 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -72,6 +72,21 @@ public static unsafe class CSharpExtension /// private const int s_lockRetryDelayMs = 100; + /// + /// Windows reserved DOS device names. Files and directories with + /// these stems behave specially even on modern NTFS (CreateFile maps + /// "CON" / "C:\path\CON.txt" to the console device, etc.), so we + /// reject them as library names regardless of host OS to keep + /// behavior consistent. + /// + private static readonly HashSet s_reservedDeviceNames = + new HashSet(StringComparer.OrdinalIgnoreCase) + { + "CON", "PRN", "AUX", "NUL", + "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", + "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + }; + /// /// This delegate declares the delegate type of Init. /// @@ -776,7 +791,7 @@ public static short InstallExternalLibrary( string manifestPath = Path.Combine(installDir, libName + ".manifest"); HashSet oldManifestEntries = ReadManifestEntries(manifestPath); - if (IsZipFile(libFilePath)) + if (HasZipExtension(libFilePath)) { InstallZipPackage(libFilePath, installDir, libName, manifestPath, oldManifestEntries, out tempFolder); @@ -894,8 +909,31 @@ private static void InstallZipPackage( "The library archive contains no entries."); } + // Pick the staging directory whose contents will be copied into + // installDir. If the outer ZIP wraps a single inner ZIP at its + // top level (the engine-wrapped pattern), extract that inner ZIP + // into a sibling subfolder of tempFolder and treat IT as the + // content root. The inner ZIP is then walked and copied via the + // same IsReparsePoint-guarded path as the no-inner-zip case -- + // so a future .NET runtime that honors symlink entries on Linux + // (or a switch to a different extraction library) cannot smuggle + // a symlink into installDir, even from an attacker-controlled + // inner ZIP. The sub-folder lives inside tempFolder, so the + // outer caller's finally cleans it up alongside the rest. string innerZipPath = FindInnerZip(tempFolder); - List extractedFiles = CollectStagedFiles(installDir, tempFolder, innerZipPath); + string contentRoot; + if (innerZipPath != null) + { + contentRoot = Path.Combine(tempFolder, "inner-content"); + Directory.CreateDirectory(contentRoot); + ZipFile.ExtractToDirectory(innerZipPath, contentRoot); + } + else + { + contentRoot = tempFolder; + } + + List extractedFiles = CollectStagedFiles(contentRoot); // Reject archives that contain only empty directories (no files). // The earlier "no entries" guard checks for an entirely-empty @@ -931,7 +969,7 @@ private static void InstallZipPackage( CleanupManifest(manifestPath, installDir); } - ExtractContentToInstallDir(installDir, tempFolder, innerZipPath); + ExtractContentToInstallDir(installDir, contentRoot); if (aliasSourceRelPath != null) { @@ -968,36 +1006,25 @@ private static string FindInnerZip(string tempFolder) } /// - /// Builds the list of relative paths that will be installed into - /// , validating each path stays under - /// installDir (defense-in-depth zip-slip check on top of - /// ZipFile.ExtractToDirectory's own validation). + /// Builds the list of relative paths under + /// that will be installed into installDir. Reparse points are skipped + /// (CollectRelativeFiles enforces this) so manifest entries always + /// correspond to a regular file that CopyDirectory will physically copy. /// - private static List CollectStagedFiles( - string installDir, - string tempFolder, - string innerZipPath) + /// + /// Both the "outer ZIP only" and "outer ZIP wrapping inner ZIP" code + /// paths route through this single on-disk walk. For the inner-zip + /// case, contentRoot is the sub-folder into which InstallZipPackage + /// extracted the inner ZIP; for the no-inner-zip case it IS tempFolder + /// itself. Walking the on-disk tree (rather than enumerating ZIP + /// entries) keeps the manifest aligned with what + /// ExtractContentToInstallDir will actually copy after IsReparsePoint + /// filtering. + /// + private static List CollectStagedFiles(string contentRoot) { List extractedFiles = new List(); - if (innerZipPath != null) - { - using (ZipArchive innerArchive = ZipFile.OpenRead(innerZipPath)) - { - foreach (ZipArchiveEntry entry in innerArchive.Entries) - { - if (string.IsNullOrEmpty(entry.Name)) - { - // Directory entry (no filename portion). Skip. - continue; - } - extractedFiles.Add(ValidateRelativePath(installDir, entry.FullName)); - } - } - } - else - { - CollectRelativeFiles(tempFolder, "", extractedFiles); - } + CollectRelativeFiles(contentRoot, "", extractedFiles); return extractedFiles; } @@ -1014,6 +1041,16 @@ private static List CollectStagedFiles( /// "lib/net8.0/{libName}.dll" does NOT make the library discoverable, /// and we still need to create a root-level alias. Only a root-level /// "{libName}.*" suppresses alias creation. + /// + /// When more than one candidate DLL exists (no root-level + /// "{libName}.*" but, say, both "lib/net8.0/foo.dll" and + /// "lib/net6.0/bar.dll" are present), we deterministically pick the + /// first by ordinal sort of the relative path. Without this sort the + /// pick depends on Directory.GetFiles order, which is + /// filesystem-defined: NTFS returns name-sorted, ext4 / XFS return + /// inode-creation-order, and the same ZIP can yield different alias + /// sources on different hosts. Ordinal sort gives stable, repeatable + /// behavior across platforms and across re-installs. /// private static string DetermineAliasSource( string libName, @@ -1037,67 +1074,61 @@ private static string DetermineAliasSource( return null; } } + // Pick the lexicographically-first .dll candidate so the choice + // is stable across hosts/runs (see above). + string chosen = null; foreach (string relPath in extractedFiles) { - if (Path.GetExtension(relPath).Equals(".dll", s_pathComparison)) + if (!Path.GetExtension(relPath).Equals(".dll", s_pathComparison)) { - return relPath; + continue; + } + if (chosen == null || + string.CompareOrdinal(relPath, chosen) < 0) + { + chosen = relPath; } } - return null; + return chosen; } /// - /// Extracts the staged content from (or - /// directly from the inner zip if is - /// non-null) into the live . + /// Copies the staged content from + /// into the live , skipping reparse + /// points at every level. /// + /// + /// Both the "outer ZIP only" and "outer ZIP wrapping inner ZIP" code + /// paths converge here. Inner-ZIP content has already been extracted + /// to a sub-folder of tempFolder by InstallZipPackage; this method + /// then copies it into installDir using the same IsReparsePoint + /// guard at every recursion level. The guards must live at the call + /// site for the top-level entries because CopyDirectory only checks + /// the children it iterates, not its top-level sourceDir argument: + /// a root-level reparse-point file could cause File.Copy to read + /// data from outside the staged tree, and a root-level reparse-point + /// directory could make CopyDirectory recurse out of contentRoot. + /// private static void ExtractContentToInstallDir( string installDir, - string tempFolder, - string innerZipPath) + string contentRoot) { - if (innerZipPath != null) - { - // SAFETY: Extracting directly to the live installDir is currently - // safe because ZipFile.ExtractToDirectory does not restore symlink - // entries as actual symlinks on any platform -- they are written - // as regular files containing the link target text. So even on - // Linux, an attacker-controlled inner ZIP cannot smuggle a symlink - // into installDir via this path today. - // - // If a future .NET version changes this (e.g. honors symlink - // entries on Linux), or if we switch to a different extraction - // library, this path MUST be re-routed through a separate temp - // folder followed by an IsReparsePoint-guarded copy -- mirror - // the non-inner-zip branch below for the pattern. - ZipFile.ExtractToDirectory(innerZipPath, installDir, false); - } - else + foreach (string file in Directory.GetFiles(contentRoot)) { - // Skip reparse points at the root of tempFolder for the same - // reason CopyDirectory does inside subdirectories: a root-level - // symlink could cause File.Copy to copy data from outside the - // staged tree, and a root-level reparse-point directory could - // make CopyDirectory recurse out of tempFolder. CopyDirectory - // only checks the entries it iterates, not its top-level - // sourceDir argument, so the guards live here at the call site. - foreach (string file in Directory.GetFiles(tempFolder)) + if (IsReparsePoint(file)) { - if (IsReparsePoint(file)) - { - continue; - } - File.Copy(file, Path.Combine(installDir, Path.GetFileName(file)), false); + continue; } - foreach (string dir in Directory.GetDirectories(tempFolder)) + File.Copy(file, Path.Combine(installDir, Path.GetFileName(file)), false); + } + + foreach (string dir in Directory.GetDirectories(contentRoot)) + { + if (IsReparsePoint(dir)) { - if (IsReparsePoint(dir)) - { - continue; - } - CopyDirectory(dir, Path.Combine(installDir, Path.GetFileName(dir))); + continue; } + CopyDirectory(dir, Path.Combine(installDir, Path.GetFileName(dir))); } } @@ -1214,15 +1245,23 @@ public static short UninstallExternalLibrary( string manifestPath = Path.Combine(installDir, libName + ".manifest"); if (File.Exists(manifestPath)) { + // Manifest covers everything we own: ZIP-extracted + // files AND -- since C3 -- the single "{libName}.dll" + // entry written by a raw-DLL install. CleanupManifest + // deletes them all. CleanupManifest(manifestPath, installDir); } - - // Non-ZIP installs write a single "{libName}.dll" file and - // no manifest; remove that file directly. - string libraryFile = Path.Combine(installDir, DllFileNameFor(libName)); - if (File.Exists(libraryFile)) + else { - File.Delete(libraryFile); + // Legacy fallback: a library installed by a pre-PR + // version of the extension would have been a raw DLL + // with no manifest. Remove "{libName}.dll" directly + // so older installs can still be uninstalled cleanly. + string libraryFile = Path.Combine(installDir, DllFileNameFor(libName)); + if (File.Exists(libraryFile)) + { + File.Delete(libraryFile); + } } } } @@ -1298,6 +1337,34 @@ private static void CopyDirectory(string sourceDir, string destDir) /// could (a) copy data from outside the staged tree (information leak) /// or (b) cause the recursive copy to escape the staged area entirely /// and write into arbitrary filesystem locations. + /// + /// Concrete attack scenario: a malicious ZIP entry named + /// "sneaky.dll" is packed with Unix mode 0o120755 (symbolic link) + /// and content "/etc/shadow". On a Linux runtime that materializes + /// such entries as real symlinks during extraction, sneaky.dll + /// becomes a symlink in tempFolder pointing at /etc/shadow. + /// Without an IsReparsePoint guard, the subsequent + /// File.Copy(sneaky.dll, installDir/sneaky.dll, false) + /// would follow the link, READ /etc/shadow with the SQL Server + /// service account's privileges, and write its contents into + /// installDir/sneaky.dll -- which is then world-readable to any + /// principal with read access to the library directory. With the + /// guard, the symlink is skipped, the manifest does not list it, + /// and the installed library directory contains only files that + /// originated inside the ZIP. The same reasoning applies at the + /// directory level: a reparse-point directory could redirect + /// CopyDirectory's recursion into /, /home, or any other tree the + /// service account can read, exfiltrating arbitrary file content + /// into installDir. + /// + /// Today's .NET ZipFile.ExtractToDirectory writes Unix-symlink-mode + /// entries as regular files containing the link target text on + /// every platform, so this scenario is theoretical against the + /// current runtime; the guard is in place for the case where a + /// future runtime starts honoring the bits, or we switch to a + /// different extraction library. Regression test: + /// in + /// CSharpLibraryTests.cpp. /// private static bool IsReparsePoint(string path) { @@ -1369,14 +1436,44 @@ private static FileStream AcquireInstallLock(string installDir) bufferSize: 1, FileOptions.DeleteOnClose); } - catch (IOException) + catch (IOException ex) when (IsSharingViolation(ex)) { - // Another process holds the lock. Wait and retry. + // Another holder has the lock; wait and retry. Any other + // IOException subtype (DirectoryNotFoundException, + // PathTooLongException, IO failure mid-creation, etc.) + // and anything outside IOException + // (UnauthorizedAccessException, ArgumentException, + // SecurityException, ...) propagates so that + // non-transient failures fail fast rather than spinning. Thread.Sleep(s_lockRetryDelayMs); } } } + /// + /// Returns true when 's HResult indicates that + /// the underlying file is held by another process with an exclusive + /// share -- i.e. the only situation in which retrying acquisition + /// of the install lock makes sense. + /// + /// + /// Win32 maps two errors here: + /// + /// ERROR_SHARING_VIOLATION (32, 0x80070020) -- another open + /// handle's FileShare flags exclude the requested access. + /// ERROR_LOCK_VIOLATION (33, 0x80070021) -- a byte-range lock + /// conflicts with the requested access. + /// + /// .NET on Linux maps EAGAIN/EWOULDBLOCK/EBUSY to the same HResults + /// when fileshare flags conflict, so the same constants work + /// cross-platform. + /// + private static bool IsSharingViolation(IOException ex) + { + int hr = ex.HResult & 0xFFFF; + return hr == 32 || hr == 33; + } + /// /// Recursively collects all file paths relative to the root directory. /// @@ -1460,7 +1557,13 @@ private static string DllFileNameFor(string libName) /// outside installDir, allowing unintended file reads / writes / /// deletes. Also rejects names that are only an extension /// (e.g. ".dll") because the resulting "{libName}.manifest" paths - /// would be hidden dotfiles on Linux and opaque on both platforms. + /// would be hidden dotfiles on Linux and opaque on both platforms; + /// rejects whitespace-only names because Windows trims trailing + /// whitespace from filenames silently and Linux behavior is + /// surprising at best; and rejects Windows reserved DOS device + /// names (CON, PRN, NUL, AUX, COM1-9, LPT1-9) because CreateFile + /// maps any path ending in those stems to a device handle even on + /// modern NTFS. /// /// /// The library name as supplied via libraryName to @@ -1468,9 +1571,9 @@ private static string DllFileNameFor(string libName) /// private static void ValidateLibraryName(string libName) { - if (string.IsNullOrEmpty(libName)) + if (string.IsNullOrWhiteSpace(libName)) { - throw new ArgumentException("Library name must not be empty."); + throw new ArgumentException("Library name must not be empty or whitespace."); } if (libName.IndexOfAny(new[] { '/', '\\', '\0' }) >= 0 || @@ -1487,51 +1590,49 @@ private static void ValidateLibraryName(string libName) // extension and the resulting "{libName}.manifest" / "{libName}.dll" // paths would be hidden dotfiles on Linux and opaque on both // platforms. - if (string.IsNullOrEmpty(Path.GetFileNameWithoutExtension(libName))) + string stem = Path.GetFileNameWithoutExtension(libName); + if (string.IsNullOrEmpty(stem)) { throw new ArgumentException( $"Library name '{libName}' must not be only an extension."); } + + // Reject Windows reserved DOS device names. CreateFile interprets + // any path whose final stem matches one of these as a handle to + // the corresponding device, which makes "{installDir}/CON.dll" + // and "{installDir}/CON.manifest" both behave unexpectedly. We + // reject on every OS so behavior is consistent for libraries + // moved between hosts. + if (s_reservedDeviceNames.Contains(stem)) + { + throw new ArgumentException( + $"Library name '{libName}' uses a reserved device name."); + } } /// - /// Returns true if the file at appears to be - /// a ZIP archive based on its leading two "magic bytes". + /// Returns true if 's filename ends with the + /// ".zip" extension (case-insensitive). Dispatch is by extension -- + /// not by inspecting file content -- so the registered filename's + /// intent is honored: a file named "foo.zip" is always treated as a + /// ZIP archive (and fails loudly via ZipFile.ExtractToDirectory if + /// the bytes are not a valid archive), and a file named "foo.dll" + /// is always treated as a raw DLL (and copied as-is even if its + /// bytes happen to start with "PK"). /// /// - /// All ZIP variants (.zip, .jar, .war, .docx, etc.) begin with the - /// two-byte signature "PK" (0x50 0x4B), inherited from PKWARE's - /// original 1989 PKZIP format. We sniff content rather than trust - /// the file extension because: - /// - /// - /// Callers can register a library file as e.g. "foo.zip" without - /// the bytes actually being a ZIP archive (the engine doesn't - /// require the registered name to match content). - /// - /// - /// Callers can register a library file with no extension or with a - /// non-".zip" extension (e.g. ".dll", ".bin") whose contents ARE a - /// ZIP archive. - /// - /// - /// The two-byte sniff is intentionally minimal: a file beginning with - /// "PK" but corrupt internally will still reach - /// ZipFile.ExtractToDirectory, which throws a clear exception that - /// the outer catch surfaces via libraryError. The sniff's job is - /// only to dispatch between the raw-DLL and ZIP install paths -- - /// not to validate ZIP integrity. + /// We previously sniffed the leading two "magic bytes" ("PK") to + /// decide. That silently rewrote a malformed "foo.zip" upload into + /// "foo.dll" on disk, hiding upload-corruption bugs from the caller + /// and surprising readers of the install directory. Extension-based + /// dispatch trades one form of robustness (tolerating misnamed + /// files) for a more important one (predictable failures and no + /// silent renaming of user-registered filenames). /// - private static bool IsZipFile(string path) + private static bool HasZipExtension(string path) { - // FileShare.Read so we don't fight with anti-virus scanners, - // backup agents, or another concurrent SQL session that may have - // the source file open for read. - using (FileStream fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read)) - { - byte[] magic = new byte[2]; - return fs.Read(magic, 0, 2) == 2 && magic[0] == 0x50 && magic[1] == 0x4B; - } + return string.Equals(Path.GetExtension(path), ".zip", + StringComparison.OrdinalIgnoreCase); } /// @@ -1559,63 +1660,6 @@ private static HashSet ReadManifestEntries(string manifestPath) return set; } - /// - /// Converts a ZIP entry path to a platform-native relative path and - /// verifies it does not escape - /// (defense-in-depth zip-slip check). - /// - /// - /// Two transformations + one check: - /// - /// - /// Separator normalization. ZIP entries always use forward - /// slashes per the ZIP spec (PKWARE APPNOTE 4.4.17.1). We rewrite - /// '/' to so subsequent - /// / calls - /// produce native paths on Windows ('\\') and Linux ('/'). We do - /// NOT touch backslashes -- they are illegal in ZIP entry names per - /// spec, and on Linux a backslash is a legitimate filename - /// character that must be preserved as-is. - /// - /// - /// Canonicalization via . - /// Resolves any "..", ".", or symlink-style segments in both - /// installDir and the combined path so the StartsWith check below - /// is performed on absolute, canonical paths. - /// - /// - /// Containment check. The combined path must start with - /// {fullInstall}{DirectorySeparatorChar}. Comparing with the - /// trailing separator is critical: without it, an installDir of - /// "C:\install" would falsely accept entries that resolve to - /// "C:\installEvil\..." (sibling directory with shared prefix). - /// - /// - /// .NET's ZipFile.ExtractToDirectory already performs its own - /// zip-slip check on extraction (since .NET Core 2.1). This - /// function is defense in depth: the manifest we write must list - /// only paths inside installDir, so that uninstall's - /// File.Delete(...) calls cannot be tricked into deleting - /// arbitrary files via a malicious manifest entry that survived - /// extraction. - /// - private static string ValidateRelativePath(string installDir, string zipEntryFullName) - { - string relPath = zipEntryFullName.Replace('/', Path.DirectorySeparatorChar); - string fullInstall = Path.GetFullPath(installDir); - string fullCombined = Path.GetFullPath(Path.Combine(fullInstall, relPath)); - string sep = Path.DirectorySeparatorChar.ToString(); - string prefix = fullInstall.EndsWith(sep) ? fullInstall : fullInstall + sep; - - if (!fullCombined.StartsWith(prefix, s_pathComparison)) - { - throw new InvalidOperationException( - $"Library archive contains entry with invalid path: '{zipEntryFullName}'."); - } - - return relPath; - } - // Throws if any staged relative path collides with an existing file that is not // owned by the previous install of this same library. private static void CheckForConflicts( @@ -1667,9 +1711,7 @@ private static void CleanupManifest(string manifestPath, string installDir) { // A malformed manifest entry shouldn't abort the rest of // the cleanup, but it must leave a diagnostic trail so - // orphaned files don't disappear silently. Use Error - // (not Trace) -- the comment promises a trail and Trace - // is typically off in production deployments. + // orphaned files don't disappear silently. Logging.Error( $"CleanupManifest: skipping manifest entry '{relPath}': {ex.Message}"); continue; diff --git a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs index 6171f5de..9abe008c 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs @@ -110,9 +110,9 @@ public static List CreateDllList( /// /// Adds DLL matches for under /// . Tries the exact name first (so callers - /// that pass "Foo.dll" resolve correctly) and falls back to the - /// "{name}.*" wildcard for bare names (so callers that pass "Foo" - /// still match "Foo.dll", "Foo.runtimeconfig.json", etc.). + /// that pass "Foo.dll" resolve correctly) and falls back to all .dll + /// files whose stem equals (so callers + /// that pass "Foo" still match "Foo.dll"). /// /// /// Directory to search. Returns immediately if null/empty or absent; @@ -128,6 +128,18 @@ public static List CreateDllList( /// Output list to append discovered .dll paths to. Caller-owned; /// AddMatches never clears or replaces. /// + /// + /// We deliberately avoid 's + /// search-pattern argument and its Win32 FindFirstFile + /// wildcard semantics, which over-match in two well-known ways on + /// Windows: "*.dll" matches files like "foo.dllx" + /// (3-char-extension prefix-match quirk inherited from FAT), and + /// "Foo.*" can spuriously match short-name (8.3) aliases of + /// long-named files. Enumerating all entries and filtering with + /// equality on + /// both the extension and the stem gives exact, predictable + /// matches and avoids loading anything we did not intend. + /// private static void AddMatches(string searchPath, string userLibName, List dllList) { if (string.IsNullOrEmpty(searchPath) || !Directory.Exists(searchPath)) @@ -142,13 +154,14 @@ private static void AddMatches(string searchPath, string userLibName, List f.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))); + foreach (string f in Directory.EnumerateFiles(searchPath)) + { + if (Path.GetExtension(f).Equals(".dll", StringComparison.OrdinalIgnoreCase) && + Path.GetFileNameWithoutExtension(f).Equals(userLibName, StringComparison.OrdinalIgnoreCase)) + { + dllList.Add(f); + } + } } /// diff --git a/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp b/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp index 4cf117d1..6985d144 100644 --- a/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp +++ b/language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp @@ -367,9 +367,27 @@ static void SetLibraryError( // Length excludes null terminator -- ExtHost adds +1 when copying // (see Utf8ToNullTerminatedUtf16Le / strcpy_s in the host). *libraryErrorLength = static_cast(errorString.length()); - std::string *pError = new std::string(errorString); - *libraryError = const_cast( - reinterpret_cast(pError->c_str())); + + // Ownership of the buffer transfers to ExtHost; it frees the pointer + // via the C runtime (free()) after consuming the message. Mirrors + // the managed SetLibraryError (CSharpExtension.cs), which uses + // Marshal.AllocHGlobal for the same reason. The previous + // implementation -- new std::string(errorString) + returning + // c_str() -- leaked the string AND handed back a pointer into a + // std::string's internal buffer, which is undefined behavior the + // moment ExtHost calls free() on it. + size_t bufLen = errorString.length() + 1; + char *buf = static_cast(malloc(bufLen)); + if (buf == nullptr) + { + // Out of memory; surface a "no error" state rather than crash + // (the original failure will still be logged by the caller). + *libraryError = nullptr; + *libraryErrorLength = 0; + return; + } + memcpy(buf, errorString.c_str(), bufLen); + *libraryError = reinterpret_cast(buf); } else { diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index e17670d2..c676a880 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -228,6 +228,76 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + //---------------------------------------------------------------------------------------------- + // Name: InnerZipFutureSymlinkRejectedTest + // + // Description: + // Regression for PR #85 / option α. The inner-zip install path must + // route through tempFolder + IsReparsePoint-guarded CopyDirectory -- + // NOT a direct ZipFile.ExtractToDirectory(innerZip, installDir) call. + // testpackageK-SYMLINK.zip is an outer zip whose inner zip contains: + // + // legitfile.dll regular file + // evil-symlink.dll Unix mode 0o120755 (symbolic link, target + // "/etc/passwd") + // + // Today's .NET ZipFile.ExtractToDirectory ignores Unix mode bits on + // every platform: it writes evil-symlink.dll as a regular file + // containing the literal text "/etc/passwd". A future .NET runtime + // (or a switch to a different extraction library) might honor those + // bits and create a real symlink in the staged tree -- at which point + // a direct extract-to-installDir would silently land an attacker- + // controlled symlink in the live library directory. + // + // This test asserts the future-proofing invariant: regardless of how + // the runtime materializes the symlink-mode entry, installDir must + // contain NO reparse points after install. If the runtime starts + // honoring the bits, the IsReparsePoint guard in CopyDirectory must + // skip evil-symlink.dll; if the runtime keeps writing it as a regular + // file, the test still passes (no reparse points exist). The test + // fails only if BOTH (a) the runtime honors the bits AND (b) the + // IsReparsePoint guard regresses. + // + // Also asserts that install completes successfully and that the + // legitimate file lands in installDir, so a regression that simply + // fails the install on encountering a symlink-mode entry is also + // caught. + // + TEST_F(CSharpExtensionApiTests, InnerZipFutureSymlinkRejectedTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageK-SYMLINK.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) + << "Symlink fixture missing -- regenerate with build-symlink-fixture.ps1"; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "symlinklib", packagePath, installDir); + ASSERT_EQ(result, SQL_SUCCESS) + << "Install must succeed -- symlink-mode entries are skipped, not fatal"; + + // The legitimate file must be present (proves install actually ran). + EXPECT_TRUE(fs::exists(fs::path(installDir) / "legitfile.dll")) + << "Expected legitfile.dll missing -- inner-zip path may not have run"; + + // The future-proofing invariant: nothing in installDir is a reparse + // point. If a future runtime materializes evil-symlink.dll as a + // real symlink in tempFolder, the IsReparsePoint guard in + // CopyDirectory must skip it so installDir stays clean. + std::error_code ec; + for (const auto &entry : fs::recursive_directory_iterator(installDir, ec)) + { + // is_symlink follows std::filesystem semantics: returns true + // for both file and directory symlinks on Linux, and for + // SYMLINK / SYMLINKD reparse points on Windows. + EXPECT_FALSE(fs::is_symlink(entry.symlink_status())) + << "installDir contains a symlink (regression): " << entry.path(); + } + + CleanupInstallDir(installDir); + } + //---------------------------------------------------------------------------------------------- // Name: InstallZipContainingDllsTest // @@ -261,21 +331,19 @@ namespace ExtensionApiTest } //---------------------------------------------------------------------------------------------- - // Name: InstallNonZipFileAsRawDllTest + // Name: InstallZipExtensionWithBadContentFailsLoudlyTest // // Description: - // Tests that installing a file with .zip extension that is NOT actually - // a valid ZIP (magic bytes are not 'PK') falls through to the raw-DLL - // install path: the file is copied to the install directory as - // "{libName}.dll" and SQL_SUCCESS is returned. This pins the contract - // that IsZipFile detects content (not file extension). - // - // NOTE: this does NOT exercise corrupt-ZIP handling. A genuinely - // invalid ZIP would start with 'PK' but be malformed inside, causing - // ZipFile.ExtractToDirectory to throw on the ZIP path. We have no - // fixture for that case today. - // - TEST_F(CSharpExtensionApiTests, InstallNonZipFileAsRawDllTest) + // Tests that installing a file with the .zip extension whose bytes + // are NOT a valid ZIP archive returns SQL_ERROR. Dispatch is by + // extension (not by content sniff), so a file the user named + // "bad-package-ZIP.zip" must be treated as a ZIP -- and if its bytes + // are not a real archive, ZipFile.ExtractToDirectory throws and the + // install must fail loudly. We must NOT silently rewrite the user's + // upload into a "{libName}.dll" raw install (which the previous + // content-sniff implementation did). + // + TEST_F(CSharpExtensionApiTests, InstallZipExtensionWithBadContentFailsLoudlyTest) { string packagesPath = GetPackagesPath(); string packagePath = (fs::path(packagesPath) / "bad-package-ZIP.zip").string(); @@ -285,10 +353,13 @@ namespace ExtensionApiTest SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, "bad-package", packagePath, installDir); - EXPECT_EQ(result, SQL_SUCCESS); + EXPECT_EQ(result, SQL_ERROR); - EXPECT_TRUE(fs::exists(fs::path(installDir) / "bad-package.dll")) - << "Raw file not found in install directory as bad-package.dll"; + // The user's file must NOT have been silently installed under a + // ".dll" rename. Pre-fix behavior was to copy bad-package-ZIP.zip + // to "bad-package.dll" -- assert that did not happen. + EXPECT_FALSE(fs::exists(fs::path(installDir) / "bad-package.dll")) + << "Bad ZIP should not be silently rewritten as bad-package.dll"; CleanupInstallDir(installDir); } @@ -556,6 +627,19 @@ namespace ExtensionApiTest // DLL as an alias named "manyfilespackage.dll". The DLL count in // installDir is therefore 50 (extracted) + 1 (alias) = 51. // + // Historical note: a pre-PR version of this test asserted + // EXPECT_EQ(dllCount, 50) and EXPECT_TRUE(fs::exists(... / + // "manyfilespackage")) (alias with no .dll extension). It passed + // legitimately at the time because the install code created the + // alias as "{libName}" with no extension -- so dllCount stayed at 50 + // and the no-extension EXPECT_TRUE matched. Test and code agreed, + // but the alias was un-loadable as a DLL on Windows. The test + // caught no regression in alias naming because its assertions were + // written to match the buggy behavior. Current assertions + // (51 + .dll extension + per-module name check below) pin the + // correct shape so a future regression toward "alias with wrong + // extension" or "extracted file silently dropped" is caught. + // TEST_F(CSharpExtensionApiTests, InstallZipWithManyFilesTest) { string packagesPath = GetPackagesPath(); @@ -580,7 +664,21 @@ namespace ExtensionApiTest EXPECT_EQ(dllCount, 51) << "Expected 51 DLL files (50 extracted + 1 alias), found " << dllCount; - // Verify the alias exists so DllUtils can discover the library by name. + // Per-module existence check. Catches a "silently dropped one + // extracted file but added another spurious .dll so the count + // still totals 51" regression that the bare count above would miss. + for (int i = 1; i <= 50; ++i) + { + string moduleName = "Module" + std::to_string(i) + ".dll"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / moduleName)) + << "Expected extracted module missing: " << moduleName; + } + + // Verify the alias exists with the .dll extension so DllUtils can + // discover the library by name. The .dll extension is critical: + // an alias without it would still be findable by DllUtils's + // "{libName}.*" wildcard but would be un-loadable as a DLL on + // Windows, which is exactly the bug the historical test missed. EXPECT_TRUE(fs::exists(fs::path(installDir) / "manyfilespackage.dll")); CleanupInstallDir(installDir); @@ -762,6 +860,7 @@ namespace ExtensionApiTest { hasNestedDll = true; } + if (e.find("native.dll") != string::npos && e.find("win-x64") != string::npos) { @@ -817,6 +916,12 @@ namespace ExtensionApiTest std::string sourceContents( (std::istreambuf_iterator(sourceStream)), std::istreambuf_iterator()); + // Explicit close (rather than relying on RAII at end-of-scope) so + // the file handles are released before the EXPECT_EQ comparison + // -- gtest macros that fail can run arbitrary code in the + // diagnostic path, and the comparison itself is cheaper to debug + // when the streams are known-closed. Same pattern is used at + // every read/write site below. aliasStream.close(); sourceStream.close(); EXPECT_EQ(aliasContents, sourceContents) @@ -840,14 +945,17 @@ namespace ExtensionApiTest } //---------------------------------------------------------------------------------------------- - // Name: DirectoryOverlapAllowedTest + // Name: NonConflictingFlatFilesCoexistTest // // Description: // Installing two libraries into the same install directory succeeds - // as long as they do not share any filenames. Directory (folder) - // overlap is explicitly permitted. + // as long as they do not share any filenames. Both packages used here + // are flat (no nested directories), so the test exercises the + // flat-file coexistence case only -- nested-directory overlap is + // covered separately by ManifestListsNestedFilesTest + + // InnerZipFileConflictFailsTest. // - TEST_F(CSharpExtensionApiTests, DirectoryOverlapAllowedTest) + TEST_F(CSharpExtensionApiTests, NonConflictingFlatFilesCoexistTest) { string packagesPath = GetPackagesPath(); string pkgA = (fs::path(packagesPath) / "testpackageA-ZIP.zip").string(); @@ -1033,6 +1141,7 @@ namespace ExtensionApiTest { hasA = true; } + if (e.find("testpackageB") != string::npos) { hasB = true; @@ -1440,6 +1549,11 @@ namespace ExtensionApiTest fs::path sentinel = sentinelDir / "do-not-delete.manifest"; std::ofstream sentinelStream(sentinel.string()); sentinelStream << "sentinel"; + // Close before fs::exists -- the assertion only checks the dir + // entry, but planting the file via an unflushed stream and then + // exercising uninstall would race against the OS commit. Keep + // the explicit close-before-assert pattern at every write site + // for consistency with the byte-comparison sites further down. sentinelStream.close(); ASSERT_TRUE(fs::exists(sentinel)); @@ -1653,10 +1767,12 @@ namespace ExtensionApiTest // side (uninstall is covered separately by // UninstallRejectsPathTraversalLibNameTest): // - empty string + // - whitespace-only // - path-traversal segment (".." resolved against installDir) // - embedded null character // - absolute path // - extension-only (covered also by InstallRejectsExtensionOnlyLibNameTest) + // - Windows reserved DOS device names (CON, NUL, COM1, LPT1, ...) // A regression in ValidateLibraryName must trip at least one of these // cases. // @@ -1675,6 +1791,7 @@ namespace ExtensionApiTest // so the embedded NUL survives. CallInstall forwards libName.length(). Case cases[] = { { string(""), "empty" }, + { string(" "), "whitespace-only" }, { string("../escape"), "path-traversal" }, { string("foo\0bar", 7), "null-character" }, #ifdef _WIN32 @@ -1683,6 +1800,19 @@ namespace ExtensionApiTest { string("/etc/foo"), "absolute-path-posix" }, #endif { string(".dll"), "extension-only" }, + // Reserved DOS device names. ValidateLibraryName checks the + // stem (Path.GetFileNameWithoutExtension), so both bare "CON" + // and "CON.dll" / "nul.txt" must be rejected. Mixed case must + // also be rejected (s_reservedDeviceNames uses + // OrdinalIgnoreCase). + { string("CON"), "reserved-device-CON" }, + { string("nul"), "reserved-device-nul-lower" }, + { string("Aux"), "reserved-device-Aux-mixed" }, + { string("PRN"), "reserved-device-PRN" }, + { string("COM1"), "reserved-device-COM1" }, + { string("LPT9"), "reserved-device-LPT9" }, + { string("CON.dll"), "reserved-device-CON-with-ext" }, + { string("nul.manifest"), "reserved-device-nul-with-ext" }, }; for (const Case &c : cases) diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/build-symlink-fixture.ps1 b/language-extensions/dotnet-core-CSharp/test/test_packages/build-symlink-fixture.ps1 new file mode 100644 index 00000000..947b8f84 --- /dev/null +++ b/language-extensions/dotnet-core-CSharp/test/test_packages/build-symlink-fixture.ps1 @@ -0,0 +1,92 @@ +# Builds testpackageK-SYMLINK.zip -- a regression fixture for PR #85. +# +# Layout: outer zip wrapping a single inner zip ("inner.zip" at root). The +# inner zip contains: +# +# legitfile.dll regular file, plain content +# evil-symlink.dll Unix mode 0o120755 (symbolic link), payload "/etc/passwd" +# +# The Unix-mode bits live in the central directory entry's external +# attributes. Today's .NET ZipFile.ExtractToDirectory ignores those bits on +# every platform and writes evil-symlink.dll as a regular file containing +# the literal text "/etc/passwd". The test that uses this fixture +# (InnerZipFutureSymlinkRejectedTest) asserts a future-proofing invariant: +# regardless of how a future .NET runtime decides to materialize that entry, +# the installed library directory must contain NO reparse points. +# +# Run once from the test_packages directory; checks the fixture in. + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +Add-Type -AssemblyName System.IO.Compression +Add-Type -AssemblyName System.IO.Compression.FileSystem + +$here = Split-Path -Parent $MyInvocation.MyCommand.Path +$outerPath = Join-Path $here 'testpackageK-SYMLINK.zip' +$innerTempPath = [System.IO.Path]::Combine( + [System.IO.Path]::GetTempPath(), + [System.Guid]::NewGuid().ToString() + '.zip') + +# Unix mode 0o120755 (symbolic link, owner rwx, group rx, other rx) packed +# into the high 16 bits of the 32-bit external attributes field. The .NET +# ExternalAttributes property is a signed Int32. We parse the hex value as +# Int32 directly so PowerShell's "literal must fit in target type" rule is +# never triggered: 0xA1ED0000 has its sign bit set, so it parses to the +# negative value -1578237952, which is the correct two's-complement +# representation in the central directory. +[int]$externalAttrsSymlink = [int]::Parse( + 'A1ED0000', + [System.Globalization.NumberStyles]::HexNumber) + +# --- Build the inner zip --- +$innerStream = [System.IO.File]::Create($innerTempPath) +try { + $innerArchive = New-Object System.IO.Compression.ZipArchive( + $innerStream, [System.IO.Compression.ZipArchiveMode]::Create) + try { + # Regular file -- proves install completed (it must end up in installDir). + $regular = $innerArchive.CreateEntry('legitfile.dll') + $regWriter = New-Object System.IO.StreamWriter($regular.Open()) + try { $regWriter.Write('regular DLL content') } finally { $regWriter.Dispose() } + + # Unix-symlink-mode entry. Today's .NET writes this as a regular + # file; a future .NET that honors the mode bits would write a real + # symlink. The IsReparsePoint guard in CopyDirectory must skip it + # in the latter case. + $symlink = $innerArchive.CreateEntry('evil-symlink.dll') + $symlink.ExternalAttributes = $externalAttrsSymlink + $symWriter = New-Object System.IO.StreamWriter($symlink.Open()) + try { $symWriter.Write('/etc/passwd') } finally { $symWriter.Dispose() } + } finally { + $innerArchive.Dispose() + } +} finally { + $innerStream.Dispose() +} + +# --- Build the outer zip wrapping inner.zip --- +if (Test-Path $outerPath) { Remove-Item $outerPath -Force } +$outerStream = [System.IO.File]::Create($outerPath) +try { + $outerArchive = New-Object System.IO.Compression.ZipArchive( + $outerStream, [System.IO.Compression.ZipArchiveMode]::Create) + try { + $entry = $outerArchive.CreateEntry('inner.zip') + $entryStream = $entry.Open() + try { + $innerBytes = [System.IO.File]::ReadAllBytes($innerTempPath) + $entryStream.Write($innerBytes, 0, $innerBytes.Length) + } finally { + $entryStream.Dispose() + } + } finally { + $outerArchive.Dispose() + } +} finally { + $outerStream.Dispose() +} + +Remove-Item $innerTempPath -Force + +Write-Host "Wrote: $outerPath ($((Get-Item $outerPath).Length) bytes)" diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageK-SYMLINK.zip b/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageK-SYMLINK.zip new file mode 100644 index 0000000000000000000000000000000000000000..f52cd3fd2a011fe293dcab6cd52afb076fb8f205 GIT binary patch literal 262 zcmWIWW@Zs#U|`^2SYkRa=JMsa?vsE#F-8UkP9U9`mzP?kSCv`7{UI$)Ffk$FK|(~5 zmHxA@iOxkj0PRQOVb7XY& Date: Thu, 30 Apr 2026 07:29:55 -0700 Subject: [PATCH 17/18] Address PR #85 B8: explain why ExecuteInvalidLibraryNameScriptTest cannot revert to the pre-PR literal yaelh asked to revert sicongliu's `"NonExistentLibrary"` rename for git-blame continuity. Tried it locally; the test fails: [ RUN ] CSharpExtensionApiTests.ExecuteInvalidLibraryNameScriptTest Hello .NET Core CSharpExtension! CSharpExecuteTests.cpp(125): error: Expected equality of these values: result Which is: 0 (-1) Which is: -1 CSharpExecuteTests.cpp(127): error: Value of: error.find("Unable to find user dll under") != string::npos Actual: false Expected: true The pre-PR literal `"Microsoft.SqlServer.CSharpExtensionTest"` is the basename of `m_UserLibName` (`"Microsoft.SqlServer.CSharpExtensionTest.dll"`), so the loader now resolves it successfully to the real test DLL and the test no longer observes the expected error. Sicongliu's rename was load-bearing, not cosmetic. Keep `"NonExistentLibrary"` and add a 4-line code comment at the call site explaining why, so a future reader (or another reviewer) does not attempt the same revert. Test results: 112/112 unit tests pass on Windows release config. --- .../test/src/native/CSharpExecuteTests.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp index d190a835..f7d492ed 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp @@ -103,7 +103,10 @@ namespace ExtensionApiTest // TEST_F(CSharpExtensionApiTests, ExecuteInvalidLibraryNameScriptTest) { - // Use a library name that doesn't match any DLL file in the library path. + // Use a library name that cannot resolve to any DLL on the search path. + // The pre-PR literal "Microsoft.SqlServer.CSharpExtensionTest" is the basename of + // m_UserLibName ("Microsoft.SqlServer.CSharpExtensionTest.dll"), so the loader now + // resolves it successfully and the test would fail to observe the expected error. // string userLibName = "NonExistentLibrary"; string scriptString = userLibName + m_Separator + m_UserClassFullName; From 21976c5cfeeaaef71e658698d53947f0ed51c34f Mon Sep 17 00:00:00 2001 From: Stuart Padley Date: Wed, 13 May 2026 14:06:00 -0700 Subject: [PATCH 18/18] PR #85 review pass 4: production fixes + reviewer items Production fixes (end-to-end testing against SQL Server 2025 RTM-GDR): * CSharpExtension.cs AcquireInstallLock: place install.lock under Path.Combine(installDir, "install.lock") instead of one level up so concurrent installs into different / slots don't serialize against one another. * CSharpExtension.cs new DispatchAsZip(libName, libFilePath): install dispatch is now driven by the registered library name's extension (.zip -> ZIP, .dll -> raw DLL, otherwise fall back to libFilePath's extension). ExtHost passes a generated temp filename with no semantic suffix in production, so the previous extension-sniff on libFilePath was meaningless there; the libFilePath fallback preserves the legacy contract for test fixtures registered under a bare library name. * CSharpOutputDataSet.cs + utils/Sql.cs DotNetNVarChar plumbing: add the DotNetNVarChar row to Sql.DataTypeSize and a DotNetNVarChar case alongside DotNetWChar in ExtractColumn / GetStrLenNullMap. Previously fell through to default and threw KeyNotFoundException in DataTypeSize before the column reached the dispatch switch. * CSharpOutputDataSet.cs DotNetWChar / DotNetNVarChar Size unit: report Size in BYTES, matching the unit emitted by GetStrLenNullMap (Encoding.Unicode.GetByteCount). The previous code reported a character count, which combined with a byte-count length map caused SPEES to log "Reading one row failed for column N row M. The length information is incorrect." and reject the rowset whenever a string column contained non-ASCII data. Reviewer items (PR #85 May 13 review): * test/src/native/CMakeLists.txt: non-MSVC -std=c++17 (one dash) instead of --std=c++17 to match the documented spelling and the rest of the build tree. * include/nativecsharpextension.h: rewrite InstallExternalLibrary doc comment to spell out the new libName-based dispatch contract and the {libName}.manifest file written for every install (ZIP or raw DLL). * CSharpExtension.cs DetermineAliasSource: alias-suppression now requires an EXACT match against "{libName}.dll" at the install root. The previous prefix check ("{libName}.") suppressed alias creation for ZIPs that planted only sidecars at the root (foo.deps.json etc.) with the real binary nested under lib/net8.0/, leaving the install un-loadable. Drops the now-unused libName parameter from the signature. Pinned by AliasCreatedWhenOnlySidecarsAtRootTest + testpackageL-SIDECAR.zip fixture (build-sidecar-fixture.ps1). * test/src/native/CSharpLibraryTests.cpp new FreeLibError(SQLCHAR *) helper using LocalFree (matches the production Marshal.AllocHGlobal / LocalAlloc allocator that ExtHost uses on the consumer side). Wired into CallInstall, CallUninstall, and CallInstallCaptureError so the test harness no longer leaks the libError buffer on every failing-install assertion. Tests: 113/113 unit tests pass on Windows release config (one new TEST_F: AliasCreatedWhenOnlySidecarsAtRootTest). --- .../include/nativecsharpextension.h | 19 ++- .../src/managed/CSharpExtension.cs | 129 ++++++++++++++---- .../src/managed/CSharpOutputDataSet.cs | 31 ++++- .../src/managed/utils/Sql.cs | 1 + .../test/src/native/CMakeLists.txt | 2 +- .../test/src/native/CSharpLibraryTests.cpp | 124 +++++++++++++++++ .../test_packages/build-sidecar-fixture.ps1 | 80 +++++++++++ .../test_packages/testpackageL-SIDECAR.zip | Bin 0 -> 491 bytes 8 files changed, 353 insertions(+), 33 deletions(-) create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/build-sidecar-fixture.ps1 create mode 100644 language-extensions/dotnet-core-CSharp/test/test_packages/testpackageL-SIDECAR.zip diff --git a/language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h b/language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h index fd2c46c5..0cb369d9 100644 --- a/language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h +++ b/language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h @@ -143,9 +143,22 @@ SQLEXTENSION_INTERFACE SQLRETURN CleanupSession(SQLGUID sessionId, SQLUSMALLINT SQLEXTENSION_INTERFACE SQLRETURN Cleanup(); // Installs an external library to the specified directory. -// The library file is expected to be a zip. If it contains an inner zip, -// that zip is extracted to the install directory. Otherwise, all files -// are copied directly. +// +// Dispatch is by the registered library name (libraryName), not the +// contents of libraryFile: +// - libraryName ending in ".zip" -> ZIP archive install. If the archive +// contains a single inner zip, that inner zip is extracted to the +// install directory; otherwise the outer archive's files are copied +// directly. A "{libName}.manifest" listing every extracted file is +// written so UninstallExternalLibrary can clean up exactly what was +// installed. +// - libraryName ending in ".dll" -> raw DLL install. libraryFile is +// copied verbatim to "{installDir}\{libraryName}" and a one-entry +// manifest is written. +// - libraryName with neither extension -> falls back to libraryFile's +// extension (preserves the legacy contract for callers that register +// libraries by bare name and point libraryFile at a "*.zip" or +// "*.dll" fixture). // SQLEXTENSION_INTERFACE SQLRETURN InstallExternalLibrary( SQLGUID setupSessionId, diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs index 2fd93232..ca7c3865 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs @@ -791,7 +791,22 @@ public static short InstallExternalLibrary( string manifestPath = Path.Combine(installDir, libName + ".manifest"); HashSet oldManifestEntries = ReadManifestEntries(manifestPath); - if (HasZipExtension(libFilePath)) + // Dispatch on the user-registered library name's + // extension first. SQL Server hands the extension a + // staged temp file with a generated name (typically no + // .zip / .dll extension), so libFilePath is not a + // reliable signal of user intent. The only reliable + // signal is libName, which the engine forwards verbatim + // from CREATE EXTERNAL LIBRARY [] -- e.g. a user + // who wrote CREATE EXTERNAL LIBRARY [Foo.dll] gets + // libName = "Foo.dll" and expects a raw-DLL install. + // + // When libName carries no extension (some test fixtures + // register libraries by bare name, pointing libFilePath + // at "foo-DLL.zip" or "foo-RAWDLL.dll"), fall back to + // libFilePath's extension so legacy callers continue to + // work without modification. + if (DispatchAsZip(libName, libFilePath)) { InstallZipPackage(libFilePath, installDir, libName, manifestPath, oldManifestEntries, out tempFolder); @@ -951,7 +966,7 @@ private static void InstallZipPackage( } string aliasFileName = DllFileNameFor(libName); - string aliasSourceRelPath = DetermineAliasSource(libName, aliasFileName, extractedFiles); + string aliasSourceRelPath = DetermineAliasSource(aliasFileName, extractedFiles); if (aliasSourceRelPath != null) { // Append the alias to extractedFiles BEFORE CheckForConflicts @@ -1039,11 +1054,23 @@ private static List CollectStagedFiles(string contentRoot) /// DllUtils.CreateDllList searches only the TOP LEVEL of the library /// path (non-recursive). So a deeply-nested DLL like /// "lib/net8.0/{libName}.dll" does NOT make the library discoverable, - /// and we still need to create a root-level alias. Only a root-level - /// "{libName}.*" suppresses alias creation. + /// and we still need to create a root-level alias. + /// + /// Suppression is intentionally narrow: only a root-level file whose + /// name EXACTLY equals (i.e. + /// "{libName}.dll" -- the file the loader will actually try to map) + /// counts as "already discoverable". An earlier, looser check used + /// name.StartsWith("{libName}."), which incorrectly treated + /// sidecars such as "{libName}.deps.json", "{libName}.runtimeconfig.json", + /// or (in the libName-with-".dll"-suffix case) "{libName}.dll.config" + /// as if they made the library loadable. They do not: the loader + /// resolves "{libName}" to a PE binary by exact filename, so a + /// sidecar at the root with no real DLL there would suppress alias + /// creation and leave the install un-loadable. The exact-match rule + /// keeps suppression aligned with what DllUtils can actually load. /// /// When more than one candidate DLL exists (no root-level - /// "{libName}.*" but, say, both "lib/net8.0/foo.dll" and + /// "{aliasFileName}" but, say, both "lib/net8.0/foo.dll" and /// "lib/net6.0/bar.dll" are present), we deterministically pick the /// first by ordinal sort of the relative path. Without this sort the /// pick depends on Directory.GetFiles order, which is @@ -1053,7 +1080,6 @@ private static List CollectStagedFiles(string contentRoot) /// behavior across platforms and across re-installs. /// private static string DetermineAliasSource( - string libName, string aliasFileName, List extractedFiles) { @@ -1067,10 +1093,12 @@ private static string DetermineAliasSource( continue; } string name = Path.GetFileName(relPath); - if (name.StartsWith(libName + ".", s_pathComparison) || - name.Equals(aliasFileName, s_pathComparison)) + if (name.Equals(aliasFileName, s_pathComparison)) { - // A root-level file matches "{libName}.*" -- already discoverable. + // The file the loader will actually map is already at + // the root -- no alias needed. Sidecars matching + // "{libName}.*" are intentionally NOT treated as + // suppressing: see above. return null; } } @@ -1394,35 +1422,42 @@ private static bool IsReparsePoint(string path) /// collides on overwrite:false -- leaving the second library GONE /// with no replacement and no manifest. /// - /// Implementation: open a sibling file - /// "{installDir}.install.lock" (NOT inside installDir) with - /// FileShare.None. Sibling placement avoids two issues: - /// (a) tests / callers that enumerate installDir don't see a - /// mystery dotfile; - /// (b) `Directory.Delete(installDir, recursive:true)` and - /// fs::remove_all don't race the OS-level DeleteOnClose - /// unlinking of the lock file inside installDir. + /// Implementation: open the file "{installDir}\install.lock" + /// (INSIDE installDir) with FileShare.None. The lock must live + /// inside installDir, not as a sibling of it, because SQL Server's + /// per-database / per-language ExternalLibraries hierarchy ACLs the + /// satellite's AppContainer SID with Modify on the per-user leaf + /// directory only; the parent (e.g. + /// "ExternalLibraries\<dbid>\<langid>\") grants the + /// AppContainer Read access only, so a sibling placement + /// "ExternalLibraries\<dbid>\<langid>\1.install.lock" + /// fails with UnauthorizedAccessException at FileStream creation + /// and the install path can never start. /// /// The OS releases the handle on process crash so there is no /// stale-lock risk. FileOptions.DeleteOnClose removes the lock /// file when the holder closes its handle, keeping the install /// area clean across runs. /// + /// Library names that would collide with the lock filename + /// ("install.lock", or "install" producing an "install.manifest") + /// are not reserved here -- callers should not register such a + /// name. The collision surfaces as a sharing violation at + /// install time rather than silent overwrite. + /// /// Acquisition blocks indefinitely with a 100ms retry interval. /// In the uncontended common case (single session), acquisition /// completes on the first attempt with no measurable overhead. /// private static FileStream AcquireInstallLock(string installDir) { - // Ensure installDir exists so that its parent exists too (the - // lock lives in the parent, not in installDir). + // Ensure installDir exists -- the lock file lives inside it. Directory.CreateDirectory(installDir); - // Lock file lives ALONGSIDE installDir, not inside it. This keeps - // the lock invisible to enumerations of installDir contents and - // avoids racing fs::remove_all(installDir). - string lockPath = installDir.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) - + ".install.lock"; + // Lock file lives INSIDE installDir. See remarks for the ACL + // rationale (the satellite's AppContainer SID has write access + // on installDir only, not on its parent). + string lockPath = Path.Combine(installDir, "install.lock"); while (true) { @@ -1635,6 +1670,52 @@ private static bool HasZipExtension(string path) StringComparison.OrdinalIgnoreCase); } + /// + /// Decides whether to dispatch / + /// through the ZIP install path or + /// the raw-DLL install path. + /// + /// + /// Precedence: + /// + /// If ends in ".zip", dispatch as + /// a ZIP archive. The user wrote + /// CREATE EXTERNAL LIBRARY [Foo.zip]; their intent is + /// unambiguous. + /// If ends in ".dll", dispatch + /// as a raw DLL. The user wrote + /// CREATE EXTERNAL LIBRARY [Foo.dll]; their intent is + /// unambiguous and the staged temp file contents must be a PE + /// binary (any attempt to parse it as a ZIP throws + /// InvalidDataException). + /// Otherwise, fall back to 's + /// extension. SQL Server's ExtHost passes a generated temp file + /// name with no semantic extension; some test fixtures, however, + /// register libraries under a bare name (e.g. + /// "testpackageB") and point libFilePath at a fixture file + /// that does carry a meaningful ".zip" / ".dll" suffix. The + /// fallback preserves backward compatibility for those callers. + /// + /// + /// + private static bool DispatchAsZip(string libName, string libFilePath) + { + string libNameExt = Path.GetExtension(libName); + if (string.Equals(libNameExt, ".zip", + StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + if (string.Equals(libNameExt, ".dll", + StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + return HasZipExtension(libFilePath); + } + /// /// Reads an existing manifest file into a set of relative paths. /// Returns an empty set if the manifest does not exist. diff --git a/language-extensions/dotnet-core-CSharp/src/managed/CSharpOutputDataSet.cs b/language-extensions/dotnet-core-CSharp/src/managed/CSharpOutputDataSet.cs index eeb116e9..bd53e5c4 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/CSharpOutputDataSet.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/CSharpOutputDataSet.cs @@ -190,13 +190,31 @@ DataFrameColumn column SetDataPtrs(columnNumber, GetStringArray(column)); break; case SqlDataType.DotNetWChar: + case SqlDataType.DotNetNVarChar: // Calculate column size from actual data. - // columnSize = max character count (UTF-16 byte length / 2). - // Minimum size is 1 character (nchar(0) is illegal in SQL). + // GetStrLenNullMap returns the per-row byte length + // (Encoding.Unicode.GetByteCount) and the SQL_C_WCHAR + // ODBC contract requires the column Size and the + // strLenOrNullMap entries to use the same unit. Report + // Size in BYTES; do NOT convert to a character count + // here -- a character-count Size combined with a + // byte-count length map causes SPEES to log + // "Reading one row failed for column N row M. The + // length information is incorrect." and reject the + // rowset. + // + // DotNetNVarChar is treated as an alias of DotNetWChar + // (both are SQL_C_WCHAR-shaped at the ODBC layer). + // Without this case, callers who set + // DataTypeMap[typeof(string)] = SqlDataType.DotNetNVarChar + // hit a KeyNotFoundException in DataTypeSize and the + // column never reaches the dispatch switch. + // + // Minimum size is 2 bytes (one UTF-16 code unit -- nchar(0) + // is illegal in SQL). // int maxUnicodeByteLen = colMap.Length > 0 ? colMap.Where(x => x > 0).DefaultIfEmpty(0).Max() : 0; - int maxCharCount = maxUnicodeByteLen / sizeof(char); - _columns[columnNumber].Size = (ulong)Math.Max(maxCharCount, MinUtf16CharSize); + _columns[columnNumber].Size = (ulong)Math.Max(maxUnicodeByteLen, MinUtf16CharSize); SetDataPtrs(columnNumber, GetUnicodeStringArray(column)); break; @@ -429,8 +447,11 @@ private int[] GetStrLenNullMap(ushort columnNumber, DataFrameColumn column) Logging.Trace($"GetStrLenNullMap: Row {rowNumber}, Value='{column[rowNumber]}', ByteLen={colMap[rowNumber]}"); break; case SqlDataType.DotNetWChar: + case SqlDataType.DotNetNVarChar: // Report the byte length of the UTF-16 encoded string (2 bytes per code unit). - // This must match the buffer size emitted by GetUnicodeStringArray(). + // This must match the buffer size emitted by GetUnicodeStringArray() + // and the column Size set in ExtractColumn (also bytes for SQL_C_WCHAR). + // DotNetNVarChar is an alias of DotNetWChar at the ODBC layer. // colMap[rowNumber] = Encoding.Unicode.GetByteCount((string)column[rowNumber]); Logging.Trace($"GetStrLenNullMap: Row {rowNumber}, Value='{column[rowNumber]}', ByteLen={colMap[rowNumber]}"); diff --git a/language-extensions/dotnet-core-CSharp/src/managed/utils/Sql.cs b/language-extensions/dotnet-core-CSharp/src/managed/utils/Sql.cs index 78ee0a9c..491c6935 100644 --- a/language-extensions/dotnet-core-CSharp/src/managed/utils/Sql.cs +++ b/language-extensions/dotnet-core-CSharp/src/managed/utils/Sql.cs @@ -102,6 +102,7 @@ public enum SqlDataType: short {SqlDataType.DotNetBit, sizeof(bool)}, {SqlDataType.DotNetChar, MinUtf8CharSize}, {SqlDataType.DotNetWChar, MinUtf16CharSize}, + {SqlDataType.DotNetNVarChar, MinUtf16CharSize}, {SqlDataType.DotNetNumeric, SqlNumericStructSize} }; diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CMakeLists.txt b/language-extensions/dotnet-core-CSharp/test/src/native/CMakeLists.txt index 6641c2d3..952df261 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CMakeLists.txt +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CMakeLists.txt @@ -25,7 +25,7 @@ add_executable(dotnet-core-CSharp-extension-test target_compile_options(dotnet-core-CSharp-extension-test PRIVATE "$<$:/std:c++17>" - "$<$>:--std=c++17>" + "$<$>:-std=c++17>" ) # Set the DLLEXPORT variable to export symbols diff --git a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp index c676a880..ea163807 100644 --- a/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp +++ b/language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp @@ -73,6 +73,32 @@ namespace ExtensionApiTest } } + // Helper: release an unmanaged error buffer returned from Install/ + // Uninstall External Library. The managed implementation allocates + // these via Marshal.AllocHGlobal (see CSharpExtension.cs::SetLibraryError), + // which on Windows is backed by LocalAlloc -- the matching deallocator + // is LocalFree. Production ExtHost frees the buffer the same way; the + // tests must do so too, otherwise every SQL_ERROR path (and every + // success path, since a null libError is just a no-op LocalFree) leaks + // unmanaged memory. With 113 tests, many of which intentionally trigger + // SQL_ERROR, the leak accumulation was non-trivial across a single + // gtest run. + // + // The native pre-flight path in nativecsharpextension.cpp also allocates + // libError, but uses malloc() and not LocalAlloc. On Windows, LocalFree + // on a malloc'd pointer is undefined behavior. The tests in this file + // only exercise the managed entry points (Install/UninstallExternalLibrary + // in Microsoft.SqlServer.CSharpExtension.dll), so every libError they + // ever see is AllocHGlobal/LocalAlloc'd and LocalFree is correct here. + // + static void FreeLibError(SQLCHAR *libError) + { + if (libError != nullptr) + { + LocalFree(reinterpret_cast(libError)); + } + } + // Helper: call InstallExternalLibrary and check result // static SQLRETURN CallInstall( @@ -95,6 +121,10 @@ namespace ExtensionApiTest &libError, &libErrorLength); + // Release the unmanaged error buffer (if any) before returning, so + // the SQL_ERROR-path tests don't accumulate unmanaged allocations + // across the run. See FreeLibError above for rationale. + FreeLibError(libError); return result; } @@ -117,6 +147,7 @@ namespace ExtensionApiTest &libError, &libErrorLength); + FreeLibError(libError); return result; } @@ -166,6 +197,10 @@ namespace ExtensionApiTest static_cast(libErrorLength)); } + // Release the unmanaged error buffer AFTER copying its contents into + // the std::string. See FreeLibError above for the allocator-pairing + // rationale. + FreeLibError(libError); return result; } @@ -944,6 +979,95 @@ namespace ExtensionApiTest CleanupInstallDir(installDir); } + //---------------------------------------------------------------------------------------------- + // Name: AliasCreatedWhenOnlySidecarsAtRootTest + // + // Description: + // Regression test for the alias-suppression tightening in + // DetermineAliasSource. The fixture testpackageL-SIDECAR.zip contains + // ONLY two root-level sidecars (foo.deps.json, foo.runtimeconfig.json) + // and the actual DLL nested at lib/net8.0/foo.dll. + // + // Pre-fix, DetermineAliasSource matched any root file whose name + // began with "{libName}." (StartsWith). The two sidecars satisfied + // that check, so alias creation was suppressed -- the install ended + // up with no root-level "foo.dll" and DllUtils.CreateDllList (which + // walks only the top level) could not find the library at load time. + // + // Post-fix, suppression requires an EXACT match against the alias + // filename ("foo.dll"). The sidecars no longer suppress, so the + // nested DLL is cloned to the root as the alias and the library is + // loadable. + // + TEST_F(CSharpExtensionApiTests, AliasCreatedWhenOnlySidecarsAtRootTest) + { + string packagesPath = GetPackagesPath(); + string packagePath = (fs::path(packagesPath) / "testpackageL-SIDECAR.zip").string(); + ASSERT_TRUE(fs::exists(packagePath)) + << "Test package not found: " << packagePath; + + string installDir = CreateInstallDir(); + + SQLRETURN result = CallInstall(sm_installExternalLibraryFuncPtr, + "foo", packagePath, installDir); + EXPECT_EQ(result, SQL_SUCCESS); + + // Both sidecars must have been extracted to the root unchanged. + EXPECT_TRUE(fs::exists(fs::path(installDir) / "foo.deps.json")) + << "Sidecar foo.deps.json missing from installDir"; + EXPECT_TRUE(fs::exists(fs::path(installDir) / "foo.runtimeconfig.json")) + << "Sidecar foo.runtimeconfig.json missing from installDir"; + + // The nested DLL must have been extracted intact. + fs::path nestedDll = fs::path(installDir) / "lib" / "net8.0" / "foo.dll"; + ASSERT_TRUE(fs::exists(nestedDll)) + << "Nested DLL lib/net8.0/foo.dll missing from installDir"; + + // Core regression assertion: the alias MUST exist at the root + // (post-fix). Pre-fix, this file was absent and the test would + // fail here, demonstrating the bug. + fs::path aliasFile = fs::path(installDir) / "foo.dll"; + ASSERT_TRUE(fs::exists(aliasFile)) + << "Expected root-level alias 'foo.dll' was not created -- " + << "alias suppression incorrectly fired on a sidecar match"; + + // Alias must be a byte-for-byte copy of the nested DLL. + EXPECT_EQ(fs::file_size(aliasFile), fs::file_size(nestedDll)) + << "Alias size differs from nested DLL"; + + std::ifstream aliasStream(aliasFile.string(), std::ios::binary); + std::ifstream nestedStream(nestedDll.string(), std::ios::binary); + std::string aliasContents( + (std::istreambuf_iterator(aliasStream)), + std::istreambuf_iterator()); + std::string nestedContents( + (std::istreambuf_iterator(nestedStream)), + std::istreambuf_iterator()); + // Explicit close before EXPECT_EQ so the file handles are released + // before any failure-diagnostic code runs (same pattern as + // InstallLibNameAliasTest above). + aliasStream.close(); + nestedStream.close(); + EXPECT_EQ(aliasContents, nestedContents) + << "Alias contents differ from nested DLL"; + + // Manifest should include the alias. + vector entries = ReadManifest( + (fs::path(installDir) / "foo.manifest").string()); + bool hasAlias = false; + for (const string &e : entries) + { + if (e == "foo.dll") + { + hasAlias = true; + break; + } + } + EXPECT_TRUE(hasAlias) << "Manifest missing alias entry 'foo.dll'"; + + CleanupInstallDir(installDir); + } + //---------------------------------------------------------------------------------------------- // Name: NonConflictingFlatFilesCoexistTest // diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/build-sidecar-fixture.ps1 b/language-extensions/dotnet-core-CSharp/test/test_packages/build-sidecar-fixture.ps1 new file mode 100644 index 00000000..508ee130 --- /dev/null +++ b/language-extensions/dotnet-core-CSharp/test/test_packages/build-sidecar-fixture.ps1 @@ -0,0 +1,80 @@ +# Builds testpackageL-SIDECAR.zip -- a regression fixture for PR #85. +# +# Layout: +# +# foo.deps.json root-level sidecar (matches "{libName}." prefix +# but is NOT a loadable DLL) +# foo.runtimeconfig.json root-level sidecar (same shape) +# lib/net8.0/foo.dll the actual DLL, NESTED -- not at root, so +# DllUtils.CreateDllList (top-level only) cannot +# find it without an alias +# +# Regression scenario: with libName="foo", the loader needs a root-level +# "foo.dll". Pre-fix, DetermineAliasSource matched any root file starting +# with "foo." (StartsWith), so the sidecars caused alias creation to be +# suppressed and the install ended up with no loadable DLL at the root. +# Post-fix, DetermineAliasSource only suppresses on EXACT match against +# "foo.dll" (the alias filename) -- so the sidecars no longer suppress, and +# the nested "lib/net8.0/foo.dll" is cloned to the root as the alias. +# +# The companion test (AliasCreatedWhenOnlySidecarsAtRootTest) asserts that +# after install: +# - SQL_SUCCESS, +# - root-level "foo.dll" alias EXISTS, +# - alias is a byte-for-byte copy of "lib/net8.0/foo.dll", +# - both sidecars were extracted to the root, +# - the nested "lib/net8.0/foo.dll" was extracted intact. +# +# Run once from the test_packages directory; checks the fixture in. + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +Add-Type -AssemblyName System.IO.Compression +Add-Type -AssemblyName System.IO.Compression.FileSystem + +$here = Split-Path -Parent $MyInvocation.MyCommand.Path +$outPath = Join-Path $here 'testpackageL-SIDECAR.zip' + +if (Test-Path $outPath) { Remove-Item $outPath -Force } + +# Helper: write a single string entry into the archive at $relPath (forward +# slashes; .NET ZipArchive normalizes correctly). +function Add-TextEntry { + param( + [System.IO.Compression.ZipArchive]$Archive, + [string]$RelPath, + [string]$Text + ) + $entry = $Archive.CreateEntry($RelPath) + $writer = New-Object System.IO.StreamWriter($entry.Open()) + try { $writer.Write($Text) } finally { $writer.Dispose() } +} + +$stream = [System.IO.File]::Create($outPath) +try { + $archive = New-Object System.IO.Compression.ZipArchive( + $stream, [System.IO.Compression.ZipArchiveMode]::Create) + try { + # Two root-level sidecars. Both match "foo." prefix; neither is a + # loadable DLL. The pre-fix StartsWith check would treat either of + # them as proof the library is "already discoverable" and skip + # alias creation -- producing an install with no root-level DLL. + Add-TextEntry -Archive $archive -RelPath 'foo.deps.json' ` + -Text '{"runtimeTarget":{"name":".NETCoreApp,Version=v8.0"}}' + Add-TextEntry -Archive $archive -RelPath 'foo.runtimeconfig.json' ` + -Text '{"runtimeOptions":{"tfm":"net8.0"}}' + + # The actual DLL, NESTED. DllUtils.CreateDllList only walks the top + # level, so without an alias clone at the root this DLL is + # undiscoverable to the loader. + Add-TextEntry -Archive $archive -RelPath 'lib/net8.0/foo.dll' ` + -Text 'fake DLL content for foo' + } finally { + $archive.Dispose() + } +} finally { + $stream.Dispose() +} + +Write-Host "Wrote: $outPath ($((Get-Item $outPath).Length) bytes)" diff --git a/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageL-SIDECAR.zip b/language-extensions/dotnet-core-CSharp/test/test_packages/testpackageL-SIDECAR.zip new file mode 100644 index 0000000000000000000000000000000000000000..c9fef8eff40fadea7931d9afccb2afcf609fe0aa GIT binary patch literal 491 zcmWIWW@Zs#U|`^2Fwa^WqrT$PWLqH56o`3&I4wV4FD130STCzMKW|l-meyJAlV^N+ zJ$1bNbpto83OnU}#y50R(527CyoKlVy-Ia7E^vD5YMncCezWf83&sJf*0KUk`Tw7p zAppgm*BuTIw1G0pKr9Ber>HcqBr`WPIX^EgGac2!ulgD%&z#o|1KOzNc^YWpDR144 zc#M~sbxBqpXsiSf3juLXW|DqhYKeuOfj-zrIXPb5XZ%77K7I`JICoy#^Q?BD$7y}0 zM>RmTP}edti7?;}7mzvxKnbM)Z&a=5UPWlj05U;ZF+GlM2D*O`W*i4HF?|={&B_K+ O!wiJ`fwUMShz9^3DT3Mn literal 0 HcmV?d00001