CSharp extension: ZIP archive support for InstallExternalLibrary/UninstallExternalLibrary

[stuartpa]

Summary

Extends InstallExternalLibrary / UninstallExternalLibrary in the .NET Core CSharp Language Extension to support ZIP archives with arbitrary file trees (e.g. packages with nested folders), not just flat DLL files. Also makes InstallExternalLibrary idempotent so ALTER EXTERNAL LIBRARY works correctly.

What changed vs. sicongliu's base branch

1. Manifest-based uninstall

UninstallExternalLibrary only receives LibraryName + LibraryInstallDirectory — no LibraryFile, so we cannot re-read ZIP entries from sys.external_libraries at drop time. To work around this, InstallExternalLibrary now writes a <libName>.manifest file listing the relative paths of every extracted file. UninstallExternalLibrary reads that manifest and deletes exactly those files — no more, no less.

The previous uninstall implementation deleted everything in the shared install directory, which would wipe out unrelated libraries' files.

2. File-level conflict detection on install

Before extracting a ZIP, every entry is checked against the install directory. If a file of the same name already exists (from another library), install fails with a clear error:

Cannot install library '<X>': file '<Y>' already exists in the install directory.

Directory (folder) overlaps are allowed — multiple libraries can share a parent folder like, they just can't overwrite each other's files.

3. Empty-directory cleanup on uninstall

After a manifest-driven delete, parent directories of removed files are walked deepest-first (sorted by separator count) and removed only if empty. Shared parent folders survive as long as any other library still has content in them.

4. ALTER EXTERNAL LIBRARY support (transactional re-install)

SQL Server may call InstallExternalLibrary again for the same library name during ALTER EXTERNAL LIBRARY without first calling UninstallExternalLibrary. The install now:

1. Extracts and conflict-checks the new content into a temporary staging folder first
2. Only cleans up the previous version's manifest after the new content has validated successfully
3. Suppresses false-positive conflicts between the new version and the old version's own files

If the new ZIP is corrupt or conflicts with another library, the old version is left intact — the install is atomic from the caller's perspective.

5. Defense in depth

• Zip-slip: every extracted path is canonicalized and verified to resolve under the install directory before it is written to the manifest or deleted. ZipFile.ExtractToDirectory already guards against zip-slip on extract; this closes the secondary vector where a malicious entry path could be persisted and later deleted on uninstall.
• Library name validation: rejects .., path separators, null characters, and absolute paths in libName before using it to build on-disk paths.
• Linux case-sensitivity: path comparisons use Ordinal on Linux and OrdinalIgnoreCase on Windows, so /install/Lib and /install/lib are correctly treated as distinct paths on Linux.

Error-handling matrix

Scenario	Behavior
ZIP contains a file that already exists on disk	Install fails with explicit error; no partial state
Two libraries extract to the same folder	Allowed (folder overlap is fine)
Non-ZIP file (raw DLL)	Copied directly as <libName>.dll; no manifest written
Drop library with no manifest	<libName>.dll is deleted directly
ALTER EXTERNAL LIBRARY with valid new content	Old content replaced atomically via manifest
ALTER EXTERNAL LIBRARY with corrupt new content	Old content preserved; error returned
ZIP entry path escapes install dir (zip-slip)	Install fails with explicit error
Library name contains .. or path separator	Install fails with explicit error

Tests

14 new TEST_F cases in CSharpLibraryTests.cpp covering:

• Manifest creation and content (flat + nested paths)
• <libName>.dll alias naming
• Directory overlap allowed, file conflict rejected
• Uninstall preserves unrelated libraries
• Empty nested directory cleanup
• ALTER-style re-install
• Error message surfacing through libraryError
• Non-ZIP uninstall path
• Conflict in inner-ZIP code path
• Temp-folder cleanup on failure
• ALTER from non-ZIP to ZIP
• Alias removed on uninstall

Three existing tests updated to assert the corrected <libName>.dll naming.

[Copilot]

Pull request overview

Adds ZIP-archive support (including nested file trees) for the .NET Core C# language extension’s InstallExternalLibrary / UninstallExternalLibrary, aiming to make installs idempotent and uninstalls precise via a per-library manifest.

Changes:

• Implemented managed InstallExternalLibrary / UninstallExternalLibrary with manifest-driven uninstall, conflict detection, and temp-folder staging.
• Updated DLL discovery logic in DllUtils and added native exports/wiring for the new APIs.
• Added extensive native tests plus new ZIP/DLL test packages for edge cases (zip-slip, empty zips, nested trees, many files, etc.).

Reviewed changes

Copilot reviewed 9 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
language-extensions/dotnet-core-CSharp/src/managed/CSharpExtension.cs	Implements managed install/uninstall logic (ZIP extraction, manifests, conflict checks, alias handling).
language-extensions/dotnet-core-CSharp/src/managed/utils/DllUtils.cs	Adjusts library DLL discovery pattern matching.
language-extensions/dotnet-core-CSharp/src/native/nativecsharpextension.cpp	Exposes native InstallExternalLibrary / UninstallExternalLibrary forwarding into managed code.
language-extensions/dotnet-core-CSharp/include/nativecsharpextension.h	Declares the new native library-management API exports.
language-extensions/dotnet-core-CSharp/test/include/CSharpExtensionApiTests.h	Adds function pointer typedefs and fixture members for install/uninstall APIs.
language-extensions/dotnet-core-CSharp/test/src/native/CSharpExtensionApiTests.cpp	Loads the install/uninstall exports for tests.
language-extensions/dotnet-core-CSharp/test/src/native/CSharpLibraryTests.cpp	Adds comprehensive unit tests for the new behaviors (manifest uninstall, conflicts, aliasing, ALTER-like reinstall, zip-slip, etc.).
language-extensions/dotnet-core-CSharp/test/src/native/CSharpExecuteTests.cpp	Tweaks invalid-library-name test input.
language-extensions/dotnet-core-CSharp/test/test_packages/*.zip / *.dll	Adds new fixture packages (nested layout, many files, zip-slip, bad zip, raw dll, etc.).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

CreateDllList now appends ".*" to userLibName unconditionally. If callers pass an explicit filename like "Microsoft.SqlServer.CSharpExtensionTest.dll", the search pattern becomes "Microsoft.SqlServer.CSharpExtensionTest.dll.*" and will not match the actual DLL, causing runtime failures. Consider searching both the exact name and the wildcard variant (e.g., try userLibName first; if no matches, fall back to userLibName + ".*").

[stuartpa]

Addressed in 246067b. Extracted the match logic into a helper AddMatches that tries the exact name first and falls back to the userLibName + ".*" wildcard only when no exact match exists. Filenames like "Foo.dll" now resolve correctly while bare names like "regex" still match "regex.dll", "regex.runtimeconfig.json", etc.

[Copilot]

UninstallExternalLibrary does not call ValidateLibraryName before using libName to build manifestPath / libraryFile. A malicious or legacy library name containing path separators could make Path.Combine(installDir, libName + ".manifest") resolve outside installDir, allowing unintended file reads/deletes. Validate libName (or otherwise enforce that the computed paths stay under installDir) before touching the filesystem.

Suggested change

	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))
	{
	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));
	string fullInstallDir = Path.GetFullPath(installDir);
	string installDirPrefix = fullInstallDir.EndsWith(Path.DirectorySeparatorChar.ToString(), StringComparison.Ordinal)
	? fullInstallDir
	: fullInstallDir + Path.DirectorySeparatorChar;
	StringComparison pathComparison = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
	? StringComparison.OrdinalIgnoreCase
	: StringComparison.Ordinal;
	string EnsurePathUnderInstallDir(string candidatePath)
	{
	string fullCandidatePath = Path.GetFullPath(candidatePath);
	if (!fullCandidatePath.StartsWith(installDirPrefix, pathComparison))
	{
	throw new ArgumentException("Invalid library name.");
	}
	return fullCandidatePath;
	}
	if (Directory.Exists(fullInstallDir))
	{
	// Check for a manifest written during install that lists
	// all files extracted from the library's ZIP content.
	string manifestPath = EnsurePathUnderInstallDir(Path.Combine(fullInstallDir, libName + ".manifest"));
	if (File.Exists(manifestPath))
	{
	CleanupManifest(manifestPath, fullInstallDir);
	}
	// Non-ZIP installs write a single "{libName}.dll" file and
	// no manifest; remove that file directly.
	string libraryFile = EnsurePathUnderInstallDir(Path.Combine(fullInstallDir, DllFileNameFor(libName)));

[stuartpa]

Addressed in 246067b. UninstallExternalLibrary now calls ValidateLibraryName(libName) immediately after unmarshalling the parameters and before any path construction. ValidateLibraryName already rejects names containing /, \, \0, .., or any absolute-path form. Added regression test UninstallRejectsPathTraversalLibNameTest that plants a sentinel outside installDir and asserts it survives an uninstall call with libName "../do-not-delete".

[Copilot]

Alias creation is not robust for nested ZIP layouts and can break the “arbitrary file tree” goal: Directory.GetFiles(installDir, "*.dll") only searches the top-level, so packages with DLLs under lib/… won’t get a {libName}.dll alias and won’t be discoverable by DllUtils. Also, the alias path isn’t included in conflict detection; if {libName}.dll already exists (from another library), File.Copy will throw after extraction, leaving partial state. Consider (1) searching for DLLs recursively and (2) pre-checking / including the alias path in the same conflict-validation phase before writing into installDir.

[stuartpa]

Addressed in 246067b. Alias handling now: (1) scans the full extractedFiles list (the entire tree, not just top-level) to see whether any file named {libName}.* or exactly {libName}.dll is already present, in which case no alias is created; (2) when an alias IS needed, it is appended to extractedFiles BEFORE CheckForConflicts runs, so an {libName}.dll collision with another library fails fast with no content written to installDir. Added regression test AliasConflictDetectedBeforeExtractionTest that plants shared.dll from library A, tries to install library B with libName "shared", and asserts: (a) install returns SQL_ERROR, (b) no new files landed in installDir (byte-identical file count before/after).

[Copilot]

CopyDirectory uses File.Copy(..., overwrite: true), which can overwrite existing files in shared install directories if the filesystem state changes after conflict-checking (TOCTOU), and it weakens the “no overwrites” guarantee this PR is trying to enforce. Prefer overwrite: false (and rely on the explicit conflict check) so an unexpected existing file causes a controlled failure rather than silent replacement.

Suggested change

	File.Copy(file, destFile, true);
	File.Copy(file, destFile, false);

[stuartpa]

Addressed in 246067b. CopyDirectory now uses File.Copy(file, destFile, false). CheckForConflicts already runs before any copy, so overwrite:false is the correct TOCTOU-safe choice: an unexpected existing file causes a loud IOException rather than silently replacing another library's content.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 17 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Raw-DLL install unconditionally copies to {installDir}\{libName}.dll with overwrite: true and does not run conflict detection. In a shared install directory this can overwrite a file installed by another library (e.g., an existing ZIP-based library extracted foo.dll, then installing a raw DLL with libName foo replaces it). Consider performing conflict checks here and avoiding overwrite unless you can prove the existing file is owned by the same library (or emit a manifest for raw installs).

Suggested change

	// 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))
	{
	CleanupManifest(manifestPath, installDir);
	}
	File.Copy(libFilePath, Path.Combine(installDir, DllFileNameFor(libName)), true);
	// Raw DLL. Only replace an existing file when ownership can be proven
	// via this library's manifest. Otherwise, fail rather than overwrite a
	// potentially unrelated library installed into the shared directory.
	string dllFileName = DllFileNameFor(libName);
	string installedDllPath = Path.Combine(installDir, dllFileName);
	if (File.Exists(manifestPath))
	{
	CleanupManifest(manifestPath, installDir);
	}
	else if (File.Exists(installedDllPath))
	{
	throw new IOException(
	"A library file already exists at the target path, but it is not owned by the library being installed.");
	}
	File.Copy(libFilePath, installedDllPath, false);
	File.WriteAllLines(manifestPath, new[] { dllFileName });

[Copilot]

ALTER-style reinstall from a previous raw-DLL install to a ZIP install with the same libName isn’t accounted for in the “previous ownership” set: ReadManifestEntries only tracks prior ZIP installs. If {libName}.dll exists from a prior raw install, the ZIP path’s alias creation + CheckForConflicts will treat it as an external conflict (or, if alias is suppressed, DllUtils will keep discovering the stale raw DLL). Consider tracking raw installs in a manifest too, or explicitly treating an existing {libName}.dll as owned-by-previous for the same libName and deleting/replacing it atomically once staging validates.

[Copilot]

Alias suppression treats a nested match (e.g., lib/net8.0/{libName}.dll) as sufficient to skip creating a root {libName}.dll alias. But DllUtils.CreateDllList searches only the top-level of the library path (non-recursive), so a nested {libName}.dll won’t be discoverable and execution will fail. Consider only counting root-level {libName}.* as “already present” (and still generate an alias when the match is nested), or update DllUtils to search recursively and add a regression test.

Suggested change

	// 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))
	// DllUtils.CreateDllList can discover the library. Since DLL
	// discovery only checks the top level of the extracted library
	// path, only a root-level "{libName}.dll" (or "{libName}.*")
	// should suppress alias creation. 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)
	{
	bool isRootLevelPath =
	relPath.IndexOf('/') < 0 &&
	relPath.IndexOf('\\') < 0;
	string name = Path.GetFileName(relPath);
	if (isRootLevelPath &&
	(name.StartsWith(libName + ".", s_pathComparison) ||
	name.Equals(aliasFileName, s_pathComparison)))

[monamaki]

Add a null-pointer guard consistent with the managed version:

if (!errorString.empty() && libraryError != nullptr && libraryErrorLength != nullptr)

The managed SetLibraryError (CSharpExtension.cs) guards with if (libraryError != null && libraryErrorLength != null), but the native helper does not. If a caller ever passes null libraryError or libraryErrorLength, the native path dereferences null pointers (*libraryErrorLength = ...), causing an access violation / crash.

[monamaki]

does it mean that it will recursively unzip everything if a zip has a zip in it?

[monamaki]

It seems that we are bypassing conflict detection here. A different library's file could be silently overwritten.

This is what was used for the CSharp extension before this PR (since CSharp didn't implement InstallExternalLibrary): [please see engine code for this]

// ExthostLibraryUtils::InstallLibrary in exthostlibraryutils_win.cpp
if (!CopyFileW(lib->pathToFile.m_string, libraryPath, true /* bFailIfExists */))
{
    hr = HRESULT_FROM_WIN32(GetLastError());
}

CopyFileW(..., true) means bFailIfExists = TRUE — the copy fails if the destination already exists. This is the opposite of overwrite.

Even in th exthost test extension (CFunctionTestExtension::InstallExternalLibrary) — **fail if exists.

In ALTER, however, it seems we call Deletes ALL files in install dir.

ExtHost's ALTER path nukes the entire install directory and re-installs all libraries from scratch. Individual install calls then use bFailIfExists = TRUE, which succeeds because the directory was just emptied.

Before this PR, when a user ran CREATE EXTERNAL LIBRARY [shared] with a raw DLL, if shared already existed in the install directory, ExtHost's default CopyFileW(..., true) would fail the operation. The PR changes this to silently overwrite.

The overwrite-vs-fail difference only matters when:

• Two different libraries share a filename in the same install directory, OR
• Something goes wrong and a stale file is left behind

I think it would better to keep the same behavior for non-Zip files.

Please double check this with UC and also with a manual test.

[monamaki]

Consider AlterFromNonZipToZipTest: v1 is raw-DLL myLib, writing myLib.dll. v2 is ZIP testpackageB-DLL.zip (containing testpackageB.dll, testpackageB.deps.json) with the same libName = "myLib".

• v1 raw-DLL install writes myLib.dll, no manifest
• v2 ZIP install needs alias myLib.dll → adds to extractedFiles
• CheckForConflicts finds myLib.dll on disk, it's NOT in ownedByPrevious (empty set) → throws
• Test expects SQL_SUCCESS but gets SQL_ERROR

Can you please check if AlterFromNonZipToZipTest is really passing?

If it's failing: the fix is to detect the "manifest-less raw-DLL upgrade" case: if no manifest exists but DllFileNameFor(libName) exists on disk, add it to ownedByPrevious so the conflict check allows the overwrite.

[monamaki]

Problem:

using (var fs = new FileStream(path, FileMode.Open, FileAccess.Read))

The default FileShare is FileShare.None, acquiring an exclusive lock. If any other process has the file open (anti-virus scanner, another SQL session, backup agent), this throws IOException.

Recommendation:

using (var fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read))

[monamaki]

nit: while you're here, please remove the extra ;

[monamaki]

this is just more of a fyi since this is test.
CallInstall and CallUninstall helpers never free libError

[monamaki]

Not a blocker; but worth noting.

Problem: Both tests assert DirectoryHasFiles(installDir) — "at least one file exists." This is weak:

• If the install succeeds but extracts wrong files, the test passes.
• If a stale file from a previous test remains, the test passes even if install failed.

Later tests (e.g., ManifestWrittenTest, InstallZipWithNestedDirectoriesTest) use strong assertions checking specific file names. These early tests should do the same.

Recommendation: Replace DirectoryHasFiles with assertions on specific expected output files, consistent with the more thorough tests later in the file.

[monamaki]

Problem: The test installs package A as "testpackage", then package B as "testpackage" (ALTER). It only checks that package B's DLL exists:

EXPECT_TRUE(hasDll) << "DLL from second package not found after reinstall";

But does not verify that package A's unique files (testpackageA.dll, testpackageA.txt) were removed. The test could pass even if cleanup is broken.

The later AlterExternalLibraryTest does verify cleanup thoroughly, but ReinstallLibraryTest should also be self-contained.

Recommendation: Add:

EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.dll"));
EXPECT_FALSE(fs::exists(fs::path(installDir) / "testpackageA.txt"));

[monamaki]

Problem: If errorString is empty, the function does nothing — *libraryError and *libraryErrorLength remain at whatever the caller initialized them to. ExtHost initializes them to nullptr / 0, so this is fine in the current call pattern. But a future caller could pass uninitialized values.

if (!errorString.empty())
{
    // Only set on non-empty — otherwise output params are untouched
}

Recommendation: Consider explicitly setting *libraryErrorLength = 0 in the else branch for robustness. This is a nit — not a bug in current usage.