Reverts dotnet/corefx#37583 ("Use clonefile for CopyFile, if available #37583") (#40753)

* Revert " Use clonefile for CopyFile, if available (dotnet/corefx#37583)"

This reverts commit 02ba75a.

* Add unit test

* Disable tests failing in browser-wasm

Author: jozkee

src/libraries/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs

@@ -10,6 +10,6 @@ internal static partial class Interop
     internal static partial class Sys
     {
         [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_CopyFile", SetLastError = true)]
-        internal static extern int CopyFile(SafeFileHandle source, string srcPath, string destPath, int overwrite);
+        internal static extern int CopyFile(SafeFileHandle source, SafeFileHandle destination);
     }
 }

src/libraries/Native/Unix/Common/pal_config.h.in

@@ -53,7 +53,7 @@
 #cmakedefine01 HAVE_SENDFILE_4
 #cmakedefine01 HAVE_SENDFILE_6
 #cmakedefine01 HAVE_SENDFILE_7
-#cmakedefine01 HAVE_CLONEFILE
+#cmakedefine01 HAVE_FCOPYFILE
 #cmakedefine01 HAVE_GETNAMEINFO_SIGNED_FLAGS
 #cmakedefine01 HAVE_GETPEEREID
 #cmakedefine01 HAVE_SUPPORT_FOR_DUAL_MODE_IPV4_PACKET_INFO

src/libraries/Native/Unix/System.Native/pal_io.c

@@ -26,11 +26,9 @@
 #include <termios.h>
 #include <unistd.h>
 #include <limits.h>
-#if HAVE_CLONEFILE
-#include <sys/attr.h>
-#include <sys/clonefile.h>
-#endif
-#if HAVE_SENDFILE_4
+#if HAVE_FCOPYFILE
+#include <copyfile.h>
+#elif HAVE_SENDFILE_4
 #include <sys/sendfile.h>
 #endif
 #if HAVE_INOTIFY
@@ -1020,6 +1018,7 @@ int32_t SystemNative_Write(intptr_t fd, const void* buffer, int32_t bufferSize)
     return Common_Write(fd, buffer, bufferSize);
 }
 
+#if !HAVE_FCOPYFILE
 // Read all data from inFd and write it to outFd
 static int32_t CopyFile_ReadWrite(int inFd, int outFd)
 {
@@ -1072,90 +1071,32 @@ static int32_t CopyFile_ReadWrite(int inFd, int outFd)
     free(buffer);
     return 0;
 }
+#endif // !HAVE_FCOPYFILE
 
-int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char* destPath, int32_t overwrite)
+int32_t SystemNative_CopyFile(intptr_t sourceFd, intptr_t destinationFd)
 {
     int inFd = ToFileDescriptor(sourceFd);
-    int outFd;
+    int outFd = ToFileDescriptor(destinationFd);
+
+#if HAVE_FCOPYFILE
+    // If fcopyfile is available (OS X), try to use it, as the whole copy
+    // can be performed in the kernel, without lots of unnecessary copying.
+    // Copy data and metadata.
+    return fcopyfile(inFd, outFd, NULL, COPYFILE_ALL) == 0 ? 0 : -1;
+#else
+    // Get the stats on the source file.
     int ret;
-    int tmpErrno;
-    int openFlags;
     struct stat_ sourceStat;
-
+    bool copied = false;
+#if HAVE_SENDFILE_4
+    // If sendfile is available (Linux), try to use it, as the whole copy
+    // can be performed in the kernel, without lots of unnecessary copying.
     while ((ret = fstat_(inFd, &sourceStat)) < 0 && errno == EINTR);
     if (ret != 0)
     {
         return -1;
     }
 
-    struct stat_ destStat;
-    while ((ret = stat_(destPath, &destStat)) < 0 && errno == EINTR);
-    if (ret == 0)
-    {
-        if (!overwrite)
-        {
-            errno = EEXIST;
-            return -1;
-        }
-
-        if (sourceStat.st_dev == destStat.st_dev && sourceStat.st_ino == destStat.st_ino)
-        {
-            // Attempt to copy file over itself. Fail with the same error code as
-            // open would.
-            errno = EBUSY;
-            return -1;
-        }
-
-#if HAVE_CLONEFILE
-        // For clonefile we need to unlink the destination file first but we need to
-        // check permission first to ensure we don't try to unlink read-only file.
-        if (access(destPath, W_OK) != 0)
-        {
-            return -1;
-        }
-
-        ret = unlink(destPath);
-        if (ret != 0)
-        {
-            return ret;
-        }
-#endif
-    }
-
-#if HAVE_CLONEFILE
-    while ((ret = clonefile(srcPath, destPath, 0)) < 0 && errno == EINTR);
-    // EEXIST can happen due to race condition between the stat/unlink above
-    // and the clonefile here. The file could be (re-)created from another
-    // thread or process before we have a chance to call clonefile. Handle
-    // it by falling back to the slow path.
-    if (ret == 0 || (errno != ENOTSUP && errno != EXDEV && errno != EEXIST))
-    {
-        return ret;
-    }
-#else
-    // Unused variable
-    (void)srcPath;
-#endif
-
-    openFlags = O_WRONLY | O_TRUNC | O_CREAT | (overwrite ? 0 : O_EXCL);
-#if HAVE_O_CLOEXEC
-    openFlags |= O_CLOEXEC;
-#endif
-    while ((outFd = open(destPath, openFlags, sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))) < 0 && errno == EINTR);
-    if (outFd < 0)
-    {
-        return -1;
-    }
-#if !HAVE_O_CLOEXEC
-    fcntl(outFd, F_SETFD, FD_CLOEXEC);
-#endif
-
-    // Get the stats on the source file.
-    bool copied = false;
-
-    // If sendfile is available (Linux), try to use it, as the whole copy
-    // can be performed in the kernel, without lots of unnecessary copying.
-#if HAVE_SENDFILE_4
 
     // On 32-bit, if you use 64-bit offsets, the last argument of `sendfile' will be a
     // `size_t' a 32-bit integer while the `st_size' field of the stat structure will be off64_t.
@@ -1171,9 +1112,6 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char
         {
             if (errno != EINVAL && errno != ENOSYS)
             {
-                tmpErrno = errno;
-                close(outFd);
-                errno = tmpErrno;
                 return -1;
             }
             else
@@ -1199,37 +1137,47 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char
     // Manually read all data from the source and write it to the destination.
     if (!copied && CopyFile_ReadWrite(inFd, outFd) != 0)
     {
-        tmpErrno = errno;
-        close(outFd);
-        errno = tmpErrno;
         return -1;
     }
 
     // Now that the data from the file has been copied, copy over metadata
     // from the source file.  First copy the file times.
     // If futimes nor futimes are available on this platform, file times will
     // not be copied over.
+    while ((ret = fstat_(inFd, &sourceStat)) < 0 && errno == EINTR);
+    if (ret == 0)
+    {
 #if HAVE_FUTIMENS
-    // futimens is prefered because it has a higher resolution.
-    struct timespec origTimes[2];
-    origTimes[0].tv_sec = (time_t)sourceStat.st_atime;
-    origTimes[0].tv_nsec = ST_ATIME_NSEC(&sourceStat);
-    origTimes[1].tv_sec = (time_t)sourceStat.st_mtime;
-    origTimes[1].tv_nsec = ST_MTIME_NSEC(&sourceStat);
-    while ((ret = futimens(outFd, origTimes)) < 0 && errno == EINTR);
+        // futimens is prefered because it has a higher resolution.
+        struct timespec origTimes[2];
+        origTimes[0].tv_sec = (time_t)sourceStat.st_atime;
+        origTimes[0].tv_nsec = ST_ATIME_NSEC(&sourceStat);
+        origTimes[1].tv_sec = (time_t)sourceStat.st_mtime;
+        origTimes[1].tv_nsec = ST_MTIME_NSEC(&sourceStat);
+        while ((ret = futimens(outFd, origTimes)) < 0 && errno == EINTR);
 #elif HAVE_FUTIMES
-    struct timeval origTimes[2];
-    origTimes[0].tv_sec = sourceStat.st_atime;
-    origTimes[0].tv_usec = (int32_t)(ST_ATIME_NSEC(&sourceStat) / 1000);
-    origTimes[1].tv_sec = sourceStat.st_mtime;
-    origTimes[1].tv_usec = (int32_t)(ST_MTIME_NSEC(&sourceStat) / 1000);
-    while ((ret = futimes(outFd, origTimes)) < 0 && errno == EINTR);
+        struct timeval origTimes[2];
+        origTimes[0].tv_sec = sourceStat.st_atime;
+        origTimes[0].tv_usec = (int32_t)(ST_ATIME_NSEC(&sourceStat) / 1000);
+        origTimes[1].tv_sec = sourceStat.st_mtime;
+        origTimes[1].tv_usec = (int32_t)(ST_MTIME_NSEC(&sourceStat) / 1000);
+        while ((ret = futimes(outFd, origTimes)) < 0 && errno == EINTR);
 #endif
+    }
+    if (ret != 0)
+    {
+        return -1;
+    }
+
+    // Then copy permissions.
+    while ((ret = fchmod(outFd, sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))) < 0 && errno == EINTR);
+    if (ret != 0)
+    {
+        return -1;
+    }
 
-    tmpErrno = errno;
-    close(outFd);
-    errno = tmpErrno;
     return 0;
+#endif // HAVE_FCOPYFILE
 }
 
 intptr_t SystemNative_INotifyInit(void)

src/libraries/Native/Unix/System.Native/pal_io.h

@@ -650,11 +650,11 @@ PALEXPORT void SystemNative_Sync(void);
 PALEXPORT int32_t SystemNative_Write(intptr_t fd, const void* buffer, int32_t bufferSize);
 
 /**
- * Copies all data from the source file descriptor/path to the destination file path.
+ * Copies all data from the source file descriptor to the destination file descriptor.
  *
  * Returns 0 on success; otherwise, returns -1 and sets errno.
  */
-PALEXPORT int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char* destPath, int32_t overwrite);
+PALEXPORT int32_t SystemNative_CopyFile(intptr_t sourceFd, intptr_t destinationFd);
 
 /**
 * Initializes a new inotify instance and returns a file

src/libraries/Native/Unix/configure.cmake

@@ -406,9 +406,9 @@ check_c_source_compiles(
     HAVE_SENDFILE_7)
 
 check_symbol_exists(
-    clonefile
-    "sys/clonefile.h"
-    HAVE_CLONEFILE)
+    fcopyfile
+    copyfile.h
+    HAVE_FCOPYFILE)
 
 check_include_files(
      "sys/sockio.h"

src/libraries/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs

@@ -21,29 +21,9 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove
 
             // Copy the contents of the file from the source to the destination, creating the destination in the process
             using (var src = new FileStream(sourceFullPath, FileMode.Open, FileAccess.Read, FileShare.Read, DefaultBufferSize, FileOptions.None))
+            using (var dst = new FileStream(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, DefaultBufferSize, FileOptions.None))
             {
-                int result = Interop.Sys.CopyFile(src.SafeFileHandle, sourceFullPath, destFullPath, overwrite ? 1 : 0);
-
-                if (result < 0)
-                {
-                    Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo();
-
-                    // If we fail to open the file due to a path not existing, we need to know whether to blame
-                    // the file itself or its directory.  If we're creating the file, then we blame the directory,
-                    // otherwise we blame the file.
-                    //
-                    // When opening, we need to align with Windows, which considers a missing path to be
-                    // FileNotFound only if the containing directory exists.
-
-                    bool isDirectory = (error.Error == Interop.Error.ENOENT) &&
-                        (overwrite || !DirectoryExists(Path.GetDirectoryName(Path.TrimEndingDirectorySeparator(destFullPath.AsSpan()))!));
-
-                    Interop.CheckIo(
-                        error.Error,
-                        destFullPath,
-                        isDirectory,
-                        errorRewriter: e => (e.Error == Interop.Error.EISDIR) ? Interop.Error.EACCES.Info() : e);
-                }
+                Interop.CheckIo(Interop.Sys.CopyFile(src.SafeFileHandle, dst.SafeFileHandle));
             }
         }
 

src/libraries/System.IO.FileSystem/tests/File/Copy.cs

@@ -115,6 +115,7 @@ public static IEnumerable<object[]> CopyFileWithData_MemberData()
 
         [Theory]
         [MemberData(nameof(CopyFileWithData_MemberData))]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/40867", TestPlatforms.Browser)]
         public void CopyFileWithData(char[] data, bool readOnly)
         {
             string testFileSource = GetTestFilePath();
@@ -360,4 +361,25 @@ public void WindowsAlternateDataStreamOverwrite(string defaultStream, string alt
             Assert.Throws<IOException>(() => Copy(testFileAlternateStream, testFile2 + alternateStream, overwrite: true));
         }
     }
+
+    /// <summary>
+    /// Single tests that shouldn't be duplicated by inheritance.
+    /// </summary>
+    [ActiveIssue("https://github.com/dotnet/runtime/issues/40867", TestPlatforms.Browser)]
+    public sealed class File_Copy_Single : FileSystemTest
+    {
+        [Fact]
+        public void EnsureThrowWhenCopyToNonSharedFile()
+        {
+            DirectoryInfo testDirectory = Directory.CreateDirectory(GetTestFilePath());
+            string file1 = Path.Combine(testDirectory.FullName, GetTestFileName());
+            string file2 = Path.Combine(testDirectory.FullName, GetTestFileName());
+
+            File.WriteAllText(file1, "foo");
+            File.WriteAllText(file2, "bar");
+
+            using var stream = new FileStream(file1, FileMode.Open, FileAccess.Read, FileShare.None);
+            Assert.Throws<IOException>(() => File.Copy(file2, file1, overwrite: true));
+        }
+    }
 }