Skip to content

Commit

Permalink
Verify paths are not the same Fixes #8684 (#8685)
Browse files Browse the repository at this point in the history 
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.)
  • Loading branch information
@Forgind
Forgind authored May 5, 2023
1 parent 280393a commit 0809a23
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 59 deletions.
3 changes: 3 additions & 0 deletions documentation/wiki/ChangeWaves.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
### 17.8
- [[RAR] Don't do I/O on SDK-provided references](https://github.com/dotnet/msbuild/pull/8688)

### 17.8
- [Delete destination file before copy](https://github.com/dotnet/msbuild/pull/8685)

### 17.6
- [Parse invalid property under target](https://github.com/dotnet/msbuild/pull/8190)
- [Eliminate project string cache](https://github.com/dotnet/msbuild/pull/7965)
Expand Down
76 changes: 76 additions & 0 deletions src/Tasks.UnitTests/Copy_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2255,6 +2255,82 @@ public void InvalidErrorIfLinkFailed()
Assert.False(result);
engine.AssertLogContains("MSB3892");
}

/// <summary>
/// An existing link source should not be modified.
/// </summary>
/// <remarks>
/// Related to issue [#8273](https://github.com/dotnet/msbuild/issues/8273)
/// </remarks>
[Theory]
[InlineData(false, false)]
[InlineData(false, true)]
[InlineData(true, false)]
public void DoNotCorruptSourceOfLink(bool useHardLink, bool useSymbolicLink)
{
using TestEnvironment env = TestEnvironment.Create();
TransientTestFile sourceFile1 = env.CreateFile("source1.tmp", "This is the first source temp file."); // HIGHCHAR: Test writes in UTF8 without preamble.
TransientTestFile sourceFile2 = env.CreateFile("source2.tmp", "This is the second source temp file."); // HIGHCHAR: Test writes in UTF8 without preamble.
TransientTestFolder destFolder = env.CreateFolder(createFolder: false);
string destFile = Path.Combine(destFolder.Path, "The Destination");

// Don't create the dest folder, let task do that
ITaskItem[] sourceFiles = { new TaskItem(sourceFile1.Path) };
ITaskItem[] destinationFiles = { new TaskItem(destFile) };

var me = new MockEngine(true);
var t = new Copy
{
RetryDelayMilliseconds = 1, // speed up tests!
BuildEngine = me,
SourceFiles = sourceFiles,
DestinationFiles = destinationFiles,
SkipUnchangedFiles = true,
UseHardlinksIfPossible = useHardLink,
UseSymboliclinksIfPossible = useSymbolicLink,
};

t.Execute().ShouldBeTrue();
File.Exists(destFile).ShouldBeTrue();
File.ReadAllText(destFile).ShouldBe("This is the first source temp file.");

sourceFiles = new TaskItem[] { new TaskItem(sourceFile2.Path) };

t = new Copy
{
RetryDelayMilliseconds = 1, // speed up tests!
BuildEngine = me,
SourceFiles = sourceFiles,
DestinationFiles = destinationFiles,
SkipUnchangedFiles = true,
UseHardlinksIfPossible = false,
UseSymboliclinksIfPossible = false,
};

t.Execute().ShouldBeTrue();
File.Exists(destFile).ShouldBeTrue();
File.ReadAllText(destFile).ShouldBe("This is the second source temp file.");

// Read the source file (it should not have been overwritten)
File.ReadAllText(sourceFile1.Path).ShouldBe("This is the first source temp file.");
((MockEngine)t.BuildEngine).AssertLogDoesntContain("MSB3026"); // Didn't do retries

destinationFiles = new TaskItem[] { new TaskItem(
Path.Combine(Path.GetDirectoryName(sourceFile2.Path), ".", Path.GetFileName(sourceFile2.Path))) // sourceFile2.Path with a "." inserted before the file name
};

t = new Copy
{
RetryDelayMilliseconds = 1, // speed up tests!
BuildEngine = me,
SourceFiles = sourceFiles,
DestinationFiles = destinationFiles,
SkipUnchangedFiles = true,
};

t.Execute().ShouldBeTrue();
File.Exists(sourceFile2.Path).ShouldBeTrue();
}
}

public class CopyHardLink_Tests : Copy_Tests
Expand Down
99 changes: 40 additions & 59 deletions src/Tasks/Copy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,9 @@ private void LogDiagnostic(string message, params object[] messageArgs)
/// </summary>
/// <returns>Return true to indicate success, return false to indicate failure and NO retry, return NULL to indicate retry.</returns>
private bool? CopyFileWithLogging(
FileState sourceFileState, // The source file
FileState destinationFileState) // The destination file
FileState sourceFileState,
FileState destinationFileState)
{
bool destinationFileExists = false;

if (destinationFileState.DirectoryExists)
{
Log.LogErrorWithCodeFromResources("Copy.DestinationIsDirectory", sourceFileState.Name, destinationFileState.Name);
Expand Down Expand Up @@ -278,16 +276,18 @@ private void LogDiagnostic(string message, params object[] messageArgs)

if (FailIfNotIncremental)
{
string sourceFilePath = FileUtilities.GetFullPathNoThrow(sourceFileState.Name);
string destinationFilePath = FileUtilities.GetFullPathNoThrow(destinationFileState.Name);
Log.LogError(FileComment, sourceFilePath, destinationFilePath);
Log.LogError(FileComment, sourceFileState.FileNameFullPath, destinationFileState.FileNameFullPath);
return false;
}

if (OverwriteReadOnlyFiles)
{
MakeFileWriteable(destinationFileState, true);
destinationFileExists = destinationFileState.FileExists;
}

if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) && destinationFileState.FileExists && !destinationFileState.IsReadOnly)
{
FileUtilities.DeleteNoThrow(destinationFileState.Name);
}

bool symbolicLinkCreated = false;
Expand All @@ -297,38 +297,39 @@ private void LogDiagnostic(string message, params object[] messageArgs)
// Create hard links if UseHardlinksIfPossible is true
if (UseHardlinksIfPossible)
{
TryCopyViaLink(HardLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, ref destinationFileExists, out hardLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethods.MakeHardLink(destination, source, ref errorMessage, Log));
TryCopyViaLink(HardLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, out hardLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethods.MakeHardLink(destination, source, ref errorMessage, Log));
if (!hardLinkCreated)
{
if (UseSymboliclinksIfPossible)
{
// This is a message for fallback to SymbolicLinks if HardLinks fail when UseHardlinksIfPossible and UseSymboliclinksIfPossible are true
Log.LogMessage(MessageImportance.Normal, RetryingAsSymbolicLink, sourceFileState.Name, destinationFileState.Name, errorMessage);
Log.LogMessage(MessageImportance.Normal, RetryingAsSymbolicLink, sourceFileState.FileNameFullPath, destinationFileState.FileNameFullPath, errorMessage);
}
else
{
Log.LogMessage(MessageImportance.Normal, RetryingAsFileCopy, sourceFileState.Name, destinationFileState.Name, errorMessage);
Log.LogMessage(MessageImportance.Normal, RetryingAsFileCopy, sourceFileState.FileNameFullPath, destinationFileState.FileNameFullPath, errorMessage);
}
}
}

// Create symbolic link if UseSymboliclinksIfPossible is true and hard link is not created
if (!hardLinkCreated && UseSymboliclinksIfPossible)
{
TryCopyViaLink(SymbolicLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, ref destinationFileExists, out symbolicLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethodsShared.MakeSymbolicLink(destination, source, ref errorMessage));
if (!NativeMethodsShared.IsWindows)
{
errorMessage = Log.FormatResourceString("Copy.NonWindowsLinkErrorMessage", "symlink()", errorMessage);
}
TryCopyViaLink(SymbolicLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, out symbolicLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethodsShared.MakeSymbolicLink(destination, source, ref errorMessage));
if (!symbolicLinkCreated)
{
Log.LogMessage(MessageImportance.Normal, RetryingAsFileCopy, sourceFileState.Name, destinationFileState.Name, errorMessage);
if (!NativeMethodsShared.IsWindows)
{
errorMessage = Log.FormatResourceString("Copy.NonWindowsLinkErrorMessage", "symlink()", errorMessage);
}

Log.LogMessage(MessageImportance.Normal, RetryingAsFileCopy, sourceFileState.FileNameFullPath, destinationFileState.FileNameFullPath, errorMessage);
}
}

if (ErrorIfLinkFails && !hardLinkCreated && !symbolicLinkCreated)
{
Log.LogErrorWithCodeFromResources("Copy.LinkFailed", sourceFileState.Name, destinationFileState.Name);
Log.LogErrorWithCodeFromResources("Copy.LinkFailed", sourceFileState.FileNameFullPath, destinationFileState.FileNameFullPath);
return false;
}

Expand All @@ -337,46 +338,31 @@ private void LogDiagnostic(string message, params object[] messageArgs)
if (!hardLinkCreated && !symbolicLinkCreated)
{
// Do not log a fake command line as well, as it's superfluous, and also potentially expensive
string sourceFilePath = FileUtilities.GetFullPathNoThrow(sourceFileState.Name);
string destinationFilePath = FileUtilities.GetFullPathNoThrow(destinationFileState.Name);
Log.LogMessage(MessageImportance.Normal, FileComment, sourceFilePath, destinationFilePath);
Log.LogMessage(MessageImportance.Normal, FileComment, sourceFileState.FileNameFullPath, destinationFileState.FileNameFullPath);

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

// Files were successfully copied or linked. Those are equivalent here.
WroteAtLeastOneFile = true;

destinationFileState.Reset();

// If the destinationFile file exists, then make sure it's read-write.
// The File.Copy command copies attributes, but our copy needs to
// leave the file writeable.
if (sourceFileState.IsReadOnly)
{
destinationFileState.Reset();
MakeFileWriteable(destinationFileState, false);
}

// Files were successfully copied or linked. Those are equivalent here.
WroteAtLeastOneFile = true;

return true;
}

private void TryCopyViaLink(string linkComment, MessageImportance messageImportance, FileState sourceFileState, FileState destinationFileState, ref bool destinationFileExists, out bool linkCreated, ref string errorMessage, Func<string, string, string, bool> createLink)
private void TryCopyViaLink(string linkComment, MessageImportance messageImportance, FileState sourceFileState, FileState destinationFileState, out bool linkCreated, ref string errorMessage, Func<string, string, string, bool> createLink)
{
// Do not log a fake command line as well, as it's superfluous, and also potentially expensive
Log.LogMessage(MessageImportance.Normal, linkComment, sourceFileState.Name, destinationFileState.Name);

if (!OverwriteReadOnlyFiles)
{
destinationFileExists = destinationFileState.FileExists;
}

// CreateHardLink and CreateSymbolicLink cannot overwrite an existing file or link
// so we need to delete the existing entry before we create the hard or symbolic link.
if (destinationFileExists)
{
FileUtilities.DeleteNoThrow(destinationFileState.Name);
}

linkCreated = createLink(sourceFileState.Name, destinationFileState.Name, errorMessage);
}

Expand Down Expand Up @@ -758,14 +744,7 @@ private bool DoCopyIfNecessary(FileState sourceFileState, FileState destinationF
"true");
MSBuildEventSource.Log.CopyUpToDateStop(destinationFileState.Name, true);
}

// We only do the cheap check for identicalness here, we try the more expensive check
// of comparing the fullpaths of source and destination to see if they are identical,
// in the exception handler lower down.
else if (!String.Equals(
sourceFileState.Name,
destinationFileState.Name,
StringComparison.OrdinalIgnoreCase))
else if (!PathsAreIdentical(sourceFileState, destinationFileState))
{
MSBuildEventSource.Log.CopyUpToDateStop(destinationFileState.Name, false);

Expand Down Expand Up @@ -854,6 +833,11 @@ private bool DoCopyWithRetries(FileState sourceFileState, FileState destinationF
LogDiagnostic("Retrying on ERROR_ACCESS_DENIED because MSBUILDALWAYSRETRY = 1");
}
}
else if (code == NativeMethods.ERROR_INVALID_FILENAME)
{
// Invalid characters used in file name; no point retrying.
throw;
}

if (e is UnauthorizedAccessException)
{
Expand All @@ -867,13 +851,6 @@ private bool DoCopyWithRetries(FileState sourceFileState, FileState destinationF
throw;
}

// if this was just because the source and destination files are the
// same file, that's not a failure.
// Note -- we check this exceptional case here, not before the copy, for perf.
if (PathsAreIdentical(sourceFileState.Name, destinationFileState.Name))
{
return true;
}
break;
}

Expand Down Expand Up @@ -971,12 +948,16 @@ public override bool Execute()
/// Compares two paths to see if they refer to the same file. We can't solve the general
/// canonicalization problem, so we just compare strings on the full paths.
/// </summary>
private static bool PathsAreIdentical(string source, string destination)
private static bool PathsAreIdentical(FileState source, FileState destination)
{
string fullSourcePath = Path.GetFullPath(source);
string fullDestinationPath = Path.GetFullPath(destination);
StringComparison filenameComparison = NativeMethodsShared.IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
return String.Equals(fullSourcePath, fullDestinationPath, filenameComparison);
if (string.Equals(source.Name, destination.Name, FileUtilities.PathComparison))
{
return true;
}

source.FileNameFullPath = Path.GetFullPath(source.Name);
destination.FileNameFullPath = Path.GetFullPath(destination.Name);
return string.Equals(source.FileNameFullPath, destination.FileNameFullPath, FileUtilities.PathComparison);
}

private static int GetParallelismFromEnvironment()
Expand Down
5 changes: 5 additions & 0 deletions src/Tasks/FileState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ public void ThrowException()
/// </summary>
private readonly string _filename;

/// <summary>
/// Holds the full path equivalent of _filename
/// </summary>
public string FileNameFullPath;

/// <summary>
/// Actual file or directory information
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions src/Tasks/NativeMethods.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,7 @@ internal static class NativeMethods

internal const int HRESULT_E_CLASSNOTREGISTERED = -2147221164;

internal const int ERROR_INVALID_FILENAME = -2147024773; // Illegal characters in name
internal const int ERROR_ACCESS_DENIED = -2147024891; // ACL'd or r/o
internal const int ERROR_SHARING_VIOLATION = -2147024864; // File locked by another use

Expand Down

0 comments on commit 0809a23

Please sign in to comment.