Fix for self-copy deleting the source file in CoW copying (#510)

erikmav · web-flow · commit 3c00cb0fb8e5 · 2023-12-06T16:23:03.000-08:00
In CoW Copy and Artifacts, do not copy when the source and destination paths are the same (canonicalized) to avoid deleting the source file when a copy-on-self is happening. This does not handle the case of the same file via differing symlinked paths.
diff --git a/src/Artifacts.UnitTests/RobocopyTests.cs b/src/Artifacts.UnitTests/RobocopyTests.cs
@@ -350,6 +350,45 @@ public void SelfCopiesShouldNoOp()
             fs.NumCopyFileCalls.ShouldBe(0);
         }
 
+        [Fact]
+        public void ExplicitSelfCopiesShouldNoOp()
+        {
+            // Use a relative path to allow aliasing a local file and its full path.
+            string localSource = Path.Combine(Environment.CurrentDirectory, "foo.txt");
+            File.WriteAllText(localSource, string.Empty);
+
+            BuildEngine buildEngine = BuildEngine.Create();
+            MockFileSystem fs = new MockFileSystem();
+
+            Robocopy copyArtifacts = new Robocopy
+            {
+                BuildEngine = buildEngine,
+                Sources = new ITaskItem[]
+                {
+                    new MockTaskItem(localSource)
+                    {
+                        ["DestinationFolder"] = ".",
+                        ["AlwaysCopy"] = "true",
+                    },
+                    new MockTaskItem(Path.Combine(".", "foo.txt"))
+                    {
+                        ["DestinationFolder"] = Environment.CurrentDirectory,
+                        ["AlwaysCopy"] = "true",
+                    },
+                },
+                Sleep = _ => { },
+                FileSystem = fs,
+            };
+
+            copyArtifacts.Execute().ShouldBeTrue(buildEngine.GetConsoleLog());
+            copyArtifacts.NumFilesCopied.ShouldBe(0);
+            copyArtifacts.NumErrors.ShouldBe(0);
+            copyArtifacts.NumFilesSkipped.ShouldBe(1);
+            copyArtifacts.NumDuplicateDestinationDelayedJobs.ShouldBe(0);
+            fs.NumCloneFileCalls.ShouldBe(0);
+            fs.NumCopyFileCalls.ShouldBe(0);
+        }
+
         [Fact]
         public void DifferentSourcesSameDestinationShouldRunDuplicatesSeparately()
         {
diff --git a/src/Artifacts/Tasks/RobocopyMetadata.cs b/src/Artifacts/Tasks/RobocopyMetadata.cs
@@ -264,6 +264,13 @@ public bool IsMatch(string item, string subDirectory, bool isFile)
 
         public bool ShouldCopy(IFileSystem fileSystem, FileInfo source, FileInfo dest)
         {
+            if (string.Equals(source.FullName, dest.FullName, FileSystem.PathComparison))
+            {
+                // Self-copy makes no sense.
+                // TODO: This does not handle the case where the file is the same via different directory symlinks/junctions.
+                return false;
+            }
+
             if (AlwaysCopy || !fileSystem.FileExists(dest))
             {
                 return true;
diff --git a/src/CopyOnWrite.UnitTests/CopyExceptionHandlingTests.cs b/src/CopyOnWrite.UnitTests/CopyExceptionHandlingTests.cs
@@ -12,16 +12,6 @@ namespace Microsoft.Build.CopyOnWrite.UnitTests;
 
 public class CopyExceptionHandlingTests : MSBuildSdkTestBase
 {
-    [Fact]
-    public void PathsAreIdentical_NoSymlinks()
-    {
-        string osRoot = IsWindows ? @"C:\" : "/";
-        string osCapitalizationDependentName = IsWindows ? "NAME" : "name";
-        Assert.True(CopyExceptionHandling.PathsAreIdentical(osRoot, osRoot));
-        Assert.True(CopyExceptionHandling.PathsAreIdentical(Path.Combine(osRoot, "name"), Path.Combine(osCapitalizationDependentName)));
-        Assert.True(CopyExceptionHandling.PathsAreIdentical(osRoot, Path.Combine(osRoot, "subDir", "..")));
-    }
-
     [Fact]
     public void PathsAreIdentical_Symlinks()
     {
@@ -33,25 +23,25 @@ public void PathsAreIdentical_Symlinks()
         using var tempDir = new DisposableTempDirectory();
         string regularFilePath = Path.Combine(tempDir.Path, "regular.txt");
         File.WriteAllText(regularFilePath, "regular");
-        Assert.True(CopyExceptionHandling.PathsAreIdentical(regularFilePath, regularFilePath));
+        Assert.True(CopyExceptionHandling.FullPathsAreIdentical(regularFilePath, regularFilePath));
 
         string regularFilePathWithNonCanonicalSegments =
             Path.Combine(tempDir.Path, "..", Path.GetFileName(tempDir.Path), ".", "regular.txt");
-        Assert.True(CopyExceptionHandling.PathsAreIdentical(regularFilePath, regularFilePathWithNonCanonicalSegments));
+        Assert.True(CopyExceptionHandling.FullPathsAreIdentical(regularFilePath, regularFilePathWithNonCanonicalSegments));
 
         string symlinkPath = Path.Combine(tempDir.Path, "symlink_to_regular.txt");
         string? errorMessage = null;
         bool linkCreated = NativeMethods.MakeSymbolicLink(symlinkPath, regularFilePath, ref errorMessage);
         Assert.True(linkCreated);
         Assert.Null(errorMessage);
-        Assert.True(CopyExceptionHandling.PathsAreIdentical(regularFilePath, symlinkPath));
+        Assert.True(CopyExceptionHandling.FullPathsAreIdentical(regularFilePath, symlinkPath));
 
         File.Delete(symlinkPath);
         errorMessage = null;
         linkCreated = NativeMethods.MakeSymbolicLink(symlinkPath, regularFilePathWithNonCanonicalSegments, ref errorMessage);
         Assert.True(linkCreated);
         Assert.Null(errorMessage);
-        Assert.True(CopyExceptionHandling.PathsAreIdentical(regularFilePath, symlinkPath));
+        Assert.True(CopyExceptionHandling.FullPathsAreIdentical(regularFilePath, symlinkPath));
     }
 
     private bool UserCanCreateSymlinks()
diff --git a/src/CopyOnWrite/Copy.cs b/src/CopyOnWrite/Copy.cs
@@ -13,7 +13,7 @@
 using System.Threading;
 using System.Threading.Tasks.Dataflow;
 
-#nullable disable
+#nullable enable annotations
 
 namespace Microsoft.Build.Tasks
 {
@@ -50,28 +50,16 @@ public class Copy : Task, ICancelableTask
         public Copy()
         {
             RetryDelayMilliseconds = RetryDelayMillisecondsDefault;
-
-            if (DidNotCopyBecauseOfFileMatch == null)
-            {
-                CreatesDirectory = "Creating directory \"{0}\"";
-                DidNotCopyBecauseOfFileMatch = "Did not copy from file \"{0}\" to file \"{1}\" because the \"{2}\" parameter was set to \"{3}\" in the project and the files' sizes and timestamps match.";
-                FileComment = "Copying file from \"{0}\" to \"{1}\".";
-                HardLinkComment = "Creating hard link to copy \"{0}\" to \"{1}\"";
-                RetryingAsFileCopy = "Could not use a link to copy \"{0}\" to \"{1}\". Copying the file instead. {2}";
-                RetryingAsSymbolicLink = "Could not use a hard link to copy \"{0}\" to \"{1}\". Copying the file with symbolic link instead. {2}";
-                RemovingReadOnlyAttribute = "Removing read-only attribute from \"{0}\".";
-                SymbolicLinkComment = "Creating symbolic link to copy \"{0}\" to \"{1}\"";
-            }
         }
 
-        private static string CreatesDirectory;
-        private static string DidNotCopyBecauseOfFileMatch;
-        private static string FileComment;
-        private static string HardLinkComment;
-        private static string RetryingAsFileCopy;
-        private static string RetryingAsSymbolicLink;
-        private static string RemovingReadOnlyAttribute;
-        private static string SymbolicLinkComment;
+        private static readonly string CreatesDirectory = "Creating directory \"{0}\"";
+        private static readonly string DidNotCopyBecauseOfFileMatch = "Did not copy from file \"{0}\" to file \"{1}\" because the \"{2}\" parameter was set to \"{3}\" in the project and the files' sizes and timestamps match.";
+        private static readonly string FileComment = "Copying file from \"{0}\" to \"{1}\".";
+        private static readonly string HardLinkComment = "Creating hard link to copy \"{0}\" to \"{1}\"";
+        private static readonly string RetryingAsFileCopy = "Could not use a link to copy \"{0}\" to \"{1}\". Copying the file instead. {2}";
+        private static readonly string RetryingAsSymbolicLink = "Could not use a hard link to copy \"{0}\" to \"{1}\". Copying the file with symbolic link instead. {2}";
+        private static readonly string RemovingReadOnlyAttribute = "Removing read-only attribute from \"{0}\".";
+        private static readonly string SymbolicLinkComment = "Creating symbolic link to copy \"{0}\" to \"{1}\"";
 
         #region Properties
 
@@ -100,9 +88,9 @@ public Copy()
         private const int RetryDelayMillisecondsDefault = 1000;
 
         [Required]
-        public ITaskItem[] SourceFiles { get; set; }
+        public ITaskItem[]  SourceFiles { get; set; }
 
-        public ITaskItem DestinationFolder { get; set; }
+        public ITaskItem? DestinationFolder { get; set; }
 
         /// <summary>
         /// Gets or sets the number of times to attempt to copy, if all previous attempts failed.
@@ -136,7 +124,7 @@ public Copy()
         public bool SkipUnchangedFiles { get; set; }
 
         [Output]
-        public ITaskItem[] DestinationFiles { get; set; }
+        public ITaskItem[]  DestinationFiles { get; set; }
 
         /// <summary>
         /// The subset of files that were successfully copied.
@@ -333,8 +321,8 @@ 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);
+                string sourceFilePath = sourceFileState.FullPath;
+                string destinationFilePath = destinationFileState.FullPath;
                 Log.LogMessage(MessageImportance.Normal, FileComment, sourceFilePath, destinationFilePath);
 
                 bool cloned = false;
@@ -438,7 +426,7 @@ internal bool Execute(
             bool success;
             try
             {
-                success = parallelism == 1 || DestinationFiles.Length == 1
+                success = parallelism == 1 || DestinationFiles!.Length == 1
                     ? CopySingleThreaded(copyFile, out destinationFilesSuccessfullyCopied)
                     : CopyParallel(copyFile, parallelism, out destinationFilesSuccessfullyCopied);
             }
@@ -462,7 +450,7 @@ private bool CopySingleThreaded(
             out List<ITaskItem> destinationFilesSuccessfullyCopied)
         {
             bool success = true;
-            destinationFilesSuccessfullyCopied = new List<ITaskItem>(DestinationFiles.Length);
+            destinationFilesSuccessfullyCopied = new List<ITaskItem>(DestinationFiles!.Length);
 
             // Set of files we actually copied and the location from which they were originally copied.  The purpose
             // of this collection is to let us skip copying duplicate files.  We will only copy the file if it 
@@ -474,7 +462,7 @@ private bool CopySingleThreaded(
                 StringComparer.OrdinalIgnoreCase);
 
             // Now that we have a list of destinationFolder files, copy from source to destinationFolder.
-            for (int i = 0; i < SourceFiles.Length && !_cancellationTokenSource.IsCancellationRequested; ++i)
+            for (int i = 0; i < SourceFiles!.Length && !_cancellationTokenSource.IsCancellationRequested; ++i)
             {
                 bool copyComplete = false;
                 string destPath = DestinationFiles[i].ItemSpec;
@@ -544,10 +532,10 @@ private bool CopyParallel(
 
             // Map: Destination path -> indexes in SourceFiles/DestinationItems array indices (ordered low->high).
             var partitionsByDestination = new Dictionary<string, List<int>>(
-                DestinationFiles.Length, // Set length to common case of 1:1 source->dest.
+                DestinationFiles!.Length, // Set length to common case of 1:1 source->dest.
                 StringComparer.OrdinalIgnoreCase);
 
-            for (int i = 0; i < SourceFiles.Length && !_cancellationTokenSource.IsCancellationRequested; ++i)
+            for (int i = 0; i < SourceFiles!.Length && !_cancellationTokenSource.IsCancellationRequested; ++i)
             {
                 ITaskItem destItem = DestinationFiles[i];
                 string destPath = destItem.ItemSpec;
@@ -680,7 +668,7 @@ private bool ValidateInputs()
             }
 
             // If the caller passed in DestinationFiles, then its length must match SourceFiles.
-            if (DestinationFiles != null && DestinationFiles.Length != SourceFiles.Length)
+            if (DestinationFiles != null && DestinationFiles.Length != SourceFiles!.Length)
             {
                 LogError(
                     "MSB3094: \"{2}\" refers to {0} item(s), and \"{3}\" refers to {1} item(s). They must have the same number of items.",
@@ -706,19 +694,19 @@ private bool InitializeDestinationFiles()
             if (DestinationFiles == null)
             {
                 // If the caller passed in DestinationFolder, convert it to DestinationFiles
-                DestinationFiles = new ITaskItem[SourceFiles.Length];
+                DestinationFiles = new ITaskItem[SourceFiles!.Length];
 
                 for (int i = 0; i < SourceFiles.Length; ++i)
                 {
                     // Build the correct path.
                     string destinationFile;
                     try
                     {
-                        destinationFile = Path.Combine(DestinationFolder.ItemSpec, Path.GetFileName(SourceFiles[i].ItemSpec));
+                        destinationFile = Path.Combine(DestinationFolder!.ItemSpec, Path.GetFileName(SourceFiles[i].ItemSpec));
                     }
                     catch (ArgumentException e)
                     {
-                        LogError("MSB3021: Unable to copy file \"{0}\" to \"{1}\". {2}", SourceFiles[i].ItemSpec, DestinationFolder.ItemSpec, e.Message);
+                        LogError("MSB3021: Unable to copy file \"{0}\" to \"{1}\". {2}", SourceFiles![i].ItemSpec!, DestinationFolder!.ItemSpec, e.Message);
                         // Clear the outputs.
                         DestinationFiles = Array.Empty<ITaskItem>();
                         return false;
@@ -761,13 +749,9 @@ private bool DoCopyIfNecessary(FileState sourceFileState, FileState destinationF
                         "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))
+                // We only do a cheap check for path equality here, we try the more expensive check
+                // of comparing the underlying file path (if symlinks/junctions are in use) in the exception handler lower down.
+                else if (!string.Equals(sourceFileState.FullPath, destinationFileState.FullPath, StringComparison.OrdinalIgnoreCase))
                 {
                     success = DoCopyWithRetries(sourceFileState, destinationFileState, copyFile);
                 }
@@ -864,7 +848,8 @@ private bool DoCopyWithRetries(FileState sourceFileState, FileState destinationF
                             // 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 (CopyExceptionHandling.PathsAreIdentical(sourceFileState.Name, destinationFileState.Name))
+                            // TODO: This does not handle the case of having deleted the destination file prior to trying to copy - we can delete the destination and it happens to be the source as well!
+                            if (CopyExceptionHandling.FullPathsAreIdentical(sourceFileState.FullPath, destinationFileState.FullPath))
                             {
                                 return true;
                             }
@@ -1023,11 +1008,11 @@ private bool TryCopyOnWrite(string sourceFullPath, string destFullPath)
             try
             {
                 CoW.CloneFile(sourceFullPath, destFullPath, CloneFlags.PathIsFullyResolved);
-                Log.LogMessage(MessageImportance.Low, $"CloneFile '{sourceFullPath}' to '{destFullPath}'.");
+                Log.LogMessage(MessageImportance.Low, $"Created copy-on-write link '{sourceFullPath}' to '{destFullPath}'.");
             }
             catch (Exception e)
             {
-                Log.LogMessage($"Could not CloneFile '{sourceFullPath}' to '{destFullPath}'. Reason: {e.Message}");
+                Log.LogMessage($"Could not create copy-on-write link '{sourceFullPath}' to '{destFullPath}'. Reason: {e.Message}");
                 return false;
             }
 
diff --git a/src/CopyOnWrite/MSBuild/FileState.cs b/src/CopyOnWrite/MSBuild/FileState.cs
@@ -42,6 +42,11 @@ private class FileDirInfo
             /// </summary>
             private readonly string _filename;
 
+            /// <summary>
+            /// The fully resolved path (via Path.GetFullPath).
+            /// </summary>
+            public readonly string FullPath;
+
             /// <summary>
             /// Set to true if file or directory exists
             /// </summary>
@@ -85,6 +90,7 @@ public FileDirInfo(string filename)
                 LastWriteTimeUtc = new DateTime(1601, 1, 1);
 
                 _filename = FileUtilities.AttemptToShortenPath(filename); // This is no-op unless the path actually is too long
+                FullPath = FileUtilities.NormalizePath(_filename);
 
                 int oldMode = 0;
 
@@ -304,10 +310,19 @@ internal long Length
 
         /// <summary>
         /// Name of the file as it was passed in.
-        /// Not normalized.
+        /// Not normalized unless the original path was too long.
         /// </summary>
         internal string Name => _filename;
 
+        internal string FullPath
+        {
+            get
+            {
+                _data.Value.ThrowException();
+                return _data.Value.FullPath;
+            }
+        }
+
         /// <summary>
         /// Use in case the state is known to have changed exogenously.
         /// </summary>
diff --git a/src/CopyOnWrite/MSBuild/FileUtilities.cs b/src/CopyOnWrite/MSBuild/FileUtilities.cs
@@ -3,9 +3,10 @@
 
 using Microsoft.Build.Framework;
 using System;
-using System.Collections.Concurrent;
 using System.IO;
 
+#nullable enable
+
 namespace Microsoft.Build.Shared
 {
     /// <summary>
@@ -15,8 +16,6 @@ namespace Microsoft.Build.Shared
     /// </summary>
     internal static class FileUtilities
     {
-        private static readonly ConcurrentDictionary<string, bool> FileExistenceCache = new ConcurrentDictionary<string, bool>(StringComparer.OrdinalIgnoreCase);
-
         /// <summary>
         /// Gets the canonicalized full path of the provided path.
         /// Guidance for use: call this on all paths accepted through internal entry
@@ -26,20 +25,10 @@ internal static class FileUtilities
         /// </summary>
         internal static string NormalizePath(string path)
         {
-            if (path == null)
-            {
-                throw new ArgumentNullException(nameof(path));
-            }
-
-            string fullPath = GetFullPath(path);
+            string fullPath = Path.GetFullPath(path);
             return FixFilePath(fullPath);
         }
 
-        private static string GetFullPath(string path)
-        {
-            return Path.GetFullPath(path);
-        }
-
         internal static string FixFilePath(string path)
         {
             return string.IsNullOrEmpty(path) || Path.DirectorySeparatorChar == '\\' ? path : path.Replace('\\', '/'); // .Replace("//", "/");
diff --git a/src/Shared/CopyExceptionHandling.cs b/src/Shared/CopyExceptionHandling.cs

Original file line number	Diff line number	Diff line change
`@@ -264,6 +264,13 @@ public bool IsMatch(string item, string subDirectory, bool isFile)`
`264`	`264`
`265`	`265`	`public bool ShouldCopy(IFileSystem fileSystem, FileInfo source, FileInfo dest)`
`266`	`266`	`{`
	`267`	`+ if (string.Equals(source.FullName, dest.FullName, FileSystem.PathComparison))`
	`268`	`+ {`
	`269`	`+ // Self-copy makes no sense.`
	`270`	`+ // TODO: This does not handle the case where the file is the same via different directory symlinks/junctions.`
	`271`	`+ return false;`
	`272`	`+ }`
	`273`	`+`
`267`	`274`	`if (AlwaysCopy \|\| !fileSystem.FileExists(dest))`
`268`	`275`	`{`
`269`	`276`	`return true;`