dotnet / dotnet/msbuild

Copy task still corrupts hard-linked files (e.g. the NuGet cache) when the destination cannot be deleted

Open
#14,956 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
5.5k
Forks
1.5k
Avg merge
1d 8h
Merged PRs (30d)
141

Description

### Issue Description

The fix for #8273 makes the `Copy` task delete an existing destination before copying, so the copy
replaces the directory entry instead of writing *through* a hard/symbolic link. That delete goes
through `FileUtilities.DeleteNoThrow`, **whose failure is silently swallowed**, and the copy then
runs regardless:

https://github.com/dotnet/msbuild/blob/main/src/Tasks/Copy.cs#L322-L328
https://github.com/dotnet/msbuild/blob/main/src/Tasks/Copy.cs#L377-L383

In this Windows repro, the blocking handle is opened directly through the destination path with
`FileAccess.ReadWrite` and `FileShare.ReadWrite`, without `FileShare.Delete`. This prevents
`DeleteFileW` from deleting that destination while still allowing the subsequent
`File.Copy(..., overwrite: true)` to overwrite the existing file record. Every hard link to that
record, including the one in the NuGet global-packages folder, sees the new bytes.

This distinction matters: git-lfs/git-lfs#6162 reports that a handle opened through a different
hard-link name also prevented deletion on Windows Server 2016, whereas the same test succeeded
after upgrading to Server 2022. The MSBuild repro uses the destination path itself; it does not
depend on that older sibling-hard-link behavior.

The build reports **success**, with **no warning and no error**.

The repro explicitly requests these sharing flags with
`File.Open(path, FileMode.Open, FileAccess.ReadWrite, FileShare.ReadWrite)`.
The version matrix below establishes this trigger; it does not establish which background tools
use those flags. Related Roslyn discussions: dotnet/roslyn#8868 / dotnet/roslyn#8875.

### Relation to #8273 / #8685

This is **not** a duplicate of #8273 and not a regression of its fix — it is a gap the fix does not
cover.

- **#8273** (fixed by #8275, reverted by #8686, re-landed via #8685): `Copy` did not delete the
destination at all, so *every* copy over a hard link wrote through to the NuGet cache.
- **This issue:** `Copy` does attempt the delete, but when the delete *cannot* succeed the failure
is ignored and the old unsafe overwrite happens anyway.

In the tested matrix, the 2023 fix repaired the unlocked case but left the case where a handle
opened through the destination path omits `FILE_SHARE_DELETE`. Measured across versions, the fix repairs the unlocked and
`FileShare.Delete` cases but leaves the locked case corrupting silently, from 17.7 through current
`main`.

Also related but distinct: #14134 (make the overwrite atomic via temp + rename) would turn this
case into a loud failure as a side effect, since `MoveFileEx(MOVEFILE_REPLACE_EXISTING)` also
requires `FILE_SHARE_DELETE`. #9250 and #8684 concern other aspects of the same delete-then-copy
sequence.

### Steps to Reproduce

Standalone repro (no SDK, no restore, just the `Copy` task) — full sources in the
**Reproducer** section at the bottom.

```
cache/A.dll <- stands in for the NuGet global-packages file
└── hard link ──> bin/A.dll <- what CopyLocal + hardlinks produces

helper process:
File.Open("bin/A.dll", FileMode.Open, FileAccess.ReadWrite, FileShare.ReadWrite)
# note: no FileShare.Delete

MSBuild:

```

The end-to-end version is exactly the #8273 repro plus a held handle:

```powershell
$env:NUGET_PACKAGES = "$scratch\packages" # never touch the real one
dotnet new console
dotnet add package Newtonsoft.Json -v 13.0.1
dotnet build -p:CreateHardLinksForCopyLocalIfPossible=true
# a helper process now opens bin/Debug//Newtonsoft.Json.dll
# with FileAccess.ReadWrite / FileShare.ReadWrite
dotnet add package Newtonsoft.Json -v 13.0.3
dotnet build
```

### Expected Behavior

`packages/newtonsoft.json/13.0.1/lib/netstandard2.0/Newtonsoft.Json.dll` is byte-identical to what
NuGet extracted. If the destination cannot be replaced safely, the build fails with a diagnostic —
as it already does when the *copy* itself is blocked.

### Actual Behavior

```
packages copy : packages\newtonsoft.json\13.0.1\lib\netstandard2.0\Newtonsoft.Json.dll
hard links : 2
packages SHA before : 72CF291D4BAB0EDD...
build exit code : 0
MSB warnings : 0
MSB errors : 0
packages SHA after : 22C649F75FCE5BE7...
matches : 13.0.3\lib\net6.0\Newtonsoft.Json.dll
=> CORRUPTED - the 13.0.1 asset now contains 13.0.3\lib\net6.0\Newtonsoft.Json.dll
```

The 13.0.1 package now ships 13.0.3 bytes. The build succeeded and said nothing.

### Variant matrix

| | Destination handle | Delete | Copy | Cache | Build |
| --- | --- | --- | --- | --- | --- |
| A | none | succeeds | new file | intact | exit 0 |
| B | `Read` / share `Read` | fails | fails | intact | **exit 1**, MSB3026 ×2 + MSB3027 |
| **C** | `ReadWrite` / share `ReadWrite` | **fails, swallowed** | **succeeds, through the link** | **CORRUPTED** | **exit 0, 0 warnings, 0 errors** |
| D | `ReadWrite` / share `ReadWrite,Delete` | succeeds | new file | intact | exit 0 |

The contrast between B and C is the heart of the report: when the copy fails the user gets a loud
error; when only the delete fails, the user gets silence and corrupted shared state.

`fsutil hardlink list` confirms the mechanism — in A and D the link count drops 2 → 1 (link broken,
as intended); in C it stays 2 → 2 and `fsutil file queryfileid` is unchanged, i.e. the record was
overwritten in place.

### Why it happens

1. Destination exists and is a hard link into the packages folder. ✔
2. `Copy` calls `FileUtilities.DeleteNoThrow(destination)` — `Copy.cs:326`. ✔
3. `DeleteFileW` fails with `ERROR_SHARING_VIOLATION`, because the handle opened through the destination path lacks
`FILE_SHARE_DELETE`. ✔
4. `DeleteNoThrow` returns `void` and swallows the exception — `FileUtilities.cs:1060-1069`. ✔
5. `File.Copy(source, destination, true)` runs — `Copy.cs:380`. ✔
6. `CopyFileW` opens the surviving file and writes into the existing record. ✔
7. All hard links to that record observe the new content. ✔
8. `File.Copy` succeeded, so `DoCopyWithRetries` never sees an exception and the task returns
success. ✔

Every step verified individually; see the analysis document in the repro bundle.

### Relevant source code

- `src/Tasks/Copy.cs:322-328` — the delete-first guard added for #8273
- `src/Tasks/Copy.cs:377-383` — the unconditional `File.Copy(..., overwrite: true)`
- `src/Framework/FileUtilities.cs:1060-1069` — `DeleteNoThrow`, `void`, empty catch
- `src/Framework/Traits.cs:304` — `MSBUILDCOPYWITHOUTDELETE`; note this escape hatch only makes the
problem worse (it skips the delete entirely) and is not a mitigation
- `src/Tasks/Copy.cs:1003+` — `DoCopyWithRetries`; the MSB3026/MSB3027/MSB3021 path that is never
reached in this scenario

### Impact

- A file in the **global** NuGet packages folder can be permanently replaced with different content;
that folder is shared by every project and solution on the machine.
- The damage outlives the build that caused it — a later, unrelated build resolving that package
version silently links the wrong assembly.
- Package contents are not re-verified on use, so nothing detects it.
`dotnet nuget locals global-packages --clear` plus a re-restore is the practical repair.
- The causing build **succeeds** with no diagnostic, so there is no signal to trace back from. In
practice, investigation starts from a much later and seemingly impossible runtime failure.

Calling this a security vulnerability would be overstating it — there is no attacker and no
privilege boundary. It is a data-integrity bug whose severity comes from the silence plus the
shared, machine-wide blast radius.

### Reproducibility

100% deterministic in the harness, on every version tested. In a real build it depends on a handle
being held at the moment `Copy` runs, so it will present as intermittent.

### Versions & Configurations

Windows 11 Pro 10.0.26200 (build 26200.9168), NTFS, x64.

| SDK | MSBuild | A | B | C | D |
| --- | --- | --- | --- | --- | --- |
| 6.0.428 | 17.3.4 | **CORRUPT** | error | **CORRUPT** | **CORRUPT** |
| 7.0.410 | 17.7.6 | ok | error | **CORRUPT** | ok |
| 8.0.424 | 17.11.48 | ok | error | **CORRUPT** | ok |
| 9.0.317 | 17.14.51 | ok | error | **CORRUPT** | ok |
| 10.0.400 | 18.9.6 | ok | error | **CORRUPT** | ok |
| `main` @ `eeeacc82` (built from source) | 18.12.0 | ok | error | **CORRUPT** | ok |

17.3 is the pre-fix baseline and matches #8273 as reported (A, C and D all corrupt). 17.7 fixes A
and D. **C has never been fixed.**

### Proposed direction for fix

The invariant:

> If the destination may be a hard or symbolic link, `Copy` must not perform an in-place overwrite
> after a failed attempt to delete it.

Concretely, before the `File.Copy`: if a delete was attempted and the destination both survived
*and* is still a link, refuse.

```csharp
if (destinationDeleteAttempted && DestinationIsSurvivingLink(destinationFileState.Path))
{
destinationFileState.Reset();
throw new IOException(ResourceUtilities.FormatResourceStringStripCodeAndKeyword(
"Copy.LinkedDestinationNotDeleted",
destinationFileState.Path.OriginalValue));
}
```

`DestinationIsSurvivingLink` returns `false` immediately in the normal case where the delete worked
(a single `File.Exists`); only on the failure path does it check `FileAttributes.ReparsePoint` and
then, on Windows, `GetFileInformationByHandle().nNumberOfLinks > 1`. The guard adds one existence check even when deletion succeeds; the attribute and
hard-link checks are only reached when the destination still exists.

Throwing rather than warning reuses the existing `DoCopyWithRetries` machinery, so the user sees the
existing retry/failure diagnostics (MSB3026, MSB3027 and MSB3021, depending on the retry path),
with an explanation of why the copy was refused. The resource is labelled MSB3897, but the call to
`FormatResourceStringStripCodeAndKeyword` strips that code before creating the exception; this
implementation does not emit a separate MSB3897 diagnostic. A build that cannot safely replace a
hard-linked file should fail, not corrupt machine-wide shared state quietly.

PR #14957 implements the change behind a Change Wave, including the non-Windows behavior
noted below. The two options considered were:

**Option 1 — gated behind a Change Wave (`Wave18_12`).** New behavior on by default;
`MSBUILDDISABLEFEATURESFROMVERSION=18.12` restores today's behavior for anyone it disrupts. Given
that #8275 had to be reverted once already (#8686), this seems like the safer way to land a change
in this method.

**Option 2 — unconditional.** The same guard without the wave check.

I'd lean toward Option 1 for reviewability, with one caveat: the escape hatch here re-enables silent
cache corruption, so it should probably be short-lived.

Alternatives considered and why they were not chosen:

| Option | Assessment |
| --- | --- |
| Make `DeleteNoThrow` return `bool` | Useful plumbing, but insufficient alone — a failed delete only matters when the destination is actually a link. Checking link status keeps the behavior change narrow. |
| Temp file + atomic replace (#14134) | Good for independent reasons. On Windows it also fixes this case, since `MoveFileEx(MOVEFILE_REPLACE_EXISTING)` likewise requires `FILE_SHARE_DELETE` — so it reaches the same loud failure by a longer route. If #14134 is landing soon, this issue may be best folded into it. |
| Retry the delete | The retry loop exists but is only entered on an exception. A long-lived handle (test host, scanner) just burns the retry budget and errors anyway — same end state, more wall clock. |
| Warn and continue | Rejected: corrupting the shared cache with a warning is still corrupting the shared cache. |

Backward-compatibility notes:

- On Windows, a surviving destination confirmed to be a non-reparse file with a single hard link
can still be overwritten. If its attributes or link count cannot establish that it is safe, the
guard can conservatively refuse the copy.
- `MSBUILDCOPYWITHOUTDELETE=1` keeps its documented semantics (no delete attempted → guard skipped).
Worth noting that this escape hatch is *not* a mitigation for this issue; it makes it strictly
worse, since it skips the delete entirely.
- In the current PR #14957, non-Windows conservatively refuses an in-place overwrite whenever the
guard observes that a destination still exists after the delete attempt. This also affects
ordinary files, because this implementation does not query the Unix hard-link count. For
example, a non-writable containing directory can prevent unlinking a writable file while
allowing its contents to be overwritten. This is broader than Windows link detection.
- Builds that today silently corrupt the cache will start failing with MSB3027. That is the intended
outcome, and it matches what those same builds already do when the copy itself is blocked.

### Regression test

The following fails on current `main` and passes with either option applied:

src/Tasks.UnitTests/Copy_Tests.cs

```csharp
[WindowsOnlyFact]
public void DoNotWriteThroughHardLinkWhenDestinationCannotBeDeleted()
{
using TestEnvironment env = TestEnvironment.Create(_testOutputHelper);

TransientTestFolder folder = env.CreateFolder(createFolder: true);
string linkedFile = Path.Combine(folder.Path, "linked.dll"); // stands in for the NuGet cache file
string destination = Path.Combine(folder.Path, "destination.dll");
string source = Path.Combine(folder.Path, "source.dll");

const string LinkedContents = "This file is shared with the NuGet cache.";
const string SourceContents = "This is a completely different file.";

File.WriteAllText(linkedFile, LinkedContents);
File.WriteAllText(source, SourceContents);

var task = new Copy
{
TaskEnvironment = TaskEnvironmentHelper.CreateForTest(),
BuildEngine = new MockEngine(_testOutputHelper),
RetryDelayMilliseconds = 1,
Retries = 0,
SourceFiles = new ITaskItem[] { new TaskItem(source) },
DestinationFiles = new ITaskItem[] { new TaskItem(destination) },
};

string linkError = string.Empty;
if (!Tasks.NativeMethods.MakeHardLink(destination, linkedFile, ref linkError, task.Log))
{
// Hard links are not available here (e.g. non-NTFS volume); nothing to verify.
return;
}

// FileShare.ReadWrite does not include FileShare.Delete, so DeleteFile on the destination
// fails with a sharing violation while CopyFile onto it still succeeds.
using (File.Open(destination, FileMode.Open, FileAccess.ReadWrite, FileShare.ReadWrite))
{
task.Execute();
}

File.ReadAllText(linkedFile).ShouldBe(
LinkedContents,
"Copy overwrote the destination in place and corrupted the file it was hard linked to.");
}
```

Verified against a from-source build of `main` (18.12.0.45701): the test fails before the change and
passes after, and the full `Copy_Tests` class stays green (150 tests: 145 passed, 5 skipped, 0
failed). Re-running the repro against the patched build turns variant C from "exit 0, cache
corrupted" into "exit 1, MSB3021 + MSB3027, cache intact", and the end-to-end NuGet scenario from
"exit 0, 13.0.1 asset replaced" into "exit 1, packages folder intact".

### Reproducer

repro.proj

```xml



```

hold-file.ps1 — helper that holds the destination open

```powershell
param(
[Parameter(Mandatory)][string] $Path,
[Parameter(Mandatory)][string] $Access, # ReadWrite
[Parameter(Mandatory)][string] $Share, # ReadWrite <- note: no Delete
[Parameter(Mandatory)][string] $ReadyFile,
[Parameter(Mandatory)][string] $StopFile)

$stream = [System.IO.File]::Open($Path, [System.IO.FileMode]::Open,
[System.IO.FileAccess]$Access, [System.IO.FileShare]$Share)
"pid=$PID" | Set-Content $ReadyFile
while (-not (Test-Path $StopFile)) { Start-Sleep -Milliseconds 100 }
$stream.Dispose()
```

Driver — creates the hard link, runs Copy, compares hashes

```powershell
$root = Join-Path $env:TEMP ("hardlink-repro-" + [guid]::NewGuid().ToString('N').Substring(0,8))
$env:NUGET_PACKAGES = Join-Path $root '_packages' # never touch the real one
New-Item -ItemType Directory -Force "$root\cache", "$root\bin", $env:NUGET_PACKAGES | Out-Null

$cache = "$root\cache\A.dll"; $dest = "$root\bin\A.dll"; $src = "$root\B.dll"
[IO.File]::WriteAllBytes($cache, (,[byte]0xAA * 4096))
[IO.File]::WriteAllBytes($src, (,[byte]0xBB * 4096))
cmd /c mklink /H $dest $cache | Out-Null

$before = (Get-FileHash $cache -Algorithm SHA256).Hash
$ready = "$root\ready"; $stop = "$root\stop"
$h = Start-Process powershell -PassThru -WindowStyle Hidden -ArgumentList @(
'-NoProfile','-File','.\hold-file.ps1','-Path',$dest,
'-Access','ReadWrite','-Share','ReadWrite','-ReadyFile',$ready,'-StopFile',$stop)
while (-not (Test-Path $ready)) { Start-Sleep -Milliseconds 50 }

dotnet msbuild .\repro.proj "-p:SourceFile=$src" "-p:DestinationFile=$dest" -nologo
"build exit code : $LASTEXITCODE"

New-Item -ItemType File $stop -Force | Out-Null; $h.WaitForExit()

"cache before : $before"
"cache after : $((Get-FileHash $cache -Algorithm SHA256).Hash)"
"hard links : $((fsutil hardlink list $cache).Count)"
```

Swap `-Share ReadWrite` for `-Share 'ReadWrite,Delete'` to get variant D (correct behavior), or drop
the helper entirely for variant A.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/Tasks/Copy.cs at the delete-first logic around lines 322-328 and the File.Copy call around lines 377-383, then read DeleteNoThrow in src/Framework/FileUtilities.cs and the DoCopyWithRetries path near line 1003. Use the standalone Windows reproducer to verify that a surviving hard-linked destination is not overwritten and that the build reports a failure instead of silently corrupting the cache.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
build-system, operating-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.