From c14fc5cc6958a1aa723e37a92f63f5807da3e400 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 8 May 2019 17:42:42 +0200 Subject: [PATCH 01/14] Rework SystemNative_CopyFile to use paths instead of file descriptors Use clonefile for CopyFile, if available --- .../Unix/System.Native/Interop.CopyFile.cs | 2 +- src/Native/Unix/Common/pal_config.h.in | 1 + src/Native/Unix/System.Native/pal_io.c | 121 ++++++++++++------ src/Native/Unix/System.Native/pal_io.h | 4 +- src/Native/Unix/configure.cmake | 4 + .../src/System/IO/FileSystem.Unix.cs | 24 +++- 6 files changed, 111 insertions(+), 45 deletions(-) diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs b/src/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs index f7035fceadd3..fb97106635b4 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs @@ -11,6 +11,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, SafeFileHandle destination); + internal static extern int CopyFile(SafeFileHandle source, string srcPath, string destPath, int overwrite); } } diff --git a/src/Native/Unix/Common/pal_config.h.in b/src/Native/Unix/Common/pal_config.h.in index 23cbd90f93df..cfac16a9fd25 100644 --- a/src/Native/Unix/Common/pal_config.h.in +++ b/src/Native/Unix/Common/pal_config.h.in @@ -50,6 +50,7 @@ #cmakedefine01 HAVE_SENDFILE_4 #cmakedefine01 HAVE_SENDFILE_6 #cmakedefine01 HAVE_FCOPYFILE +#cmakedefine01 HAVE_CLONEFILE #cmakedefine01 HAVE_GETNAMEINFO_SIGNED_FLAGS #cmakedefine01 HAVE_GETPEEREID #cmakedefine01 HAVE_SUPPORT_FOR_DUAL_MODE_IPV4_PACKET_INFO diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index 0032e3f5461c..eab8d365fad5 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -28,6 +28,10 @@ #include #include #include +#if HAVE_CLONEFILE +#include +#include +#endif #if HAVE_FCOPYFILE #include #elif HAVE_SENDFILE_4 @@ -1188,30 +1192,80 @@ static int32_t CopyFile_ReadWrite(int inFd, int outFd) } #endif // !HAVE_FCOPYFILE -int32_t SystemNative_CopyFile(intptr_t sourceFd, intptr_t destinationFd) +int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char* destPath, int32_t overwrite) { int inFd = ToFileDescriptor(sourceFd); - int outFd = ToFileDescriptor(destinationFd); + int outFd; + int ret; + struct stat_ sourceStat; + + 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 (sourceStat.st_dev == destStat.st_dev && sourceStat.st_ino == destStat.st_ino) + { + errno = EBUSY; + return -1; + } + + if (!overwrite) + { + errno = EEXIST; + 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) + { + ret = unlink(destPath); + if (ret != 0) + { + return ret; + } + } +#endif + } + +#if HAVE_CLONEFILE + while ((ret = clonefile(srcPath, destPath, 0)) < 0 && errno == EINTR); + if (ret == 0 || (errno != ENOTSUP && errno != EXDEV)) + { + return ret; + } +#else + // Unused variable + (void)srcPath; +#endif + + while ((outFd = open(destPath, O_WRONLY | O_TRUNC | O_CREAT | (overwrite ? 0 : O_EXCL), sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))) < 0 && errno == EINTR); + if (outFd < 0) + { + return -1; + } #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; + ret = fcopyfile(inFd, outFd, NULL, COPYFILE_ALL) == 0 ? 0 : -1; + close(outFd); + return ret; #else // Get the stats on the source file. - int ret; - 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; - } - +#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. @@ -1227,6 +1281,7 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, intptr_t destinationFd) { if (errno != EINVAL && errno != ENOSYS) { + close(outFd); return -1; } else @@ -1252,6 +1307,7 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, intptr_t destinationFd) // Manually read all data from the source and write it to the destination. if (!copied && CopyFile_ReadWrite(inFd, outFd) != 0) { + close(outFd); return -1; } @@ -1259,37 +1315,22 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, intptr_t destinationFd) // 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 = ST_ATIME_NSEC(&sourceStat) / 1000; - origTimes[1].tv_sec = sourceStat.st_mtime; - origTimes[1].tv_usec = 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 = ST_ATIME_NSEC(&sourceStat) / 1000; + origTimes[1].tv_sec = sourceStat.st_mtime; + origTimes[1].tv_usec = 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; - } return 0; #endif // HAVE_FCOPYFILE diff --git a/src/Native/Unix/System.Native/pal_io.h b/src/Native/Unix/System.Native/pal_io.h index 805c6d058d05..147e64e89458 100644 --- a/src/Native/Unix/System.Native/pal_io.h +++ b/src/Native/Unix/System.Native/pal_io.h @@ -667,11 +667,11 @@ DLLEXPORT void SystemNative_Sync(void); DLLEXPORT int32_t SystemNative_Write(intptr_t fd, const void* buffer, int32_t bufferSize); /** - * Copies all data from the source file descriptor to the destination file descriptor. + * Copies all data from the source file descriptor/path to the destination file path. * * Returns 0 on success; otherwise, returns -1 and sets errno. */ -DLLEXPORT int32_t SystemNative_CopyFile(intptr_t sourceFd, intptr_t destinationFd); +DLLEXPORT int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char* destPath, int32_t overwrite); /** * Initializes a new inotify instance and returns a file diff --git a/src/Native/Unix/configure.cmake b/src/Native/Unix/configure.cmake index 913f1f98eca9..1cbaaae3aa91 100644 --- a/src/Native/Unix/configure.cmake +++ b/src/Native/Unix/configure.cmake @@ -349,6 +349,10 @@ check_symbol_exists( copyfile.h HAVE_FCOPYFILE) +check_include_files( + "sys/clonefile.h" + HAVE_CLONEFILE) + check_include_files( "sys/sockio.h" HAVE_SYS_SOCKIO_H) diff --git a/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs b/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs index 03f8019ba96c..1340cbd760ec 100644 --- a/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs +++ b/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs @@ -22,9 +22,29 @@ 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)) { - Interop.CheckIo(Interop.Sys.CopyFile(src.SafeFileHandle, dst.SafeFileHandle)); + 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(PathInternal.TrimEndingDirectorySeparator(destFullPath!))!)); + + Interop.CheckIo( + error.Error, + destFullPath, + isDirectory, + errorRewriter: e => (e.Error == Interop.Error.EISDIR) ? Interop.Error.EACCES.Info() : e); + } } } From b72e115dc56b9cefbf54a11c35b73fd4f735e050 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Mon, 13 May 2019 13:13:46 +0200 Subject: [PATCH 02/14] Bail out in SystemNative_CopyFile if access fails. We don't want to proceed if the destination file is read-only. --- src/Native/Unix/System.Native/pal_io.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index eab8d365fad5..630248eb84f9 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1224,13 +1224,16 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char #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) + if (access(destPath, W_OK) != 0) { - ret = unlink(destPath); - if (ret != 0) - { - return ret; - } + errno = EACCES; + return -1; + } + + ret = unlink(destPath); + if (ret != 0) + { + return ret; } #endif } From e298f7e9b146877fa67185beac1e226e5dd7396f Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Mon, 13 May 2019 13:24:42 +0200 Subject: [PATCH 03/14] Handle a race condition where clonefile can result in EEXIST error --- src/Native/Unix/System.Native/pal_io.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index 630248eb84f9..c9b306cd6888 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1240,7 +1240,11 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char #if HAVE_CLONEFILE while ((ret = clonefile(srcPath, destPath, 0)) < 0 && errno == EINTR); - if (ret == 0 || (errno != ENOTSUP && errno != EXDEV)) + // 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; } From 521c4eed2ddc4f1a241a7154dcac3f5ffdb5507c Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Mon, 13 May 2019 13:30:04 +0200 Subject: [PATCH 04/14] Update comment about fcopyfile --- src/Native/Unix/System.Native/pal_io.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index c9b306cd6888..67a0b8bc2946 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1260,9 +1260,10 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char } #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. + // If fcopyfile is available (macOS), try to use it, as it handles + // copying both data and metadata. Unlike sendfile below it's + // implemented as user space library but future version of macOS + // may optimize it. ret = fcopyfile(inFd, outFd, NULL, COPYFILE_ALL) == 0 ? 0 : -1; close(outFd); return ret; From fcd8d40ba3f073a805d41460cd89e518aa49dbb1 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 31 Oct 2019 17:05:51 +0100 Subject: [PATCH 05/14] Update PathInternal.TrimEndingDirectorySeparator to Path.TrimEndingDirectorySeparator --- src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs b/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs index 1340cbd760ec..d6692005b24e 100644 --- a/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs +++ b/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs @@ -37,7 +37,7 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove // FileNotFound only if the containing directory exists. bool isDirectory = (error.Error == Interop.Error.ENOENT) && - (overwrite || !DirectoryExists(Path.GetDirectoryName(PathInternal.TrimEndingDirectorySeparator(destFullPath!))!)); + (overwrite || !DirectoryExists(Path.GetDirectoryName(Path.TrimEndingDirectorySeparator(destFullPath!))!)); Interop.CheckIo( error.Error, From 4147151d3fe9937235e882ae6fca5a9028f8f479 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 31 Oct 2019 19:44:58 +0100 Subject: [PATCH 06/14] Add missing close(outFd) call --- src/Native/Unix/System.Native/pal_io.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index 67a0b8bc2946..60837db8bb67 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1340,6 +1340,7 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char while ((ret = futimes(outFd, origTimes)) < 0 && errno == EINTR); #endif + close(outFd); return 0; #endif // HAVE_FCOPYFILE } From 56dd29ff29aade170a6dafcf1f613c6232a29b04 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 1 Nov 2019 15:02:57 +0100 Subject: [PATCH 07/14] Remove unnecessary ! operator --- src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs b/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs index d6692005b24e..75f8f89eaa3e 100644 --- a/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs +++ b/src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs @@ -37,7 +37,7 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove // FileNotFound only if the containing directory exists. bool isDirectory = (error.Error == Interop.Error.ENOENT) && - (overwrite || !DirectoryExists(Path.GetDirectoryName(Path.TrimEndingDirectorySeparator(destFullPath!))!)); + (overwrite || !DirectoryExists(Path.GetDirectoryName(Path.TrimEndingDirectorySeparator(destFullPath))!)); Interop.CheckIo( error.Error, From 88e6fb0f7b5a5eabaa00791700c076d76dd9d34a Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 1 Nov 2019 15:03:30 +0100 Subject: [PATCH 08/14] Save errno around close(outFd) calls --- src/Native/Unix/System.Native/pal_io.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index 60837db8bb67..6720b4791c99 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1197,6 +1197,7 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char int inFd = ToFileDescriptor(sourceFd); int outFd; int ret; + int tmpErrno; struct stat_ sourceStat; while ((ret = fstat_(inFd, &sourceStat)) < 0 && errno == EINTR); @@ -1265,7 +1266,9 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char // implemented as user space library but future version of macOS // may optimize it. ret = fcopyfile(inFd, outFd, NULL, COPYFILE_ALL) == 0 ? 0 : -1; + tmpErrno = errno; close(outFd); + errno = tmpErrno; return ret; #else // Get the stats on the source file. @@ -1289,7 +1292,9 @@ 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 @@ -1315,7 +1320,9 @@ 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; } @@ -1340,7 +1347,9 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char while ((ret = futimes(outFd, origTimes)) < 0 && errno == EINTR); #endif + tmpErrno = errno; close(outFd); + errno = tmpErrno; return 0; #endif // HAVE_FCOPYFILE } From 327d3b633020f20703df9fc9299da57f9616dcbf Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 1 Nov 2019 15:04:32 +0100 Subject: [PATCH 09/14] access already sets EACCESS errno, just keep it --- src/Native/Unix/System.Native/pal_io.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index 6720b4791c99..63131338d369 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1227,7 +1227,6 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char // check permission first to ensure we don't try to unlink read-only file if (access(destPath, W_OK) != 0) { - errno = EACCES; return -1; } From dcee2e9f4f142a07c9de9e996147ec5ae6421a2e Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 1 Nov 2019 15:06:04 +0100 Subject: [PATCH 10/14] Remove fcopyfile support --- src/Native/Unix/System.Native/pal_io.c | 18 +----------------- src/Native/Unix/configure.cmake | 5 ----- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index 63131338d369..a4044196d01b 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -32,9 +32,7 @@ #include #include #endif -#if HAVE_FCOPYFILE -#include -#elif HAVE_SENDFILE_4 +#if HAVE_SENDFILE_4 #include #endif #if HAVE_INOTIFY @@ -1137,7 +1135,6 @@ int32_t SystemNative_Write(intptr_t fd, const void* buffer, int32_t bufferSize) return (int32_t)count; } -#if !HAVE_FCOPYFILE // Read all data from inFd and write it to outFd static int32_t CopyFile_ReadWrite(int inFd, int outFd) { @@ -1190,7 +1187,6 @@ 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) { @@ -1259,17 +1255,6 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char return -1; } -#if HAVE_FCOPYFILE - // If fcopyfile is available (macOS), try to use it, as it handles - // copying both data and metadata. Unlike sendfile below it's - // implemented as user space library but future version of macOS - // may optimize it. - ret = fcopyfile(inFd, outFd, NULL, COPYFILE_ALL) == 0 ? 0 : -1; - tmpErrno = errno; - close(outFd); - errno = tmpErrno; - return ret; -#else // Get the stats on the source file. bool copied = false; @@ -1350,7 +1335,6 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char close(outFd); errno = tmpErrno; return 0; -#endif // HAVE_FCOPYFILE } intptr_t SystemNative_INotifyInit(void) diff --git a/src/Native/Unix/configure.cmake b/src/Native/Unix/configure.cmake index 1cbaaae3aa91..6c4e7c2b38a8 100644 --- a/src/Native/Unix/configure.cmake +++ b/src/Native/Unix/configure.cmake @@ -344,11 +344,6 @@ check_c_source_compiles( " HAVE_SENDFILE_6) -check_symbol_exists( - fcopyfile - copyfile.h - HAVE_FCOPYFILE) - check_include_files( "sys/clonefile.h" HAVE_CLONEFILE) From 24582a23de53d2e06c97ea6bff18a60cd98c0833 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 1 Nov 2019 15:11:36 +0100 Subject: [PATCH 11/14] Open destination file with O_CLOEXEC --- src/Native/Unix/System.Native/pal_io.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index a4044196d01b..7b5fb0820d03 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1194,6 +1194,7 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char int outFd; int ret; int tmpErrno; + int openFlags; struct stat_ sourceStat; while ((ret = fstat_(inFd, &sourceStat)) < 0 && errno == EINTR); @@ -1249,11 +1250,18 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char (void)srcPath; #endif - while ((outFd = open(destPath, O_WRONLY | O_TRUNC | O_CREAT | (overwrite ? 0 : O_EXCL), sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))) < 0 && errno == EINTR); + 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; From 22d0ee755e75b6609563b2678aa21434bf62f929 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 1 Nov 2019 15:34:39 +0100 Subject: [PATCH 12/14] Return correct error code for source == destination --- src/Native/Unix/System.Native/pal_io.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index 7b5fb0820d03..e0d4c2a309a8 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1209,7 +1209,9 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char { if (sourceStat.st_dev == destStat.st_dev && sourceStat.st_ino == destStat.st_ino) { - errno = EBUSY; + // Attempt to copy file over itself. Bail out early with the appropriate + // error code. + errno = overwrite ? EBUSY : EEXIST; return -1; } From 8deeeab864dd6c53cefc2e9f74f1224ba36dac20 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 1 Nov 2019 15:36:38 +0100 Subject: [PATCH 13/14] Rearrange error handling a bit --- src/Native/Unix/System.Native/pal_io.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_io.c b/src/Native/Unix/System.Native/pal_io.c index e0d4c2a309a8..dd491e5cf7e9 100644 --- a/src/Native/Unix/System.Native/pal_io.c +++ b/src/Native/Unix/System.Native/pal_io.c @@ -1207,23 +1207,23 @@ int32_t SystemNative_CopyFile(intptr_t sourceFd, const char* srcPath, const char while ((ret = stat_(destPath, &destStat)) < 0 && errno == EINTR); if (ret == 0) { - if (sourceStat.st_dev == destStat.st_dev && sourceStat.st_ino == destStat.st_ino) + if (!overwrite) { - // Attempt to copy file over itself. Bail out early with the appropriate - // error code. - errno = overwrite ? EBUSY : EEXIST; + errno = EEXIST; return -1; } - if (!overwrite) + if (sourceStat.st_dev == destStat.st_dev && sourceStat.st_ino == destStat.st_ino) { - errno = EEXIST; + // 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 + // check permission first to ensure we don't try to unlink read-only file. if (access(destPath, W_OK) != 0) { return -1; From 2cf36d4e90ed6bd3c94060d219d744a67f6f5057 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 1 Nov 2019 15:39:07 +0100 Subject: [PATCH 14/14] Remove forgotten HAVE_FCOPYFILE --- src/Native/Unix/Common/pal_config.h.in | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Native/Unix/Common/pal_config.h.in b/src/Native/Unix/Common/pal_config.h.in index cfac16a9fd25..66dc20648e93 100644 --- a/src/Native/Unix/Common/pal_config.h.in +++ b/src/Native/Unix/Common/pal_config.h.in @@ -49,7 +49,6 @@ #cmakedefine01 HAVE_KQUEUE #cmakedefine01 HAVE_SENDFILE_4 #cmakedefine01 HAVE_SENDFILE_6 -#cmakedefine01 HAVE_FCOPYFILE #cmakedefine01 HAVE_CLONEFILE #cmakedefine01 HAVE_GETNAMEINFO_SIGNED_FLAGS #cmakedefine01 HAVE_GETPEEREID