Issue8273 corrupt nu get cache #8275

manfred-brands · 2023-01-03T06:08:07Z

Fixes #8273

Context

Prevent overwriting the source of a hard/symbolic link.

Changes Made

Always delete the destination file (unless readonly and OverwriteReadOnlyFiles is false)

Testing

Added unit test.

Notes

Forgind · 2023-01-19T04:41:56Z

I opened dotnet/runtime#80832

Jozkee labeled it 'bug', so I'm guessing this is a runtime bug rather than an MSBuild bug. He also noted that File.Move does not have the same issue. People using MSBuild's Copy often don't care whether the source file exists afterwards, but we can't guarantee that, so I don't think we can use File.Move, unfortunately.

We're still talking internally about whether we want to:

Take this
Close this in favor of the runtime fixing it
Take this but consider reverting it if the runtime changes File.Copy's behavior
Possible other option?

What happens on .NET Framework (and older .NET Core) may be an important question, but I haven't tested that yet.

Sorry we're being a bit slow; I just wanted to give you something of an update 🙂

Forgind · 2023-02-13T17:14:12Z

It looks like runtime isn't planning to change anything in the immediate future.

We're thinking we will likely take this, but our primary concern is performance. @rokonec volunteered to take a quick look into that to see if he can find anything problematic. Additionally, would you mind putting this behind a change wave? That will turn it on by default for all users but provide a way for users to opt out should they hit perf problems.

marcin-krystianc · 2023-02-15T16:25:45Z

src/Tasks/Copy.cs

@@ -324,41 +329,28 @@ private void LogDiagnostic(string message, params object[] messageArgs)
                Log.LogMessage(MessageImportance.Normal, FileComment, sourceFilePath, destinationFilePath);

                File.Copy(sourceFileState.Name, destinationFileState.Name, true);


Given that the destination is always deleted, I think it would be a good idea to set the overwrite flag to false to protect us from accidental overwrites.

Yes, it is probably better to fail in case someone gets in between than overwriting a link. I will update the PR.

I do not believe it is necessary. Sometimes logic might change to not delete it, or new Copy task argument will be introduced, or some weird concurrent logic is relaying with last win. I would recommend to keep it as is, but if you decide to change it, it has to go under same ChangeWave or same conditions as deleting files.

Forgind · 2023-02-27T16:12:14Z

Ping @rokonec

marcin-krystianc · 2023-03-06T11:58:58Z

It looks like runtime isn't planning to change anything in the immediate future.

We're thinking we will likely take this, but our primary concern is performance. @rokonec volunteered to take a quick look into that to see if he can find anything problematic. Additionally, would you mind putting this behind a change wave? That will turn it on by default for all users but provide a way for users to opt out should they hit perf problems.

FWIW, I've run some benchmarks (building a large solution, with and without a change from this PR), and here are my conclusions:

When building on Windows, the variant with forced FileUtilities.DeleteNoThrow is actually faster (2m 25s vs 2m 35s). Unfortunately, I cannot share the tested solution here as it contains IP. I attribute this difference to AntiVirus software as I suspect that it handles file deletes and file overwrites differently.
On Linux I didn't notice any real difference, in both cases, the build time was around 4 minutes:

Case 1 (forced `FileUtilities.DeleteNoThrow`)
Time Elapsed 00:03:59.28
Time Elapsed 00:03:58.44
Time Elapsed 00:04:01.78
Time Elapsed 00:03:57.04
Time Elapsed 00:04:02.73

Case 2:
Time Elapsed 00:03:59.87
Time Elapsed 00:03:56.23
Time Elapsed 00:03:58.41
Time Elapsed 00:03:58.95
Time Elapsed 00:04:01.58

Tested solution:
LargeAppWithPrivatePackagesCentralisedNGBVRemoved.zip
Test command:

cd ~/LargeAppWithPrivatePackagesCentralisedNGBVRemoved/solution$
echo "" >> Directory.Build.props && ~/dotnet/dotnet build -clp:summary --no-restore /bl /p:UseLinks=false

rokonec

I have spent some time to measure it and although it is very slightly slower, it is, IMO, acceptable. It took less then 0.1 % on my tests.
I went away and created some minor suggestions...
Good job.

src/Tasks.UnitTests/Copy_Tests.cs

src/Tasks/Copy.cs

Co-authored-by: Roman Konecny <rokonecn@microsoft.com>

manfred-brands · 2023-03-13T04:41:53Z

@rokonec I have applied your changes and rebased the branch on current main branch.

Forgind

Thanks!

Forgind · 2023-03-13T22:56:47Z

src/Tasks/Copy.cs

+            // If the destination file is a hard or symbolic link, File.Copy would overwrite the source.
+            // To prevent this, we need to delete the existing entry before we Copy or create a link.
+            // We could try to figure out if the file is a link, but I can't think of a reason to not simply delete it always.
+            if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_6) && destinationFileState.FileExists && !destinationFileState.IsReadOnly)


Should we also take into account OverwriteReadOnlyFiles and also delete them in that case?

if OverwriteReadOnlyFiles is set, the file is made writable and hence deleted here.

...and now I see it 6 lines up. Stupid question, thanks!

Fixes dotnet#8273 Context Prevent overwriting the source of a hard/symbolic link. Changes Made Always delete the destination file (unless readonly and OverwriteReadOnlyFiles is false) Testing Added unit test.

This reverts commit a93882f.

This reverts commit a93882f. This is a temporary fix for #8684 The current plan is to revert #8275 in 17.6, as it caused some difficulties, and try to bring it back in 17.7 via #8685. Summary #8275 fixed a longstanding confusing and unfortunate behavior in MSBuild in which passing the Copy task a symlink as its destination would copy the source file onto the destination of the symlink rather than overwriting the symlink. Unfortunately, it also introduced a new issue in which copying a file onto itself could often just delete the file instead of copying anything. Customers reported this issue. Customer Impact Projects that copy a file onto itself using the Copy task without passing identical paths for source and destination instead delete the file without necessarily even logging an error. Regression? Yes, from #8275. Testing Unit tests and manually tested that the repro described in #8684 no longer works. Risk Minimal (straight revert of the commit that caused the bug) --------- Co-authored-by: Forgind <Forgind@users.noreply.github.com>

Fixes #8684 Fixes #8273 Context After #8275, we delete any destination file as part of the Copy task if we determine that we really should copy onto it. Unfortunately, if we try to copy a file onto itself, we delete it before we can copy onto itself, which just means it's gone. Fortunately, we have a check earlier that ensures that we skip any copy operation from a location to the same location. Unfortunately, it's a direct string comparison that doesn't evaluate to full paths first, so it misses slightly more complicated examples. Changes Made Take into account full paths Testing Unit tests + manual test that it doesn't delete the file anymore Notes This implementation tries to remove now-unnecessary full path derivations downstream, hence some added complexity, but it still means extra computation on the happy path if we end up creating a hard/symbolic link. An alternate direction eliminating any full path derivations on the happy path would save about 4% of Copy's execution time, per a quick perf test. (With how many samples I used, "no change" is within a standard deviation.)

manfred-brands force-pushed the issue8273_corruptNuGetCache branch from 35b3ddd to c88d6a3 Compare January 4, 2023 02:04

Forgind requested a review from rokonec February 13, 2023 17:14

marcin-krystianc reviewed Feb 15, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

rokonec approved these changes Mar 8, 2023

View reviewed changes

src/Tasks.UnitTests/Copy_Tests.cs Outdated Show resolved Hide resolved

src/Tasks/Copy.cs Outdated Show resolved Hide resolved

manfred-brands and others added 3 commits March 13, 2023 12:13

Ensure Copy with invalid file names do not get retries

359f511

Always delete Destination File to prevent corrupting linked files.

864d5b7

Move issue to comment iso method name

cb5d0de

Co-authored-by: Roman Konecny <rokonecn@microsoft.com>

manfred-brands force-pushed the issue8273_corruptNuGetCache branch from 0cdf3d7 to cb5d0de Compare March 13, 2023 04:39

Add ChangeWave for file deletion

c21ac4d

Co-authored-by: Roman Konecny <rokonecn@microsoft.com>

Forgind approved these changes Mar 13, 2023

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 14, 2023

JaynieBai merged commit a93882f into dotnet:main Mar 15, 2023

This was referenced Apr 20, 2023

[Bug]: Copying a file onto itself deletes the file #8684

Closed

Verify paths are not the same Fixes #8684 #8685

Merged

Forgind added a commit to Forgind/msbuild that referenced this pull request Apr 20, 2023

Revert "Issue8273 corrupt nu get cache (dotnet#8275)"

a30c519

This reverts commit a93882f.

Forgind mentioned this pull request Apr 20, 2023

Revert "Issue8273 corrupt nu get cache (#8275)" #8686

Merged

marcin-krystianc mentioned this pull request May 4, 2023

Usage of hard or symbolic linking leads to NuGet cache corruption #8273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue8273 corrupt nu get cache #8275

Issue8273 corrupt nu get cache #8275

manfred-brands commented Jan 3, 2023

Forgind commented Jan 19, 2023 •

edited

Loading

Forgind commented Feb 13, 2023

marcin-krystianc Feb 15, 2023

manfred-brands Feb 22, 2023

rokonec Mar 8, 2023

Forgind commented Feb 27, 2023

marcin-krystianc commented Mar 6, 2023

This comment was marked as outdated.

rokonec left a comment •

edited

Loading

manfred-brands commented Mar 13, 2023

Forgind left a comment

Forgind Mar 13, 2023

manfred-brands Mar 14, 2023

Forgind Mar 14, 2023

		@@ -324,41 +329,28 @@ private void LogDiagnostic(string message, params object[] messageArgs)
		Log.LogMessage(MessageImportance.Normal, FileComment, sourceFilePath, destinationFilePath);

		File.Copy(sourceFileState.Name, destinationFileState.Name, true);

Issue8273 corrupt nu get cache #8275

Issue8273 corrupt nu get cache #8275

Conversation

manfred-brands commented Jan 3, 2023

Context

Changes Made

Testing

Notes

Forgind commented Jan 19, 2023 • edited Loading

Forgind commented Feb 13, 2023

marcin-krystianc Feb 15, 2023

Choose a reason for hiding this comment

manfred-brands Feb 22, 2023

Choose a reason for hiding this comment

rokonec Mar 8, 2023

Choose a reason for hiding this comment

Forgind commented Feb 27, 2023

marcin-krystianc commented Mar 6, 2023

This comment was marked as outdated.

rokonec left a comment • edited Loading

Choose a reason for hiding this comment

manfred-brands commented Mar 13, 2023

Forgind left a comment

Choose a reason for hiding this comment

Forgind Mar 13, 2023

Choose a reason for hiding this comment

manfred-brands Mar 14, 2023

Choose a reason for hiding this comment

Forgind Mar 14, 2023

Choose a reason for hiding this comment

Forgind commented Jan 19, 2023 •

edited

Loading

rokonec left a comment •

edited

Loading