CSharp extension: ZIP archive support for InstallExternalLibrary/UninstallExternalLibrary

[Stuart Padley (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 (or, for raw-DLL installs, the single <libName>.dll entry). 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; 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: ZipFile.ExtractToDirectory rejects path-traversal entries at extraction time. The single-path collapse in review pass 3 (see Update below) means the install code never sees an entry that escaped the staged tree, so there is no second-level vector to defend.
• Symlink defense: CopyDirectory skips entries with the ReparsePoint attribute at every recursion level ΓÇö both file and directory symlinks. Today's .NET writes symlink-mode ZIP entries as regular files on every platform; this guard is future-proofing for a runtime that materializes them as real symlinks. Pinned by InnerZipFutureSymlinkRejectedTest.
• Library-name validation: ValidateLibraryName rejects null/empty/whitespace-only names, names containing .. / path separators / null characters, absolute paths, extension-only names like .dll, and Windows reserved DOS device names (CON, NUL, AUX, PRN, COM1ΓÇôCOM9, LPT1ΓÇôLPT9 ΓÇö bare or suffixed). Reserved-name rejection is enforced on every OS so libraries moved between hosts behave consistently.
• Native error-buffer ownership: SetLibraryError in the native code allocates the error buffer with malloc (matching the managed Marshal.AllocHGlobal contract that ExtHost expects on the other side), so ownership transfers cleanly to the host. Pre-fix returned a c_str() pointer into a freed std::string ΓÇö undefined behavior on the host side.
• 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, file overlap is rejected)
Non-ZIP file (raw DLL)	Copied directly as <libName>.dll; manifest written so uninstall and ALTER work uniformly
Raw DLL would overwrite a foreign <libName>.dll planted by another library or external tooling	Install fails with explicit error (preserves the pre-PR bFailIfExists=TRUE contract)
File with .zip extension whose bytes are not a valid ZIP	Install fails with explicit error; user's file is not silently renamed to <libName>.dll
Drop library with no manifest	<libName>.dll is deleted directly (legacy-compat path for libraries installed by pre-PR builds)
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 is empty, whitespace-only, contains ../path separator/null character, is an absolute path, is extension-only (.dll), or is a Windows reserved DOS device name (CON/NUL/AUX/PRN/COMn/LPTn ΓÇö bare or suffixed)	Install fails with explicit error
Inner ZIP entry has a Unix symlink mode (theoretical, future runtime)	IsReparsePoint guard skips the entry; legitimate files still install

Tests

28 new TEST_F cases in CSharpLibraryTests.cpp covering:

• Manifest creation and content (flat + nested paths)
• <libName>.dll alias naming and removal on uninstall
• Flat-file coexistence allowed; file conflict rejected
• Inner-ZIP file conflict rejected
• Uninstall preserves unrelated libraries and shared nested directories
• Empty nested directory cleanup
• ALTER-style re-install (ZIPΓåöZIP, ZIPΓåönon-ZIP, non-ZIPΓåönon-ZIP, ALTER to empty-dirs ZIP preserving v1)
• Error message surfacing through libraryError
• Conflict in inner-ZIP code path; temp-folder cleanup on failure
• Raw-DLL install fails when a foreign <libName>.dll exists
• Alias-conflict detected before extraction
• Library-name validation rejects empty / whitespace-only / path-traversal / null character / absolute path / extension-only / reserved DOS device name
• Uninstall rejects path-traversal library names
• ZIP install with .dll-suffixed library name
• Install of a .zip-extension file with non-ZIP bytes fails loudly (does not silently rewrite the user's file)
• Inner-ZIP future-symlink rejection (regression test for the single-path collapse ΓÇö see Update below)
• Uninstall handles missing install directory gracefully

Update (review pass 2)

Atomicity contract clarified: Install is NOT atomic at the per-file level. A crash between CleanupManifest and the CopyDirectory loop can leave the install directory inconsistent. End-to-end recovery is provided by SQL Server's library management architecture: the catalog is the source of truth, and the next session re-installs from the catalog. The in-extension code is staging-validated (corrupt ZIPs cannot start a destructive cleanup) but is not crash-safe.

Raw-DLL installs now write a manifest. A one-entry {libName}.manifest listing {libName}.dll is written for raw-DLL installs as well as ZIPs. This:

• Restores the pre-PR CopyFileW(..., bFailIfExists=TRUE) contract: a foreign {libName}.dll planted by another library or external tooling is no longer silently overwritten ΓÇö install fails.
• Makes ALTER from rawΓåÆZIP and ZIPΓåÆraw work via the existing manifest cleanup path ΓÇö no special-case logic needed.

Other v3.8.0 hardening: native SetLibraryError null-check, ZIP file opens with FileShare.Read, CopyDirectory skips ReparsePoint entries (Linux symlink defense), alias suppression now correctly counts only root-level matches (DllUtils.CreateDllList is non-recursive), various comments / doc improvements per inline review.

Update (review pass 3)

Addresses 4 inline comments from JustinMDotNet and 13 from yaelh. Tip: 8ef1573.

Native / managed correctness fixes:

• nativecsharpextension.cpp SetLibraryError: replaced new std::string(errorString) + c_str() with malloc(len + 1) + memcpy. Buffer ownership transfers cleanly to ExtHost; OOM path returns the no-error state instead of crashing.
• CSharpExtension.cs AcquireInstallLock: narrowed the catch via when (IsSharingViolation(ex)) filter (HResult ERROR_SHARING_VIOLATION (32) / ERROR_LOCK_VIOLATION (33)). DirectoryNotFoundException, PathTooLongException, UnauthorizedAccessException, etc. now propagate fast instead of being swallowed.
• DllUtils.cs: replaced Directory.GetFiles(searchPath, "{name}.*") + .Where(...EndsWith(".dll")) with EnumerateFiles + explicit Equals(".dll", OrdinalIgnoreCase) on extension and Equals(userLibName, OrdinalIgnoreCase) on stem. Eliminates the *.dll matching foo.dllx and Foo.* matching short-name 8.3 alias quirks.
• CSharpExtension.cs install path collapsed to a single code path: both inner-zip and outer-zip cases now extract into tempFolder/inner-content/ first and walk the result on disk via IsReparsePoint-guarded CopyDirectory. Removed the direct-extract-to-installDir shortcut so a future runtime that materializes Unix symlink-mode entries can't bypass the reparse-point guard. ValidateRelativePath removed (zero callers after the collapse ΓÇö zip-slip defense is now provided by ZipFile.ExtractToDirectory plus the on-disk walk being unable to see entries that escaped the staged tree).
• New regression test InnerZipFutureSymlinkRejectedTest + fixture testpackageK-SYMLINK.zip (262 bytes, generated by build-symlink-fixture.ps1). Inner zip contains legitfile.dll + evil-symlink.dll (Unix mode 0o120755, content /etc/passwd). Asserts: install succeeds, legitfile.dll lands in installDir, and installDir contains zero reparse points.
• UninstallExternalLibrary: wrapped File.Delete(libraryFile) in an else branch so the manifest path exclusively owns cleanup for current-version installs. The direct delete only runs as a legacy-compat path for libraries installed by pre-PR builds with no manifest.
• DetermineAliasSource: lexicographically-first .dll candidate via string.CompareOrdinal (stable across NTFS / ext4 / XFS and re-installs).

Validation / behavior tightening:

• ValidateLibraryName: now rejects whitespace-only names (IsNullOrWhiteSpace) and Windows reserved DOS device names (full set: CON, NUL, AUX, PRN, COM0ΓÇôCOM9, LPT0ΓÇôLPT9). Stem is checked via Path.GetFileNameWithoutExtension, so CON, CON.dll, and nul.manifest are all rejected. Enforced on every OS for consistency. New rows in InstallRejectsInvalidLibNameTest.
• IsZipFile content-sniff replaced by extension-based HasZipExtension. A .zip file whose bytes are not a valid archive now returns SQL_ERROR instead of being silently rewritten as <libName>.dll ΓÇö the user's registered filename is opaque to the install path. The corresponding test was renamed to InstallZipExtensionWithBadContentFailsLoudlyTest and inverted to assert the loud-failure behavior.

Test hardenings:

• InstallZipWithManyFilesTest: per-module existence loop (Module1.dll ΓǪ Module50.dll) plus a comment block with the full historical context (the original EXPECT_EQ(dllCount, 50) agreed with the buggy install code that created the alias as {libName} with no extension ΓÇö test asserted what the code did, not what it should do; fixed in commit 38c553d).
• DirectoryOverlapAllowedTest renamed to NonConflictingFlatFilesCoexistTest with a tighter comment block clarifying that nested-directory overlap is covered separately.

Docs / style:

• IsReparsePoint <remarks> now includes the concrete sneaky.dll → /etc/shadow worked example covering both file and directory reparse-point cases.
• Trimmed the CleanupManifest catch comment to stop at the diagnostic-trail justification.
• Project-wide blank-line-after-} sweep applied where the pattern was genuinely two adjacent independent constructs (file/dir loops, consecutive flag-setter ifs); guard-then-action } followed by continue;/return X; left alone.
• One-line rationale comments at the explicit .close() sites in test code so the pattern is self-documenting.

Test results: 112/112 unit tests pass on Windows release config.

Update (review pass 4)

Addresses 4 inline comments from JustinMDotNet (CMake std flag, header doc contract, alias suppression for sidecar-only roots, native error-buffer ownership in tests) plus 4 production-found bugs from end-to-end testing against SQL Server 2025 RTM-GDR.

Production fixes (end-to-end testing):

• CSharpExtension.cs AcquireInstallLock: lock file is now created at Path.Combine(installDir, "install.lock") instead of one level up. The previous path put the lock outside the per-<dbid>/<langid> install directory, so concurrent installs into different DB/language slots serialized against each other unnecessarily and (worse) a single install could race against itself when the parent directory was on a separately-permissioned mount.
• CSharpExtension.cs new DispatchAsZip(libName, libFilePath) helper: install dispatch is now driven by the registered library name's extension, not the staged temp file's extension. SQL Server's ExtHost passes a generated temp file with no semantic suffix, so the previous content-/extension-sniff on libFilePath was meaningless in production. Order of resolution is libName ends in .zip → ZIP, libName ends in .dll → raw DLL, otherwise fall back to libFilePath's extension (preserves the legacy contract for test fixtures that register libraries by bare name and point libraryFile at a *.zip / *.dll fixture).
• CSharpOutputDataSet.cs + Sql.cs DotNetNVarChar plumbing: the SqlDataType.DotNetNVarChar enum value (the string row in Sql.DataTypeMap) had no entry in Sql.DataTypeSize and no case in CSharpOutputDataSet.ExtractColumn / GetStrLenNullMap. Calls fell through to default and threw KeyNotFoundException in DataTypeSize before the column ever reached the dispatch switch. Fixed by adding the MinUtf16CharSize row to DataTypeSize and a DotNetNVarChar case alongside DotNetWChar in both switches (they're SQL_C_WCHAR-shaped at the ODBC layer and share an implementation).
• CSharpOutputDataSet.cs DotNetWChar / DotNetNVarChar Size unit: Size is now reported in bytes, matching the unit emitted by GetStrLenNullMap (Encoding.Unicode.GetByteCount). The previous code divided by a UTF-16 code-unit width and reported a character count, so SPEES logged "Reading one row failed for column N row M. The length information is incorrect." and rejected the rowset whenever a string column contained non-ASCII data.

Reviewer fixes:

• test/src/native/CMakeLists.txt: non-MSVC target_compile_options flag changed from --std=c++17 to -std=c++17 (one dash). GCC and Clang accept both spellings, but the single-dash form is the documented one and matches the rest of the build tree.
• include/nativecsharpextension.h: InstallExternalLibrary doc comment rewritten to spell out the new dispatch contract (libName-based, not libFile-content-based) and to document that a {libName}.manifest is written for every install (ZIP or raw DLL). The previous comment described only the legacy "raw DLL or ZIP detected from the file" behavior.
• CSharpExtension.cs DetermineAliasSource: alias-suppression now requires an exact match against aliasFileName ("{libName}.dll") at the install root. The previous check accepted any root-level entry whose name started with "{libName}.", so a ZIP that planted only sidecars at the root (e.g. foo.deps.json, foo.runtimeconfig.json) and kept the real binary nested under lib/net8.0/foo.dll was treated as if foo.dll were already loadable — alias creation was suppressed and the install was un-loadable. The unused libName parameter was dropped from the signature in the same change. New regression test AliasCreatedWhenOnlySidecarsAtRootTest + fixture testpackageL-SIDECAR.zip (491 bytes, generated by build-sidecar-fixture.ps1) pins the new behavior.
• test/src/native/CSharpLibraryTests.cpp new FreeLibError(SQLCHAR *) helper using LocalFree (matches the production Marshal.AllocHGlobal / LocalAlloc allocator that ExtHost expects 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. Pre-fix the harness allocated, ignored, and never freed — a 113-test run accumulated kilobytes of orphan libError strings on the heap.

Test results: 113/113 unit tests pass on Windows release config (one new TEST_F: AliasCreatedWhenOnlySidecarsAtRootTest).

[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]

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.

[yaelh]

Edge case: ZIP with only empty directories passes the archive-not-empty check but produces no files

The empty-archive guard checks Directory.GetFiles and Directory.GetDirectories on the tempFolder:

if (Directory.GetFiles(tempFolder).Length == 0 &&
    Directory.GetDirectories(tempFolder).Length == 0)
{
    throw new InvalidOperationException(
        "The library archive contains no entries.");
}

A ZIP containing only empty directories (no files) passes this check because GetDirectories is non-zero. But CollectRelativeFiles only collects files, so extractedFiles ends up empty. The code then proceeds to:

1. Run CheckForConflicts with an empty list (no-op)
2. Call CleanupManifest for any prior version (destructive — deletes old files)
3. Copy nothing into installDir
4. Skip writing the manifest (if (extractedFiles.Count > 0) is false)

The result: the old library content is deleted, nothing new is installed, and no manifest is written — a silent data-loss scenario during ALTER.

Consider adding a check after CollectRelativeFiles / inner-zip entry collection:

if (extractedFiles.Count == 0)
{
    throw new InvalidOperationException(
        "The library archive contains no files.");
}

[yaelh]

Inner-zip path writes directly to installDir, bypassing CopyDirectory's reparse-point defense

The non-inner-zip path extracts to tempFolder and then copies through CopyDirectory, which skips symlinks/junctions. The inner-zip path extracts directly into the live installDir:

ZipFile.ExtractToDirectory(innerZipPath, installDir, false);

This is currently safe on Windows because .NET's ZipFile.ExtractToDirectory does not restore symlink entries as actual symlinks — it writes them as regular files containing the link target text.

However, with Linux support planned (mentioned in the PR description and in the CopyDirectory symlink discussion), this assumption may not hold across all future .NET versions or if the extraction library changes. The two extraction paths would then have different security guarantees: non-inner-zip is symlink-safe, inner-zip is not.

Since this PR is already adding cross-platform defense (the CopyDirectory reparse-point checks, OS-aware path comparisons, etc.), it would be better to make this path correct now rather than leaving a known gap for the next person to discover. Consider routing the inner-zip extraction through the staged temp folder + CopyDirectory as well, or at minimum adding a post-extraction reparse-point scan of installDir.

If that's too much churn for this PR, at least add a comment documenting why this is safe today and flagging it for re-evaluation:

// SAFETY: ZipFile.ExtractToDirectory does not restore symlink entries as
// actual symlinks on any platform — they are written as regular files.
// If this changes in a future .NET version or if we switch to a different
// extraction library, this path must be routed through CopyDirectory
// (or an equivalent reparse-point filter) to match the non-inner-zip path.
ZipFile.ExtractToDirectory(innerZipPath, installDir, false);

[yaelh]

Root-level files skip the reparse-point check that CopyDirectory enforces

CopyDirectory correctly skips reparse points (symlinks/junctions) before calling File.Copy:

// Inside CopyDirectory — protected ✓
FileAttributes fileAttrs = File.GetAttributes(file);
if ((fileAttrs & FileAttributes.ReparsePoint) != 0)
    continue;

But the root-level copy loop here does not have that check:

// Non-inner-zip path in InstallExternalLibrary — NOT protected ✗
foreach (string file in Directory.GetFiles(tempFolder))
{
    File.Copy(file, Path.Combine(installDir, Path.GetFileName(file)), false);
}

Example attack scenario on Linux (upcoming per PR description): a ZIP extracts to:

tempFolder/
  legit.dll          ← normal file, copied fine
  sneaky.dll         ← symlink pointing to /etc/shadow or another sensitive file
  subdir/
    nested.dll       ← symlink here → correctly SKIPPED by CopyDirectory

sneaky.dll at the root level would be followed by File.Copy, copying the target of the symlink (e.g. /etc/shadow) into installDir as sneaky.dll — an information-leak vector. Meanwhile nested.dll inside subdir/ is correctly skipped by CopyDirectory's reparse-point guard. The defense has a gap at exactly one level.

Same gap exists for the root-level directory loop: a reparse-point directory would be passed to CopyDirectory, which checks its children but not the directory itself on entry (the check is on entries discovered during iteration, not the top-level sourceDir argument).

Consider adding the same guard before both root-level copies:

foreach (string file in Directory.GetFiles(tempFolder))
{
    if ((File.GetAttributes(file) & FileAttributes.ReparsePoint) != 0)
        continue;
    File.Copy(file, Path.Combine(installDir, Path.GetFileName(file)), false);
}
foreach (string dir in Directory.GetDirectories(tempFolder))
{
    if ((File.GetAttributes(dir) & FileAttributes.ReparsePoint) != 0)
        continue;
    CopyDirectory(dir, Path.Combine(installDir, Path.GetFileName(dir)));
}

[yaelh]

Test coverage gaps

The test suite is thorough — nice work covering both extraction paths, conflict detection, ALTER directions, alias lifecycle, zip-slip, and error propagation. A few gaps remain:

1. Raw DLL → Raw DLL ALTER
Every ALTER direction is tested (ZIP→ZIP, raw→ZIP, ZIP→raw) except raw→raw. v1 installs raw DLL as "myLib", v2 installs a different raw DLL as "myLib". The code should clean up v1 via manifest, then copy v2. Simple path, but it's the only ALTER direction without coverage.

2. ValidateLibraryName edge cases (install + uninstall)
Path traversal is only tested on the uninstall side (UninstallRejectsPathTraversalLibNameTest). Missing:

• Install with "../escape" libName (should fail)
• Empty libName "" (should fail)
• Null character in libName e.g. "foo\0bar" (should fail)
• Absolute path libName e.g. "C:\\Windows\\foo" (should fail)

These are all rejected by ValidateLibraryName, but a regression in that function would go undetected on the install side.

3. DllUtils.AddMatches not directly tested
The new exact-name-first / wildcard-fallback / .dll-only-filtering logic has no direct test. ExecuteInvalidLibraryNameScriptTest only tests the "no match" path. Consider adding tests for:

• Bare name "regex" correctly discovers "regex.dll" (wildcard fallback)
• Explicit name "Foo.dll" resolves via exact match (not wildcard)
• Wildcard match doesn't return .runtimeconfig.json / .deps.json files

4. .dll-suffixed libName in ZIP install path
InstallRawDllWithDllSuffixedLibNameTest covers DllFileNameFor for raw DLLs. But no test installs a ZIP with libName = "foo.dll" to verify alias behavior doesn't produce a double-extension alias or suppress alias creation incorrectly.

5. Uninstall when installDir doesn't exist
UninstallNonExistentLibraryTest creates the installDir first. The Directory.Exists(installDir) guard should handle a missing directory gracefully, but there's no test for it.

6. Nested-dir uninstall with shared parent directory
UninstallRemovesEmptyNestedDirsTest tests single-library cleanup. Missing: two libraries sharing a parent folder (e.g. both under lib/), uninstalling one should leave lib/ intact because the other library still has content there.

[yaelh]

InstallZipContainingDllsTest — weak assertion

This test only asserts that some .dll exists in installDir — it doesn't check for a specific file. If the install extracts the wrong content, or a stale file from a prior test survives CreateInstallDir, the test still passes. Consider asserting fs::exists(... / "testpackageB.dll") like the other tests do.

[yaelh]

ReinstallLibraryTest — redundant hasDll loop

The hasDll loop at the top of the post-reinstall assertions is now redundant. The stronger assertions added below (EXPECT_TRUE(... / "testpackageB.dll"), EXPECT_FALSE(... / "testpackageA.dll")) already verify the correct files are present. The generic "any .dll exists" loop can be removed to reduce noise.

[yaelh]

ManifestWrittenTest — substring match instead of exact equality

Manifest entry checks use string::find (substring match):

if (e.find("testpackageB.dll") != string::npos) hasDll = true;

This would match entries like "xtestpackageB.dll", "testpackageB.dll.bak", or "subdir/testpackageB.dll" (the last one is arguably OK for nested layouts, but not for this flat package). Consider using exact equality for flat-package tests:

if (e == "testpackageB.dll") hasDll = true;

[yaelh]

InstallLibNameAliasTest — doesn't verify alias file content

The test verifies the alias file myAlias.dll exists and appears in the manifest, but doesn't verify its content. If the alias were zero-length, a copy of the wrong file, or corrupted, the test still passes. Consider comparing file size or a few bytes against the expected source DLL (testpackageB.dll) to confirm the alias is a valid copy.

[yaelh]

DirectoryOverlapAllowedTest — doesn't actually test directory overlap

Despite the name, this test doesn't exercise directory overlap. Packages A and B both have flat root-level files (testpackageA.dll, testpackageB.dll, etc.) with no shared subdirectories. To actually test that two libraries can share a parent folder (e.g. both extracting files under lib/), you'd need test packages whose directory trees overlap but whose filenames differ. As written, this is really a "non-conflicting flat files coexist" test.

[yaelh]

ErrorMessagePopulatedOnFailureTest — modes 1+2 don't validate message content

Failure modes 1 (missing file) and 2 (zip-slip) only assert !msg.empty(). A message like " " or a single garbage byte would pass. Mode 3 already does content validation (msg.find("libB")). Consider adding similar content checks to modes 1 and 2 — e.g. verify the missing-file message contains the path, and the zip-slip message contains "invalid path" or similar.

[Stuart Padley (stuartpa)]

Pushed two new commits addressing the third review pass:

• 55d15e3 — bug-fix omnibus: A1 native buffer ownership (malloc+memcpy, fixes c_str() UB), A2 IOException filter via IsSharingViolation, A3 DllUtils explicit enumeration (no more Foo.* 8.3 quirks), A4+B1 inner-zip routed through tempFolder/inner-content/ + IsReparsePoint-guarded copy + new symlink regression fixture (testpackageK-SYMLINK.zip, generated by build-symlink-fixture.ps1), A5 File.Delete gated to legacy path only, A6 s_reservedDeviceNames HashSet (CON/PRN/NUL/AUX/COM1-9/LPT1-9 + 8 new test rows), A7 lexicographic alias selection, B6 extension-based HasZipExtension (no more silent .zip→.dll rewrite), B12 IsNullOrWhiteSpace, B13 ValidateRelativePath removed (zero callers after A4), plus B2/B3/B4/B5/B7/B9/B10/B11 docs/tests/style.
• 8ef1573 — B8: kept "NonExistentLibrary" + added a 4-line code comment explaining why the pre-PR literal can't be restored (the loader resolves it to the real test DLL and the test would fail to observe the expected error). Test-failure transcript posted in the B8 thread.

Test results: 112/112 unit tests pass on Windows release config.

Replies posted on every thread above. yaelh Justin M (@JustinMDotNet) — ready for another look.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 22 changed files in this pull request and generated 4 comments.

[yaelh]

Signing off since the remaining comments are small and trivial to implement. also since I'll be OOF I don't want to block the PR

[MHN (monamaki)]

Please run PVS once before merging this PR.

I have shared the instructions with Justin.

[Stuart Padley (stuartpa)]

Acknowledged MHN (@monamaki) — coordinating with Justin M (@JustinMDotNet) to run PVS against the dotnet-core-CSharp extension. We'll post the results (and address any findings) before requesting re-review. Thanks for getting the instructions over to him.

[SicongLiu2000]

Review Summary — ZIP Archive Install/Uninstall Support

Excellent test coverage (2103 lines) and solid defense-in-depth (reparse point checks, manifest-based cleanup, DOS device name rejection, zip-slip prevention). One security finding that should be addressed before merge:

Must-fix (High)

1. Path traversal bypass — ValidateLibraryName rejects literal .. but not URL-encoded %2e%2e or Unicode homoglyphs. Switch to whitelist: [a-zA-Z0-9_.-]+

Should-fix (Medium)

2. Infinite lock — AcquireInstallLock blocks forever with no timeout or cancellation
3. TOCTOU race — gap between CheckForConflicts and ExtractContentToInstallDir (mitigated by ACLs, but undocumented)
4. Incomplete reparse detection — IsReparsePoint doesn't catch hard links
5. Path separator mismatch — manifest entries use platform-native separators, breaking cross-platform portability

See inline comments for details and suggested fixes.

[SicongLiu2000]

Changing to Approve — the path traversal concern (Finding #1) depends on whether SQL Server normalizes the library name before passing it to the extension. Given that CREATE EXTERNAL LIBRARY likely passes the name as-is (T-SQL identifiers don't undergo URL decoding), the existing validation is sufficient in practice. The other findings are hardening suggestions, not blockers. LGTM with the inline suggestions noted.

[Stuart Padley (stuartpa)]

MHN (@monamaki) — Understood on the PVS-Studio run. Will run it before merging and address any findings.