Issue8273 corrupt nu get cache (#8275)

manfred-brands · web-flow · commit a93882f9b954 · 2023-03-15T14:48:12.000+08:00
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.
diff --git a/src/Tasks.UnitTests/Copy_Tests.cs b/src/Tasks.UnitTests/Copy_Tests.cs
@@ -2096,6 +2096,87 @@ 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)
+        {
+            string sourceFile1 = FileUtilities.GetTemporaryFile();
+            string sourceFile2 = FileUtilities.GetTemporaryFile();
+            string temp = Path.GetTempPath();
+            string destFolder = Path.Combine(temp, "2A333ED756AF4dc392E728D0F864A398");
+            string destFile = Path.Combine(destFolder, "The Destination");
+
+            try
+            {
+                File.WriteAllText(sourceFile1, "This is the first source temp file."); // HIGHCHAR: Test writes in UTF8 without preamble.
+                File.WriteAllText(sourceFile2, "This is the second source temp file."); // HIGHCHAR: Test writes in UTF8 without preamble.
+
+                // Don't create the dest folder, let task do that
+
+                ITaskItem[] sourceFiles = { new TaskItem(sourceFile1) };
+                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,
+                };
+
+                bool success = t.Execute();
+
+                Assert.True(success); // "success"
+                Assert.True(File.Exists(destFile)); // "destination exists"
+
+                string destinationFileContents = File.ReadAllText(destFile);
+                Assert.Equal("This is the first source temp file.", destinationFileContents);
+
+                sourceFiles = new TaskItem[] { new TaskItem(sourceFile2) };
+
+                t = new Copy
+                {
+                    RetryDelayMilliseconds = 1, // speed up tests!
+                    BuildEngine = me,
+                    SourceFiles = sourceFiles,
+                    DestinationFiles = destinationFiles,
+                    SkipUnchangedFiles = true,
+                    UseHardlinksIfPossible = false,
+                    UseSymboliclinksIfPossible = false,
+                };
+
+                success = t.Execute();
+
+                Assert.True(success); // "success"
+                Assert.True(File.Exists(destFile)); // "destination exists"
+
+                destinationFileContents = File.ReadAllText(destFile);
+                Assert.Equal("This is the second source temp file.", destinationFileContents);
+
+                // Read the source file (it should not have been overwritten)
+                string sourceFileContents = File.ReadAllText(sourceFile1);
+                Assert.Equal("This is the first source temp file.", sourceFileContents);
+
+                ((MockEngine)t.BuildEngine).AssertLogDoesntContain("MSB3026"); // Didn't do retries
+            }
+            finally
+            {
+                Helpers.DeleteFiles(sourceFile1, sourceFile2, destFile);
+            }
+        }
     }
 
     public class CopyHardLink_Tests : Copy_Tests
diff --git a/src/Tasks/Copy.cs b/src/Tasks/Copy.cs
@@ -226,8 +226,6 @@ private void LogDiagnostic(string message, params object[] messageArgs)
             FileState sourceFileState,      // The source file
             FileState destinationFileState)  // The destination file
         {
-            bool destinationFileExists = false;
-
             if (destinationFileState.DirectoryExists)
             {
                 Log.LogErrorWithCodeFromResources("Copy.DestinationIsDirectory", sourceFileState.Name, destinationFileState.Name);
@@ -269,7 +267,14 @@ private void LogDiagnostic(string message, params object[] messageArgs)
             if (OverwriteReadOnlyFiles)
             {
                 MakeFileWriteable(destinationFileState, true);
-                destinationFileExists = destinationFileState.FileExists;
+            }
+
+            // 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)
+            {
+                FileUtilities.DeleteNoThrow(destinationFileState.Name);
             }
 
             bool symbolicLinkCreated = false;
@@ -279,7 +284,7 @@ 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)
@@ -297,13 +302,14 @@ private void LogDiagnostic(string message, params object[] messageArgs)
             // 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)
                 {
+                    if (!NativeMethodsShared.IsWindows)
+                    {
+                        errorMessage = Log.FormatResourceString("Copy.NonWindowsLinkErrorMessage", "symlink()", errorMessage);
+                    }
+
                     Log.LogMessage(MessageImportance.Normal, RetryingAsFileCopy, sourceFileState.Name, destinationFileState.Name, errorMessage);
                 }
             }
@@ -324,41 +330,28 @@ private void LogDiagnostic(string message, params object[] messageArgs)
                 Log.LogMessage(MessageImportance.Normal, FileComment, sourceFilePath, destinationFilePath);
 
                 File.Copy(sourceFileState.Name, destinationFileState.Name, true);
+
+                // 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;
 
-            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)
-            {
-                MakeFileWriteable(destinationFileState, false);
-            }
-
             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);
         }
 
@@ -826,6 +819,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)
                             {
diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs
@@ -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
 
