From a373c597acc30a8ab6b67e9954030e2b36320861 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 06:49:38 +1100 Subject: [PATCH 01/81] Initial implementation of fix for #77835 - cloning files on macOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Use copyfile (with COPYFILE_CLONE_FORCE) when possible on macOS to clone the file while still keeping file locking logic intact • Split common Unix logic into multiple functions that the macOS implementation uses parts of at different times • Add string version of ResolveLinkTarget to save the allocation since part of the code needs it • Need to add tests to check the file is actually cloned so we know if it works or not --- .../Common/src/Interop/OSX/Interop.libc.cs | 4 + .../System.Private.CoreLib.Shared.projitems | 2 + .../src/System/IO/FileSystem.CopyFile.OSX.cs | 141 ++++++++++++++++++ .../IO/FileSystem.CopyFile.OtherUnix.cs | 17 +++ .../src/System/IO/FileSystem.Unix.cs | 98 ++++++++++-- 5 files changed, 251 insertions(+), 11 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs diff --git a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs index 44717c56d0ba8..3777391f429b1 100644 --- a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs +++ b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs @@ -44,5 +44,9 @@ internal static unsafe int fsetattrlist(SafeHandle handle, AttrList* attrList, v handle.DangerousRelease(); } } + + [LibraryImport(Libraries.libc, EntryPoint = "copyfile", SetLastError = true)] + internal static unsafe partial int copyfile(string from, string to, void* state, uint flags); + internal const uint COPYFILE_CLONE_FORCE = 0x02000000; } } diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 46747d7885f80..469888be183ea 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -2300,6 +2300,7 @@ + @@ -2353,6 +2354,7 @@ Common\Interop\OSX\Interop.libc.cs + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs new file mode 100644 index 0000000000000..66786655cc7e2 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -0,0 +1,141 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.InteropServices; + +namespace System.IO +{ + partial class FileSystem + { + public static unsafe void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + { + //Attempt to clone the file: + + //Get the full path of the source path + string fullSource = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath; + + //Start the file copy and prepare for finalization + StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false); + + //Attempt counter just in case we somehow loop infinite times e.g. on a + //filesystem that doesn't actually delete files but pretends it does. + //Declare error variable here since it can be used after some jumping around. + int attempts = 0; + int error; + + try + { + //Don't need to re-read the link on our first attempt + bool failOnRereadDoesntChange = false; + if (overwrite) + { + //Ensure file is deleted on first try. + //Get a lock to the dest file for compat reasons, and then delete it. + using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); + File.Delete(destFullPath); + } + goto tryAgain; + + //We may want to re-read the link to see if its path has changed + tryAgainWithReadLink: + if (++attempts >= 5) goto throwError; + string fullSource2 = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath; + if (fullSource != fullSource2) + { + //Path has changed + startedCopyFile.Dispose(); + startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false); + } + else if (failOnRereadDoesntChange) + { + //Path hasn't changed and we want to throw the error we got earlier + goto throwError; + } + failOnRereadDoesntChange = false; + + //Attempt to clone the file + tryAgain: + if (Interop.@libc.copyfile(fullSource, destFullPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0) + { + return; + } + + //Check the error + error = Marshal.GetLastWin32Error(); + const int ENOTSUP = 45; + const int EEXIST = 17; + const int ENOENT = 2; + bool directoryExist = false; + if ((error == ENOTSUP && FileOrDirectoryExists(destFullPath)) || error == EEXIST) + { + //This means the destination existed, try again with the destination deleted if appropriate + error = EEXIST; + if (Directory.Exists(destFullPath)) + { + directoryExist = true; + goto throwError; + } + if (overwrite) + { + //Get a lock to the dest file for compat reasons, and then delete it. + using SafeFileHandle dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile); + File.Delete(destFullPath); + goto tryAgainWithReadLink; + } + } + else if (error == ENOTSUP) + { + //This probably means cloning is not supported, try the standard implementation + goto fallback; + } + else if (error == ENOENT) + { + //This can happen if the source is a symlink and it has been changed to a different file, and the first has been deleted or renamed, for example. + //failOnRereadDoesntChange means we want to fail if the link didn't change, indicating the source actually doesn't exist. + failOnRereadDoesntChange = true; + goto tryAgainWithReadLink; + } + + //Throw an appropriate error + throwError: + if (directoryExist) + { + throw new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); + } + throw Interop.GetExceptionForIoErrno(new ErrorInfo(error)); + + //Fallback to the standard unix implementation for when cloning is not supported + fallback: + + //Open the dst handle + startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); + + //Copy the file using the standard unix implementation + StandardCopyFile(startedCopyFile); + } + finally + { + startedCopyFile.Dispose(); + } + + //Attempts to read the path's link target, or returns null even if the path doesn't exist + static string? TryGetLinkTarget(string path) + { + try + { + return ResolveLinkTargetString(sourceFullPath, true, false); + } + catch + { + return null; + } + } + + //Checks if a file or directory exists without caring which it was + static bool FileOrDirectoryExists(string path) + { + return Interop.Sys.Stat(fullPath, out _) >= 0; + } + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs new file mode 100644 index 0000000000000..baeed275b03bb --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -0,0 +1,17 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace System.IO +{ + partial class FileSystem + { + public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + { + //Start the file copy and prepare for finalization + using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); + + //Copy the file using the standard unix implementation + StandardCopyFile(startCopyFile); + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index f28cd70f08f83..88b5d9c685366 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,16 +28,72 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + //Helper type to facilitate returning values from StartCopyFile without having + //to declare a massive tuple multiple times, and making it easier to dispose. + private struct StartedCopyFileState : IDisposable { - long fileLength; - UnixFileMode filePermissions; - using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out fileLength, out filePermissions); - using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, - CreateOpenException); + public long fileLength; + public UnixFileMode filePermissions; + public SafeFileHandle? src; + public SafeFileHandle? dst; - Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileLength)); + public StartedCopyFileState(long fileLength, UnixFileMode filePermissions, SafeFileHandle src, SafeFileHandle dst) + { + this.fileLength = fileLength; + this.filePermissions = filePermissions; + this.src = src; + this.dst = dst; + } + + public void Dispose() + { + src?.Dispose(); + dst?.Dispose(); + } + } + + private static StartedCopyFileState StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite) + { + //The return value is expected to be Disposed by the caller (unless this method throws) once the copy is complete. + //Begins 'CopyFile' by locking and creating the relevant file handles. + + StartedCopyFileState startedCopyFile = default; + try + { + startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out startedCopyFile.fileLength, out startedCopyFile.filePermissions); + startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); + } + catch + { + startedCopyFile.Dispose(); + throw; + } + + //Return the collection of information we have gotten back. + return startedCopyFile; + } + + private static SafeFileHandle? OpenCopyFileDstHandle(string destFullPath, bool overwrite, StartedCopyFileState startedCopyFile, bool openNewFile) + { + //This function opens the 'dst' file handle for 'CopyFile', it is + //split out since the logic on OSX-like OSes is a bit different. + //'openNewFile' = false is used when we want to try to find the file only. + if (!openNewFile) + { + try + { + return SafeFileHandle.Open(destFullPath, FileMode.Open, + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, startedCopyFile.filePermissions, + CreateOpenException); + } + catch (FileNotFoundException) + { + return null; + } + } + return SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, + CreateOpenException); static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) { @@ -51,6 +107,18 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove } } + private static void StandardCopyFile(StartedCopyFileState startedCopyFile) + { + //Copy the file in a way that works on all Unix Operating Systems. + //The 'startedCopyFile' parameter should take the output from 'StartCopyFile'. + //'startedCopyFile' should be disposed by the caller. + Interop.CheckIo(Interop.Sys.CopyFile(startedCopyFile.src, startedCopyFile.dst, startedCopyFile.fileLength)); + } + + //CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs + //The implementations on OSX-like Operating Systems attempts to clone the file first. + static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); + #pragma warning disable IDE0060 public static void Encrypt(string path) { @@ -659,6 +727,16 @@ internal static void CreateSymbolicLink(string path, string pathToTarget, bool i #pragma warning restore IDE0060 internal static FileSystemInfo? ResolveLinkTarget(string linkPath, bool returnFinalTarget, bool isDirectory) + { + string? linkTarget = ResolveLinkTargetString(linkPath, returnFinalTarget, isDirectory); + if (linkTarget == null) return null; + + return isDirectory ? + new DirectoryInfo(linkTarget) : + new FileInfo(linkTarget); + } + + private static string? ResolveLinkTargetString(string linkPath, bool returnFinalTarget, bool isDirectory) { ValueStringBuilder sb = new(Interop.DefaultPathBufferSize); sb.Append(linkPath); @@ -704,9 +782,7 @@ internal static void CreateSymbolicLink(string path, string pathToTarget, bool i Debug.Assert(sb.Length > 0); linkTarget = sb.ToString(); // ToString disposes - return isDirectory ? - new DirectoryInfo(linkTarget) : - new FileInfo(linkTarget); + return linkTarget; // In case of link target being relative: // Preserve the full path of the directory of the previous path From f21a5ae1f4d6a650505ead16d38316faf5ceb888 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 07:15:03 +1100 Subject: [PATCH 02/81] Fix some build issues from previous commit for #77835 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • I missed setting StringMarshalling on the LibraryImport • I missed partial on the CopyFile method implementations --- src/libraries/Common/src/Interop/OSX/Interop.libc.cs | 2 +- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 +- .../src/System/IO/FileSystem.CopyFile.OtherUnix.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs index 3777391f429b1..b03b41e3fbf3a 100644 --- a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs +++ b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs @@ -45,7 +45,7 @@ internal static unsafe int fsetattrlist(SafeHandle handle, AttrList* attrList, v } } - [LibraryImport(Libraries.libc, EntryPoint = "copyfile", SetLastError = true)] + [LibraryImport(Libraries.libc, EntryPoint = "copyfile", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)] internal static unsafe partial int copyfile(string from, string to, void* state, uint flags); internal const uint COPYFILE_CLONE_FORCE = 0x02000000; } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 66786655cc7e2..faf751d760947 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -7,7 +7,7 @@ namespace System.IO { partial class FileSystem { - public static unsafe void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + public static unsafe partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { //Attempt to clone the file: diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index baeed275b03bb..ab5efe33756e8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -5,7 +5,7 @@ namespace System.IO { partial class FileSystem { - public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { //Start the file copy and prepare for finalization using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); From 189011560862aac0bd73f2d111929e0590dc4508 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 07:30:19 +1100 Subject: [PATCH 03/81] Fix some build issues from previous commit for #77835 again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Apparently 'Both partial method declarations must be unsafe or neither may be unsafe' --- .../src/System/IO/FileSystem.CopyFile.OtherUnix.cs | 2 +- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index ab5efe33756e8..a3c4f98a19411 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -5,7 +5,7 @@ namespace System.IO { partial class FileSystem { - public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + public static unsafe partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { //Start the file copy and prepare for finalization using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 88b5d9c685366..798f8a802788a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -117,7 +117,7 @@ private static void StandardCopyFile(StartedCopyFileState startedCopyFile) //CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs //The implementations on OSX-like Operating Systems attempts to clone the file first. - static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); + static unsafe partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); #pragma warning disable IDE0060 public static void Encrypt(string path) From e490d5ecba1c0653e6eda2e2bef3f6ff158d7545 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 07:51:41 +1100 Subject: [PATCH 04/81] Fix some build issues from previous commit for #77835 again (2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • I love partial methods • Move unsafe keyword into the OSX method rather than in method declaration • Fix accessibility modifier of CopyFile in FileSystem.Unix.cs --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 9 ++++++--- .../src/System/IO/FileSystem.CopyFile.OtherUnix.cs | 2 +- .../src/System/IO/FileSystem.Unix.cs | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index faf751d760947..9106aaeb81db7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -7,7 +7,7 @@ namespace System.IO { partial class FileSystem { - public static unsafe partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { //Attempt to clone the file: @@ -55,9 +55,12 @@ public static unsafe partial void CopyFile(string sourceFullPath, string destFul //Attempt to clone the file tryAgain: - if (Interop.@libc.copyfile(fullSource, destFullPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0) + unsafe { - return; + if (Interop.@libc.copyfile(fullSource, destFullPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0) + { + return; + } } //Check the error diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index a3c4f98a19411..ab5efe33756e8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -5,7 +5,7 @@ namespace System.IO { partial class FileSystem { - public static unsafe partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { //Start the file copy and prepare for finalization using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 798f8a802788a..445465c39e9e1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -117,7 +117,7 @@ private static void StandardCopyFile(StartedCopyFileState startedCopyFile) //CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs //The implementations on OSX-like Operating Systems attempts to clone the file first. - static unsafe partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); + public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); #pragma warning disable IDE0060 public static void Encrypt(string path) From 1e43a919203becd9dc0b222aabfa5069baedfc58 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 08:17:57 +1100 Subject: [PATCH 05/81] Fix some build issues from previous commit for #77835 again (3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Fix indentation (should have used spaces) • Import Microsoft.Win32.SafeHandles in FileSystem.CopyFile.OSX.cs file • Fix for SA1205 'Partial elements should declare an access modifier' • Fix typo in FileSystem.CopyFile.OtherUnix.cs of 'startedCopyFile' • Fix missing openDst parameter code in StartCopyFile • Fix not checking the nullability in StandardCopyFile, which led to 'Possible null reference argument for parameter' --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 241 +++++++++--------- .../IO/FileSystem.CopyFile.OtherUnix.cs | 20 +- .../src/System/IO/FileSystem.Unix.cs | 12 +- 3 files changed, 139 insertions(+), 134 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 9106aaeb81db7..8916f6412782c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -2,143 +2,144 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Runtime.InteropServices; +using Microsoft.Win32.SafeHandles; namespace System.IO { - partial class FileSystem - { - public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) - { - //Attempt to clone the file: + internal static partial class FileSystem + { + public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + { + //Attempt to clone the file: - //Get the full path of the source path - string fullSource = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath; + //Get the full path of the source path + string fullSource = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath; - //Start the file copy and prepare for finalization - StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false); + //Start the file copy and prepare for finalization + StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false); - //Attempt counter just in case we somehow loop infinite times e.g. on a - //filesystem that doesn't actually delete files but pretends it does. - //Declare error variable here since it can be used after some jumping around. - int attempts = 0; - int error; + //Attempt counter just in case we somehow loop infinite times e.g. on a + //filesystem that doesn't actually delete files but pretends it does. + //Declare error variable here since it can be used after some jumping around. + int attempts = 0; + int error; - try - { - //Don't need to re-read the link on our first attempt - bool failOnRereadDoesntChange = false; - if (overwrite) - { - //Ensure file is deleted on first try. - //Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); - File.Delete(destFullPath); - } - goto tryAgain; + try + { + //Don't need to re-read the link on our first attempt + bool failOnRereadDoesntChange = false; + if (overwrite) + { + //Ensure file is deleted on first try. + //Get a lock to the dest file for compat reasons, and then delete it. + using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); + File.Delete(destFullPath); + } + goto tryAgain; - //We may want to re-read the link to see if its path has changed - tryAgainWithReadLink: - if (++attempts >= 5) goto throwError; - string fullSource2 = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath; - if (fullSource != fullSource2) - { - //Path has changed - startedCopyFile.Dispose(); - startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false); - } - else if (failOnRereadDoesntChange) - { - //Path hasn't changed and we want to throw the error we got earlier - goto throwError; - } - failOnRereadDoesntChange = false; + //We may want to re-read the link to see if its path has changed + tryAgainWithReadLink: + if (++attempts >= 5) goto throwError; + string fullSource2 = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath; + if (fullSource != fullSource2) + { + //Path has changed + startedCopyFile.Dispose(); + startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false); + } + else if (failOnRereadDoesntChange) + { + //Path hasn't changed and we want to throw the error we got earlier + goto throwError; + } + failOnRereadDoesntChange = false; - //Attempt to clone the file - tryAgain: - unsafe - { - if (Interop.@libc.copyfile(fullSource, destFullPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0) - { - return; - } - } + //Attempt to clone the file + tryAgain: + unsafe + { + if (Interop.@libc.copyfile(fullSource, destFullPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0) + { + return; + } + } - //Check the error - error = Marshal.GetLastWin32Error(); - const int ENOTSUP = 45; - const int EEXIST = 17; - const int ENOENT = 2; - bool directoryExist = false; - if ((error == ENOTSUP && FileOrDirectoryExists(destFullPath)) || error == EEXIST) - { - //This means the destination existed, try again with the destination deleted if appropriate - error = EEXIST; - if (Directory.Exists(destFullPath)) - { - directoryExist = true; - goto throwError; - } - if (overwrite) - { - //Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile); - File.Delete(destFullPath); - goto tryAgainWithReadLink; - } - } - else if (error == ENOTSUP) - { - //This probably means cloning is not supported, try the standard implementation - goto fallback; - } - else if (error == ENOENT) - { - //This can happen if the source is a symlink and it has been changed to a different file, and the first has been deleted or renamed, for example. - //failOnRereadDoesntChange means we want to fail if the link didn't change, indicating the source actually doesn't exist. - failOnRereadDoesntChange = true; - goto tryAgainWithReadLink; - } + //Check the error + error = Marshal.GetLastWin32Error(); + const int ENOTSUP = 45; + const int EEXIST = 17; + const int ENOENT = 2; + bool directoryExist = false; + if ((error == ENOTSUP && FileOrDirectoryExists(destFullPath)) || error == EEXIST) + { + //This means the destination existed, try again with the destination deleted if appropriate + error = EEXIST; + if (Directory.Exists(destFullPath)) + { + directoryExist = true; + goto throwError; + } + if (overwrite) + { + //Get a lock to the dest file for compat reasons, and then delete it. + using SafeFileHandle dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile); + File.Delete(destFullPath); + goto tryAgainWithReadLink; + } + } + else if (error == ENOTSUP) + { + //This probably means cloning is not supported, try the standard implementation + goto fallback; + } + else if (error == ENOENT) + { + //This can happen if the source is a symlink and it has been changed to a different file, and the first has been deleted or renamed, for example. + //failOnRereadDoesntChange means we want to fail if the link didn't change, indicating the source actually doesn't exist. + failOnRereadDoesntChange = true; + goto tryAgainWithReadLink; + } - //Throw an appropriate error - throwError: - if (directoryExist) + //Throw an appropriate error + throwError: + if (directoryExist) { throw new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); } - throw Interop.GetExceptionForIoErrno(new ErrorInfo(error)); + throw Interop.GetExceptionForIoErrno(new ErrorInfo(error)); - //Fallback to the standard unix implementation for when cloning is not supported - fallback: + //Fallback to the standard unix implementation for when cloning is not supported + fallback: - //Open the dst handle - startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); + //Open the dst handle + startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); - //Copy the file using the standard unix implementation - StandardCopyFile(startedCopyFile); - } - finally - { - startedCopyFile.Dispose(); - } + //Copy the file using the standard unix implementation + StandardCopyFile(startedCopyFile); + } + finally + { + startedCopyFile.Dispose(); + } - //Attempts to read the path's link target, or returns null even if the path doesn't exist - static string? TryGetLinkTarget(string path) - { - try - { - return ResolveLinkTargetString(sourceFullPath, true, false); - } - catch - { - return null; - } - } + //Attempts to read the path's link target, or returns null even if the path doesn't exist + static string? TryGetLinkTarget(string path) + { + try + { + return ResolveLinkTargetString(sourceFullPath, true, false); + } + catch + { + return null; + } + } - //Checks if a file or directory exists without caring which it was - static bool FileOrDirectoryExists(string path) - { - return Interop.Sys.Stat(fullPath, out _) >= 0; - } - } - } + //Checks if a file or directory exists without caring which it was + static bool FileOrDirectoryExists(string path) + { + return Interop.Sys.Stat(fullPath, out _) >= 0; + } + } + } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index ab5efe33756e8..b9069b7345684 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -3,15 +3,15 @@ namespace System.IO { - partial class FileSystem - { - public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) - { - //Start the file copy and prepare for finalization - using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); + internal static partial class FileSystem + { + public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + { + //Start the file copy and prepare for finalization + using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); - //Copy the file using the standard unix implementation - StandardCopyFile(startCopyFile); - } - } + //Copy the file using the standard unix implementation + StandardCopyFile(startedCopyFile); + } + } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 445465c39e9e1..8013422d1c96a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -52,16 +52,17 @@ public void Dispose() } } - private static StartedCopyFileState StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite) + private static StartedCopyFileState StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite, bool openDst = true) { //The return value is expected to be Disposed by the caller (unless this method throws) once the copy is complete. //Begins 'CopyFile' by locking and creating the relevant file handles. + //If 'openDst' is false, it doesn't open the destination file handle, nor check anything to do with it (used in macOS implementation). StartedCopyFileState startedCopyFile = default; try { startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out startedCopyFile.fileLength, out startedCopyFile.filePermissions); - startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); + if (openDst) startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); } catch { @@ -111,8 +112,11 @@ private static void StandardCopyFile(StartedCopyFileState startedCopyFile) { //Copy the file in a way that works on all Unix Operating Systems. //The 'startedCopyFile' parameter should take the output from 'StartCopyFile'. - //'startedCopyFile' should be disposed by the caller. - Interop.CheckIo(Interop.Sys.CopyFile(startedCopyFile.src, startedCopyFile.dst, startedCopyFile.fileLength)); + //'startedCopyFile' should be disposed by the caller. Assumes src and dst + //are non-null, caller must check this, return values from StartCopyFile + //are non-null (except dst when openDst is true), and from return values from + //OpenCopyFileDstHandle are non-null (except possibly when openNewFile is false). + Interop.CheckIo(Interop.Sys.CopyFile(startedCopyFile.src!, startedCopyFile.dst!, startedCopyFile.fileLength)); } //CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs From 4c5a995a374fd9d8f7e19aab1d1e57c442fced56 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 08:41:58 +1100 Subject: [PATCH 06/81] Fix some build issues from previous commit for #77835 again (4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Add missing parameter value (openNewFile) to OpenCopyFileDstHandle • Fix misnamed variable usages throughout some code • Properly qualify Interop.ErrorInfo • This should be the last fix for build issues for this set --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 8916f6412782c..ed6608b6c366e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -82,7 +82,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, if (overwrite) { //Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile); + using SafeFileHandle dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); File.Delete(destFullPath); goto tryAgainWithReadLink; } @@ -104,9 +104,9 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, throwError: if (directoryExist) { - throw new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); + throw new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, destFullPath)); } - throw Interop.GetExceptionForIoErrno(new ErrorInfo(error)); + throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(error)); //Fallback to the standard unix implementation for when cloning is not supported fallback: @@ -127,7 +127,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, { try { - return ResolveLinkTargetString(sourceFullPath, true, false); + return ResolveLinkTargetString(path, true, false); } catch { @@ -138,7 +138,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, //Checks if a file or directory exists without caring which it was static bool FileOrDirectoryExists(string path) { - return Interop.Sys.Stat(fullPath, out _) >= 0; + return Interop.Sys.Stat(path, out _) >= 0; } } } From c65e229b9aec24244c8f94d3589d12e45d6fc8eb Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 08:58:47 +1100 Subject: [PATCH 07/81] Fix some build issues from previous commit for #77835 again (5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Add missing nullable ? for dstHandle (which obtains the file lock) --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index ed6608b6c366e..fd7448f2798ce 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -82,7 +82,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, if (overwrite) { //Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); + using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); File.Delete(destFullPath); goto tryAgainWithReadLink; } From 5377efb8502ab76f91abb15a4ec431960b82e417 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 17:00:22 +1100 Subject: [PATCH 08/81] Fix copying a file onto itself logic for #77835 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Test failures were since copying a file onto itself should cause an error, this fixes that --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 86 +++++++++++++++---- 1 file changed, 68 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index fd7448f2798ce..884ac88415141 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -12,11 +12,25 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, { //Attempt to clone the file: - //Get the full path of the source path - string fullSource = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath; + //Simplify the destination path (i.e. unlink all the links except the last one itself, + //i.e. for /link1/link2, you could get /folder1/link2). + string destFullPath_Full = Path.GetFullPath(destFullPath); + string destPath = Path.GetDirectoryName(destFullPath_Full); + destPath = ResolveLinkTargetString(destPath, true, true); + if (string.IsNullOrEmpty(destPath)) + { + destPath = destFullPath_Full; + } + else + { + destPath = Path.Combine(destPath, Path.GetFileName(destFullPath)); + } + + //Get the full path of the source path and verify that we're not copying the source file onto itself + string fullSource = TryGetLinkTarget(sourceFullPath, destPath, overwrite) ?? destPath; //Start the file copy and prepare for finalization - StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false); + StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destPath, overwrite, openDst: false); //Attempt counter just in case we somehow loop infinite times e.g. on a //filesystem that doesn't actually delete files but pretends it does. @@ -32,20 +46,20 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, { //Ensure file is deleted on first try. //Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); - File.Delete(destFullPath); + using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destPath, true, startedCopyFile, false); + File.Delete(destPath); } goto tryAgain; //We may want to re-read the link to see if its path has changed tryAgainWithReadLink: if (++attempts >= 5) goto throwError; - string fullSource2 = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath; + string fullSource2 = TryGetLinkTarget(sourceFullPath, destPath, overwrite) ?? sourceFullPath; if (fullSource != fullSource2) { //Path has changed startedCopyFile.Dispose(); - startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false); + startedCopyFile = StartCopyFile(fullSource, destPath, overwrite, openDst: false); } else if (failOnRereadDoesntChange) { @@ -58,7 +72,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, tryAgain: unsafe { - if (Interop.@libc.copyfile(fullSource, destFullPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0) + if (Interop.@libc.copyfile(fullSource, destPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0) { return; } @@ -70,11 +84,11 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, const int EEXIST = 17; const int ENOENT = 2; bool directoryExist = false; - if ((error == ENOTSUP && FileOrDirectoryExists(destFullPath)) || error == EEXIST) + if ((error == ENOTSUP && FileOrDirectoryExists(destPath)) || error == EEXIST) { //This means the destination existed, try again with the destination deleted if appropriate error = EEXIST; - if (Directory.Exists(destFullPath)) + if (Directory.Exists(destPath)) { directoryExist = true; goto throwError; @@ -82,8 +96,8 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, if (overwrite) { //Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); - File.Delete(destFullPath); + using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destPath, true, startedCopyFile, false); + File.Delete(destPath); goto tryAgainWithReadLink; } } @@ -122,17 +136,53 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, startedCopyFile.Dispose(); } - //Attempts to read the path's link target, or returns null even if the path doesn't exist - static string? TryGetLinkTarget(string path) + //Attempts to read the path's link target, or returns null even if the path doesn't exist rather than throwing. + //Throws an error if 'path' is at any point equal to 'destPath', since it means we're copying onto itself. + //Throws an error if 'path' has too many symbolic link levels. + static string? TryGetLinkTarget(string path, string destPath, bool overwrite) { - try + if (path == destPath) { - return ResolveLinkTargetString(path, true, false); + //Throw an appropriate error + if (overwrite) throw new IOException(SR.Format(SR.IO_SharingViolation_File, destPath)); + else throw new IOException(SR.Format(SR.IO_FileExists_Name, destPath)); } - catch + string? currentTarget = null; + for (int i = 0; i < MaxFollowedLinks; i++) { - return null; + string? newTarget; + try + { + //Attempt to unlink the current link + newTarget = ResolveLinkTargetString(currentTarget ?? path, false, false); + } + catch + { + //This means path's target doesn't exist, stop unlinking + return currentTarget; + } + + //Check the new target path + if (newTarget == destPath) + { + //Throw an appropriate error + if (overwrite) throw new IOException(SR.Format(SR.IO_SharingViolation_File, destPath)); + else throw new IOException(SR.Format(SR.IO_FileExists_Name, destPath)); + } + + //Store the unlinked path, otherwise return our current path + if (string.IsNullOrEmpty(newTarget)) + { + return currentTarget; + } + else + { + currentTarget = newTarget; + } } + + //If we get here, we've gone through MaxFollowedLinks iterations + throw new IOException(SR.Format(SR.IO_TooManySymbolicLinkLevels, path)); } //Checks if a file or directory exists without caring which it was From 8d8328aef642331a31f6db791a31c40520811c21 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 17:42:09 +1100 Subject: [PATCH 09/81] Fix some nullability issues from previous commit for #77835 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Fix nullability issues --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 884ac88415141..efa025a12b4ac 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -15,15 +15,23 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, //Simplify the destination path (i.e. unlink all the links except the last one itself, //i.e. for /link1/link2, you could get /folder1/link2). string destFullPath_Full = Path.GetFullPath(destFullPath); - string destPath = Path.GetDirectoryName(destFullPath_Full); - destPath = ResolveLinkTargetString(destPath, true, true); + string? destPathFolder = Path.GetDirectoryName(destFullPath_Full); + string destPath; if (string.IsNullOrEmpty(destPath)) { destPath = destFullPath_Full; } else { - destPath = Path.Combine(destPath, Path.GetFileName(destFullPath)); + destPathFolder = ResolveLinkTargetString(destPathFolder, true, true); + if (string.IsNullOrEmpty(destPath)) + { + destPath = destFullPath_Full; + } + else + { + destPath = Path.Combine(destPathFolder, Path.GetFileName(destFullPath)); + } } //Get the full path of the source path and verify that we're not copying the source file onto itself From 835fc2f0f50535659aa1f16514bb1f112244f517 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 18:01:17 +1100 Subject: [PATCH 10/81] Fix some nullability issues from previous commit for #77835 (again) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • The variable in the if statements weren't updated • Handling for if readlink fails --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index efa025a12b4ac..b7bd5665ec32d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -17,14 +17,22 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, string destFullPath_Full = Path.GetFullPath(destFullPath); string? destPathFolder = Path.GetDirectoryName(destFullPath_Full); string destPath; - if (string.IsNullOrEmpty(destPath)) + if (string.IsNullOrEmpty(destPathFolder)) { destPath = destFullPath_Full; } else { - destPathFolder = ResolveLinkTargetString(destPathFolder, true, true); - if (string.IsNullOrEmpty(destPath)) + try + { + destPathFolder = ResolveLinkTargetString(destPathFolder, true, true); + } + catch + { + //In case readlink fails + destPathFolder = null; + } + if (string.IsNullOrEmpty(destPathFolder)) { destPath = destFullPath_Full; } From 05f9d26c816d4ebe5a792d7e5ba644df650958fb Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Dec 2022 19:12:19 +1100 Subject: [PATCH 11/81] Fix bug introduced in 'Fix copying a file onto itself logic for #77835' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • The source path should have used sourceFullPath rather than destPath --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index b7bd5665ec32d..4eb46d25535b3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -43,7 +43,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } //Get the full path of the source path and verify that we're not copying the source file onto itself - string fullSource = TryGetLinkTarget(sourceFullPath, destPath, overwrite) ?? destPath; + string fullSource = TryGetLinkTarget(sourceFullPath, destPath, overwrite) ?? sourceFullPath; //Start the file copy and prepare for finalization StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destPath, overwrite, openDst: false); From 21144e0b7c21694d6f3f11c8fc5fc5e01f838bd3 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 24 Jan 2023 14:49:09 +1100 Subject: [PATCH 12/81] Add extra test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • From #33159 • As requested by feedback • I assumed this test already existed, so it's good we have it now --- .../System.IO.FileSystem/tests/File/Copy.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/libraries/System.IO.FileSystem/tests/File/Copy.cs b/src/libraries/System.IO.FileSystem/tests/File/Copy.cs index 8b1f1c2d4e78c..4dc4714f4543f 100644 --- a/src/libraries/System.IO.FileSystem/tests/File/Copy.cs +++ b/src/libraries/System.IO.FileSystem/tests/File/Copy.cs @@ -352,6 +352,19 @@ public void WindowsAlternateDataStreamOverwrite(string defaultStream, string alt Assert.Throws(() => Copy(testFileAlternateStream, testFile2 + alternateStream, overwrite: true)); } + [Fact] + public void CopyOntoLockedFile() + { + string testFileSource = GetTestFilePath(); + string testFileDest = GetTestFilePath(); + File.Create(testFileSource).Dispose(); + File.Create(testFileDest).Dispose(); + using (var stream = new FileStream(testFileDest, FileMode.Open, FileAccess.Read, FileShare.None)) + { + Assert.Throws(() => Copy(testFileSource, testFileDest, overwrite: true)); + } + } + [Fact] public void DestinationFileIsTruncatedWhenItsLargerThanSourceFile() { From 049627fa50da55ceb4f1126eb48df693bfc8ecf1 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 24 Jan 2023 14:52:37 +1100 Subject: [PATCH 13/81] Use correct comment formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Add a space after // --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 72 +++++++++---------- .../IO/FileSystem.CopyFile.OtherUnix.cs | 4 +- .../src/System/IO/FileSystem.Unix.cs | 34 ++++----- 3 files changed, 55 insertions(+), 55 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 4eb46d25535b3..67f7fda4d0275 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -10,10 +10,10 @@ internal static partial class FileSystem { public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - //Attempt to clone the file: + // Attempt to clone the file: - //Simplify the destination path (i.e. unlink all the links except the last one itself, - //i.e. for /link1/link2, you could get /folder1/link2). + // Simplify the destination path (i.e. unlink all the links except the last one itself, + // i.e. for /link1/link2, you could get /folder1/link2). string destFullPath_Full = Path.GetFullPath(destFullPath); string? destPathFolder = Path.GetDirectoryName(destFullPath_Full); string destPath; @@ -42,49 +42,49 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } } - //Get the full path of the source path and verify that we're not copying the source file onto itself + // Get the full path of the source path and verify that we're not copying the source file onto itself string fullSource = TryGetLinkTarget(sourceFullPath, destPath, overwrite) ?? sourceFullPath; - //Start the file copy and prepare for finalization + // Start the file copy and prepare for finalization StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destPath, overwrite, openDst: false); - //Attempt counter just in case we somehow loop infinite times e.g. on a - //filesystem that doesn't actually delete files but pretends it does. - //Declare error variable here since it can be used after some jumping around. + // Attempt counter just in case we somehow loop infinite times e.g. on a + // filesystem that doesn't actually delete files but pretends it does. + // Declare error variable here since it can be used after some jumping around. int attempts = 0; int error; try { - //Don't need to re-read the link on our first attempt + // Don't need to re-read the link on our first attempt bool failOnRereadDoesntChange = false; if (overwrite) { - //Ensure file is deleted on first try. - //Get a lock to the dest file for compat reasons, and then delete it. + // Ensure file is deleted on first try. + // Get a lock to the dest file for compat reasons, and then delete it. using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destPath, true, startedCopyFile, false); File.Delete(destPath); } goto tryAgain; - //We may want to re-read the link to see if its path has changed + // We may want to re-read the link to see if its path has changed tryAgainWithReadLink: if (++attempts >= 5) goto throwError; string fullSource2 = TryGetLinkTarget(sourceFullPath, destPath, overwrite) ?? sourceFullPath; if (fullSource != fullSource2) { - //Path has changed + // Path has changed startedCopyFile.Dispose(); startedCopyFile = StartCopyFile(fullSource, destPath, overwrite, openDst: false); } else if (failOnRereadDoesntChange) { - //Path hasn't changed and we want to throw the error we got earlier + // Path hasn't changed and we want to throw the error we got earlier goto throwError; } failOnRereadDoesntChange = false; - //Attempt to clone the file + // Attempt to clone the file tryAgain: unsafe { @@ -94,7 +94,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } } - //Check the error + // Check the error error = Marshal.GetLastWin32Error(); const int ENOTSUP = 45; const int EEXIST = 17; @@ -102,7 +102,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, bool directoryExist = false; if ((error == ENOTSUP && FileOrDirectoryExists(destPath)) || error == EEXIST) { - //This means the destination existed, try again with the destination deleted if appropriate + // This means the destination existed, try again with the destination deleted if appropriate error = EEXIST; if (Directory.Exists(destPath)) { @@ -111,7 +111,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } if (overwrite) { - //Get a lock to the dest file for compat reasons, and then delete it. + // Get a lock to the dest file for compat reasons, and then delete it. using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destPath, true, startedCopyFile, false); File.Delete(destPath); goto tryAgainWithReadLink; @@ -119,18 +119,18 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } else if (error == ENOTSUP) { - //This probably means cloning is not supported, try the standard implementation + // This probably means cloning is not supported, try the standard implementation goto fallback; } else if (error == ENOENT) { - //This can happen if the source is a symlink and it has been changed to a different file, and the first has been deleted or renamed, for example. - //failOnRereadDoesntChange means we want to fail if the link didn't change, indicating the source actually doesn't exist. + // This can happen if the source is a symlink and it has been changed to a different file, and the first has been deleted or renamed, for example. + // failOnRereadDoesntChange means we want to fail if the link didn't change, indicating the source actually doesn't exist. failOnRereadDoesntChange = true; goto tryAgainWithReadLink; } - //Throw an appropriate error + // Throw an appropriate error throwError: if (directoryExist) { @@ -138,13 +138,13 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(error)); - //Fallback to the standard unix implementation for when cloning is not supported + // Fallback to the standard unix implementation for when cloning is not supported fallback: - //Open the dst handle + // Open the dst handle startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); - //Copy the file using the standard unix implementation + // Copy the file using the standard unix implementation StandardCopyFile(startedCopyFile); } finally @@ -152,14 +152,14 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, startedCopyFile.Dispose(); } - //Attempts to read the path's link target, or returns null even if the path doesn't exist rather than throwing. - //Throws an error if 'path' is at any point equal to 'destPath', since it means we're copying onto itself. - //Throws an error if 'path' has too many symbolic link levels. + // Attempts to read the path's link target, or returns null even if the path doesn't exist rather than throwing. + // Throws an error if 'path' is at any point equal to 'destPath', since it means we're copying onto itself. + // Throws an error if 'path' has too many symbolic link levels. static string? TryGetLinkTarget(string path, string destPath, bool overwrite) { if (path == destPath) { - //Throw an appropriate error + // Throw an appropriate error if (overwrite) throw new IOException(SR.Format(SR.IO_SharingViolation_File, destPath)); else throw new IOException(SR.Format(SR.IO_FileExists_Name, destPath)); } @@ -169,24 +169,24 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, string? newTarget; try { - //Attempt to unlink the current link + // Attempt to unlink the current link newTarget = ResolveLinkTargetString(currentTarget ?? path, false, false); } catch { - //This means path's target doesn't exist, stop unlinking + // This means path's target doesn't exist, stop unlinking return currentTarget; } - //Check the new target path + // Check the new target path if (newTarget == destPath) { - //Throw an appropriate error + // Throw an appropriate error if (overwrite) throw new IOException(SR.Format(SR.IO_SharingViolation_File, destPath)); else throw new IOException(SR.Format(SR.IO_FileExists_Name, destPath)); } - //Store the unlinked path, otherwise return our current path + // Store the unlinked path, otherwise return our current path if (string.IsNullOrEmpty(newTarget)) { return currentTarget; @@ -197,11 +197,11 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } } - //If we get here, we've gone through MaxFollowedLinks iterations + // If we get here, we've gone through MaxFollowedLinks iterations throw new IOException(SR.Format(SR.IO_TooManySymbolicLinkLevels, path)); } - //Checks if a file or directory exists without caring which it was + // Checks if a file or directory exists without caring which it was static bool FileOrDirectoryExists(string path) { return Interop.Sys.Stat(path, out _) >= 0; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index b9069b7345684..74f371217faff 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -7,10 +7,10 @@ internal static partial class FileSystem { public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - //Start the file copy and prepare for finalization + // Start the file copy and prepare for finalization using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); - //Copy the file using the standard unix implementation + // Copy the file using the standard unix implementation StandardCopyFile(startedCopyFile); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 8013422d1c96a..df47767ce0697 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,8 +28,8 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - //Helper type to facilitate returning values from StartCopyFile without having - //to declare a massive tuple multiple times, and making it easier to dispose. + // Helper type to facilitate returning values from StartCopyFile without having + // to declare a massive tuple multiple times, and making it easier to dispose. private struct StartedCopyFileState : IDisposable { public long fileLength; @@ -54,9 +54,9 @@ public void Dispose() private static StartedCopyFileState StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite, bool openDst = true) { - //The return value is expected to be Disposed by the caller (unless this method throws) once the copy is complete. - //Begins 'CopyFile' by locking and creating the relevant file handles. - //If 'openDst' is false, it doesn't open the destination file handle, nor check anything to do with it (used in macOS implementation). + // The return value is expected to be Disposed by the caller (unless this method throws) once the copy is complete. + // Begins 'CopyFile' by locking and creating the relevant file handles. + // If 'openDst' is false, it doesn't open the destination file handle, nor check anything to do with it (used in macOS implementation). StartedCopyFileState startedCopyFile = default; try @@ -70,15 +70,15 @@ private static StartedCopyFileState StartCopyFile(string sourceFullPath, string throw; } - //Return the collection of information we have gotten back. + // Return the collection of information we have gotten back. return startedCopyFile; } private static SafeFileHandle? OpenCopyFileDstHandle(string destFullPath, bool overwrite, StartedCopyFileState startedCopyFile, bool openNewFile) { - //This function opens the 'dst' file handle for 'CopyFile', it is - //split out since the logic on OSX-like OSes is a bit different. - //'openNewFile' = false is used when we want to try to find the file only. + // This function opens the 'dst' file handle for 'CopyFile', it is + // split out since the logic on OSX-like OSes is a bit different. + // 'openNewFile' = false is used when we want to try to find the file only. if (!openNewFile) { try @@ -110,17 +110,17 @@ private static StartedCopyFileState StartCopyFile(string sourceFullPath, string private static void StandardCopyFile(StartedCopyFileState startedCopyFile) { - //Copy the file in a way that works on all Unix Operating Systems. - //The 'startedCopyFile' parameter should take the output from 'StartCopyFile'. - //'startedCopyFile' should be disposed by the caller. Assumes src and dst - //are non-null, caller must check this, return values from StartCopyFile - //are non-null (except dst when openDst is true), and from return values from - //OpenCopyFileDstHandle are non-null (except possibly when openNewFile is false). + // Copy the file in a way that works on all Unix Operating Systems. + // The 'startedCopyFile' parameter should take the output from 'StartCopyFile'. + // 'startedCopyFile' should be disposed by the caller. Assumes src and dst + // are non-null, caller must check this, return values from StartCopyFile + // are non-null (except dst when openDst is true), and from return values from + // OpenCopyFileDstHandle are non-null (except possibly when openNewFile is false). Interop.CheckIo(Interop.Sys.CopyFile(startedCopyFile.src!, startedCopyFile.dst!, startedCopyFile.fileLength)); } - //CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs - //The implementations on OSX-like Operating Systems attempts to clone the file first. + // CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs + // The implementations on OSX-like Operating Systems attempts to clone the file first. public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); #pragma warning disable IDE0060 From ab18c053df6428e7aa318154ca6dc2183bbb38de Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 24 Jan 2023 19:03:44 +1100 Subject: [PATCH 14/81] Use clonefile API instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Use clonefile API instead • Update OpenReadOnly API to return the whole FileStatus • Revert changes to ResolveLinkTarget • Incorporate some feedback about how CopyFile should be implemented - Use st_dev and st_ino to optimise checks - Read them from an already required fstat call - Avoid calling clonefile when it would be likely to fail - Avoid expensive destination file locking scheme when clonefile won't be called - Fail fast when copying file onto itself --- .../Common/src/Interop/OSX/Interop.libc.cs | 5 +- .../Win32/SafeHandles/SafeFileHandle.Unix.cs | 29 +- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 249 +++++++----------- .../src/System/IO/FileSystem.Unix.cs | 26 +- 4 files changed, 125 insertions(+), 184 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs index b03b41e3fbf3a..1523b14c752ce 100644 --- a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs +++ b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs @@ -45,8 +45,7 @@ internal static unsafe int fsetattrlist(SafeHandle handle, AttrList* attrList, v } } - [LibraryImport(Libraries.libc, EntryPoint = "copyfile", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)] - internal static unsafe partial int copyfile(string from, string to, void* state, uint flags); - internal const uint COPYFILE_CLONE_FORCE = 0x02000000; + [LibraryImport(Libraries.libc, EntryPoint = "clonefile", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)] + internal static unsafe partial int clonefile(string from, string to, int flags); } } diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index fb5c8faad4688..2426afe8ec539 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs @@ -165,15 +165,21 @@ public override bool IsInvalid } } - // Specialized Open that returns the file length and permissions of the opened file. + // Specialized Open that returns the filestatus of the opened file. // This information is retrieved from the 'stat' syscall that must be performed to ensure the path is not a directory. - internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options, out long fileLength, out UnixFileMode filePermissions) + internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options, out Interop.Sys.FileStatus status) { - SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out fileLength, out filePermissions, null); - Debug.Assert(fileLength >= 0); + SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out status, out var readStatus, null); + Debug.Assert(readStatus == true); return handle; } + // Used by callers of OpenReadOnly. + internal static UnixFileMode FilePermissionsForStatus(Interop.Sys.FileStatus status) + { + return ((UnixFileMode)status.Mode) & PermissionMask; + } + internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode? unixCreateMode = null, Func? createOpenException = null) { @@ -181,7 +187,7 @@ internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess a } private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode openPermissions, - out long fileLength, out UnixFileMode filePermissions, + out Interop.Sys.FileStatus status, out bool readStatus, Func? createOpenException = null) { // Translate the arguments into arguments for an open call. @@ -196,7 +202,7 @@ private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess ac // When Init return false, the path has changed to another file entry, and // we need to re-open the path to reflect that. - if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize, out fileLength, out filePermissions)) + if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize, out status, out readStatus)) { return safeFileHandle; } @@ -294,12 +300,12 @@ private static Interop.Sys.OpenFlags PreOpenConfigurationFromOptions(FileMode mo } private bool Init(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, - out long fileLength, out UnixFileMode filePermissions) + out Interop.Sys.FileStatus status, out bool readStatus) { - Interop.Sys.FileStatus status = default; bool statusHasValue = false; - fileLength = -1; - filePermissions = 0; + + status = default; + readStatus = false; // Make sure our handle is not a directory. // We can omit the check when write access is requested. open will have failed with EISDIR. @@ -322,8 +328,7 @@ private bool Init(string path, FileMode mode, FileAccess access, FileShare share Debug.Assert(Interop.Sys.LSeek(this, 0, Interop.Sys.SeekWhence.SEEK_CUR) >= 0); } - fileLength = status.Size; - filePermissions = ((UnixFileMode)status.Mode) & PermissionMask; + readStatus = true; } IsAsync = (options & FileOptions.Asynchronous) != 0; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 67f7fda4d0275..ec3fc856b4629 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Runtime.InteropServices; using Microsoft.Win32.SafeHandles; @@ -12,199 +13,135 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, { // Attempt to clone the file: - // Simplify the destination path (i.e. unlink all the links except the last one itself, - // i.e. for /link1/link2, you could get /folder1/link2). - string destFullPath_Full = Path.GetFullPath(destFullPath); - string? destPathFolder = Path.GetDirectoryName(destFullPath_Full); - string destPath; - if (string.IsNullOrEmpty(destPathFolder)) + // Helper function to throw an error for copying onto self + [StackTraceHidden] + void CopyOntoSelfError() { - destPath = destFullPath_Full; + // Throw an appropriate error + if (overwrite) throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); + else throw new IOException(SR.Format(SR.IO_FileExists_Name, destFullPath)); } - else + + // Helper function to throw an error when the destination exists and overwrite = false. + [StackTraceHidden] + static void DestinationExistsError() { - try - { - destPathFolder = ResolveLinkTargetString(destPathFolder, true, true); - } - catch - { - //In case readlink fails - destPathFolder = null; - } - if (string.IsNullOrEmpty(destPathFolder)) - { - destPath = destFullPath_Full; - } - else - { - destPath = Path.Combine(destPathFolder, Path.GetFileName(destFullPath)); - } + throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); } - // Get the full path of the source path and verify that we're not copying the source file onto itself - string fullSource = TryGetLinkTarget(sourceFullPath, destPath, overwrite) ?? sourceFullPath; + // Fail fast for blatantly copying onto self + if (sourceFullPath == destFullPath) + { + CopyOntoSelfError(); + } // Start the file copy and prepare for finalization - StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destPath, overwrite, openDst: false); - - // Attempt counter just in case we somehow loop infinite times e.g. on a - // filesystem that doesn't actually delete files but pretends it does. - // Declare error variable here since it can be used after some jumping around. - int attempts = 0; - int error; + StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite, openDst: false); + // Ensure we dispose startedCopyFile. try { - // Don't need to re-read the link on our first attempt - bool failOnRereadDoesntChange = false; - if (overwrite) - { - // Ensure file is deleted on first try. - // Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destPath, true, startedCopyFile, false); - File.Delete(destPath); - } - goto tryAgain; + // Read filestatus of destination file to determine how we continue + int destError = Interop.Sys.Stat(destFullPath, out var destStat); - // We may want to re-read the link to see if its path has changed - tryAgainWithReadLink: - if (++attempts >= 5) goto throwError; - string fullSource2 = TryGetLinkTarget(sourceFullPath, destPath, overwrite) ?? sourceFullPath; - if (fullSource != fullSource2) - { - // Path has changed - startedCopyFile.Dispose(); - startedCopyFile = StartCopyFile(fullSource, destPath, overwrite, openDst: false); - } - else if (failOnRereadDoesntChange) - { - // Path hasn't changed and we want to throw the error we got earlier - goto throwError; - } - failOnRereadDoesntChange = false; - - // Attempt to clone the file + // Counter to count the amount of times we have repeated. Used in case EEXIST is thrown by clonefile, indicating the + // file was re-created before we copy. Not a problematic error, but we do not want to retry an infinite amount of times. + int repeats = 0; tryAgain: - unsafe + + // Interpret the error from stat + if (destError != 0) { - if (Interop.@libc.copyfile(fullSource, destPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0) + // Some error, let's see what it is: + Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo(); + + if (error.Error == Interop.Error.ENOENT) { - return; + goto tryCloneFile; + } + else + { + goto tryFallback; } } - - // Check the error - error = Marshal.GetLastWin32Error(); - const int ENOTSUP = 45; - const int EEXIST = 17; - const int ENOENT = 2; - bool directoryExist = false; - if ((error == ENOTSUP && FileOrDirectoryExists(destPath)) || error == EEXIST) + else { - // This means the destination existed, try again with the destination deleted if appropriate - error = EEXIST; - if (Directory.Exists(destPath)) + // Destination exists: + if (!overwrite) { - directoryExist = true; - goto throwError; + // Throw an error if we're not overriding + DestinationExistsError(); } - if (overwrite) + if (startedCopyFile.fileDev != destStat.Dev) { - // Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destPath, true, startedCopyFile, false); - File.Delete(destPath); - goto tryAgainWithReadLink; + // On different device + goto tryFallback; + } + else if (startedCopyFile.fileIno == destStat.Ino) + { + // Copying onto itself + CopyOntoSelfError(); + } + else + { + goto tryDelete; } - } - else if (error == ENOTSUP) - { - // This probably means cloning is not supported, try the standard implementation - goto fallback; - } - else if (error == ENOENT) - { - // This can happen if the source is a symlink and it has been changed to a different file, and the first has been deleted or renamed, for example. - // failOnRereadDoesntChange means we want to fail if the link didn't change, indicating the source actually doesn't exist. - failOnRereadDoesntChange = true; - goto tryAgainWithReadLink; } - // Throw an appropriate error - throwError: - if (directoryExist) + // Try deleting destination: + tryDelete: { - throw new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, destFullPath)); + // Delete the destination. This should fail on directories. And update the mode. + // Get a lock to the dest file for compat reasons, and then delete it. + using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); + File.Delete(destFullPath); } - throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(error)); - - // Fallback to the standard unix implementation for when cloning is not supported - fallback: - - // Open the dst handle - startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); - // Copy the file using the standard unix implementation - StandardCopyFile(startedCopyFile); - } - finally - { - startedCopyFile.Dispose(); - } - - // Attempts to read the path's link target, or returns null even if the path doesn't exist rather than throwing. - // Throws an error if 'path' is at any point equal to 'destPath', since it means we're copying onto itself. - // Throws an error if 'path' has too many symbolic link levels. - static string? TryGetLinkTarget(string path, string destPath, bool overwrite) - { - if (path == destPath) + // Try clonefile: + tryCloneFile: { - // Throw an appropriate error - if (overwrite) throw new IOException(SR.Format(SR.IO_SharingViolation_File, destPath)); - else throw new IOException(SR.Format(SR.IO_FileExists_Name, destPath)); - } - string? currentTarget = null; - for (int i = 0; i < MaxFollowedLinks; i++) - { - string? newTarget; - try - { - // Attempt to unlink the current link - newTarget = ResolveLinkTargetString(currentTarget ?? path, false, false); - } - catch + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, 0) == 0) { - // This means path's target doesn't exist, stop unlinking - return currentTarget; + // Success + return; } - // Check the new target path - if (newTarget == destPath) - { - // Throw an appropriate error - if (overwrite) throw new IOException(SR.Format(SR.IO_SharingViolation_File, destPath)); - else throw new IOException(SR.Format(SR.IO_FileExists_Name, destPath)); - } + // Read error + Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo(); - // Store the unlinked path, otherwise return our current path - if (string.IsNullOrEmpty(newTarget)) - { - return currentTarget; - } - else + // Check error + if (error.Error == Interop.Error.EEXIST) { - currentTarget = newTarget; + if (!overwrite) + { + // Throw an error if we're not overriding + DestinationExistsError(); + } + + // Destination existed, try again (up a certain number of times). + if (++repeats < 5) + { + goto tryAgain; + } } + + // Try fallback + // goto tryFallback; } - // If we get here, we've gone through MaxFollowedLinks iterations - throw new IOException(SR.Format(SR.IO_TooManySymbolicLinkLevels, path)); - } + // Try fallback: + tryFallback: + { + // Open the dst handle + startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); - // Checks if a file or directory exists without caring which it was - static bool FileOrDirectoryExists(string path) + // Copy the file using the standard unix implementation + StandardCopyFile(startedCopyFile); + } + } + finally { - return Interop.Sys.Stat(path, out _) >= 0; + startedCopyFile.Dispose(); } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index df47767ce0697..3448e0493b3aa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -34,13 +34,17 @@ private struct StartedCopyFileState : IDisposable { public long fileLength; public UnixFileMode filePermissions; + public long fileDev; + public long fileIno; public SafeFileHandle? src; public SafeFileHandle? dst; - public StartedCopyFileState(long fileLength, UnixFileMode filePermissions, SafeFileHandle src, SafeFileHandle dst) + public StartedCopyFileState(long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle dst) { this.fileLength = fileLength; this.filePermissions = filePermissions; + this.fileDev = fileDev; + this.fileIno = fileIno; this.src = src; this.dst = dst; } @@ -61,7 +65,11 @@ private static StartedCopyFileState StartCopyFile(string sourceFullPath, string StartedCopyFileState startedCopyFile = default; try { - startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out startedCopyFile.fileLength, out startedCopyFile.filePermissions); + startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var fileStatus); + startedCopyFile.fileLength = fileStatus.Size; + startedCopyFile.filePermissions = SafeFileHandle.FilePermissionsForStatus(fileStatus); + startedCopyFile.fileDev = fileStatus.Dev; + startedCopyFile.fileIno = fileStatus.Ino; if (openDst) startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); } catch @@ -731,16 +739,6 @@ internal static void CreateSymbolicLink(string path, string pathToTarget, bool i #pragma warning restore IDE0060 internal static FileSystemInfo? ResolveLinkTarget(string linkPath, bool returnFinalTarget, bool isDirectory) - { - string? linkTarget = ResolveLinkTargetString(linkPath, returnFinalTarget, isDirectory); - if (linkTarget == null) return null; - - return isDirectory ? - new DirectoryInfo(linkTarget) : - new FileInfo(linkTarget); - } - - private static string? ResolveLinkTargetString(string linkPath, bool returnFinalTarget, bool isDirectory) { ValueStringBuilder sb = new(Interop.DefaultPathBufferSize); sb.Append(linkPath); @@ -786,7 +784,9 @@ internal static void CreateSymbolicLink(string path, string pathToTarget, bool i Debug.Assert(sb.Length > 0); linkTarget = sb.ToString(); // ToString disposes - return linkTarget; + return isDirectory ? + new DirectoryInfo(linkTarget) : + new FileInfo(linkTarget); // In case of link target being relative: // Preserve the full path of the directory of the previous path From 776f500f41b83424e83b081982fd087e32f46131 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 24 Jan 2023 19:17:52 +1100 Subject: [PATCH 15/81] Remove redundant equality --- .../src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index 2426afe8ec539..0aee9949af81b 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs @@ -170,7 +170,7 @@ public override bool IsInvalid internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options, out Interop.Sys.FileStatus status) { SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out status, out var readStatus, null); - Debug.Assert(readStatus == true); + Debug.Assert(readStatus); return handle; } From dcddd8f9c9e6f8bf6818a01eb2ac2401a3ab9d41 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Mon, 30 Jan 2023 21:02:10 +1100 Subject: [PATCH 16/81] Clone ACLs and make CopyOntoLockedFile test conditional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • When using clonefile, specify CLONE_ACL to ensure ACLs are cloned, matching previous behaviour • Make CopyOntoLockedFile test conditional to platforms that support file locking --- src/libraries/Common/src/Interop/OSX/Interop.libc.cs | 1 + src/libraries/System.IO.FileSystem/tests/File/Copy.cs | 2 +- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs index 1523b14c752ce..8d79b9caca7d4 100644 --- a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs +++ b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs @@ -47,5 +47,6 @@ internal static unsafe int fsetattrlist(SafeHandle handle, AttrList* attrList, v [LibraryImport(Libraries.libc, EntryPoint = "clonefile", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)] internal static unsafe partial int clonefile(string from, string to, int flags); + internal const int CLONE_ACL = 0x0004; } } diff --git a/src/libraries/System.IO.FileSystem/tests/File/Copy.cs b/src/libraries/System.IO.FileSystem/tests/File/Copy.cs index 4dc4714f4543f..4ab26c3748877 100644 --- a/src/libraries/System.IO.FileSystem/tests/File/Copy.cs +++ b/src/libraries/System.IO.FileSystem/tests/File/Copy.cs @@ -352,7 +352,7 @@ public void WindowsAlternateDataStreamOverwrite(string defaultStream, string alt Assert.Throws(() => Copy(testFileAlternateStream, testFile2 + alternateStream, overwrite: true)); } - [Fact] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsFileLockingEnabled))] public void CopyOntoLockedFile() { string testFileSource = GetTestFilePath(); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index ec3fc856b4629..5ccaff1cd6bec 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -100,7 +100,7 @@ static void DestinationExistsError() // Try clonefile: tryCloneFile: { - if (Interop.@libc.clonefile(sourceFullPath, destFullPath, 0) == 0) + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) { // Success return; From ac27788265869c015dd5d9818302ce6556874447 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Mon, 30 Jan 2023 21:04:43 +1100 Subject: [PATCH 17/81] Remove retry logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Remove retry logic after feedback that it is not necessary --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 5ccaff1cd6bec..8fb46a76eb842 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -44,11 +44,6 @@ static void DestinationExistsError() // Read filestatus of destination file to determine how we continue int destError = Interop.Sys.Stat(destFullPath, out var destStat); - // Counter to count the amount of times we have repeated. Used in case EEXIST is thrown by clonefile, indicating the - // file was re-created before we copy. Not a problematic error, but we do not want to retry an infinite amount of times. - int repeats = 0; - tryAgain: - // Interpret the error from stat if (destError != 0) { @@ -117,12 +112,6 @@ static void DestinationExistsError() // Throw an error if we're not overriding DestinationExistsError(); } - - // Destination existed, try again (up a certain number of times). - if (++repeats < 5) - { - goto tryAgain; - } } // Try fallback From f320a700a47a6638796945e1091748a0bf2f7eca Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Sun, 14 May 2023 10:15:51 +1000 Subject: [PATCH 18/81] Address feedback Simplify error throwing, rename readStatus to isReadOnly, prepare for removal of StartedCopyFileState type. Co-authored-by: Stephen Toub Squashed 19 commits, which addressed most of Stephen Toub's feedback. Thanks GH for all of those separate commits :) --- .../Common/src/Interop/OSX/Interop.libc.cs | 2 +- .../Win32/SafeHandles/SafeFileHandle.Unix.cs | 22 ++++---- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 51 +++++-------------- .../IO/FileSystem.CopyFile.OtherUnix.cs | 1 - .../src/System/IO/FileSystem.Unix.cs | 2 +- 5 files changed, 25 insertions(+), 53 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs index 8d79b9caca7d4..c33dcdf3ecd2f 100644 --- a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs +++ b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs @@ -46,7 +46,7 @@ internal static unsafe int fsetattrlist(SafeHandle handle, AttrList* attrList, v } [LibraryImport(Libraries.libc, EntryPoint = "clonefile", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)] - internal static unsafe partial int clonefile(string from, string to, int flags); + internal static unsafe partial int clonefile(string src, string dst, int flags); internal const int CLONE_ACL = 0x0004; } } diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index 0aee9949af81b..d3f6c36b153de 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs @@ -165,20 +165,18 @@ public override bool IsInvalid } } - // Specialized Open that returns the filestatus of the opened file. + // Specialized Open that returns the FileStatus of the opened file. // This information is retrieved from the 'stat' syscall that must be performed to ensure the path is not a directory. internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options, out Interop.Sys.FileStatus status) { - SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out status, out var readStatus, null); - Debug.Assert(readStatus); + SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out status, out bool isReadOnly); + Debug.Assert(isReadOnly); return handle; } // Used by callers of OpenReadOnly. - internal static UnixFileMode FilePermissionsForStatus(Interop.Sys.FileStatus status) - { - return ((UnixFileMode)status.Mode) & PermissionMask; - } + internal static UnixFileMode GetFileMode(Interop.Sys.FileStatus status) => + ((UnixFileMode)status.Mode) & PermissionMask; internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode? unixCreateMode = null, Func? createOpenException = null) @@ -187,7 +185,7 @@ internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess a } private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode openPermissions, - out Interop.Sys.FileStatus status, out bool readStatus, + out Interop.Sys.FileStatus status, out bool isReadOnly, Func? createOpenException = null) { // Translate the arguments into arguments for an open call. @@ -202,7 +200,7 @@ private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess ac // When Init return false, the path has changed to another file entry, and // we need to re-open the path to reflect that. - if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize, out status, out readStatus)) + if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize, out status, out isReadOnly)) { return safeFileHandle; } @@ -300,12 +298,12 @@ private static Interop.Sys.OpenFlags PreOpenConfigurationFromOptions(FileMode mo } private bool Init(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, - out Interop.Sys.FileStatus status, out bool readStatus) + out Interop.Sys.FileStatus status, out bool isReadOnly) { bool statusHasValue = false; status = default; - readStatus = false; + isReadOnly = false; // Make sure our handle is not a directory. // We can omit the check when write access is requested. open will have failed with EISDIR. @@ -328,7 +326,7 @@ private bool Init(string path, FileMode mode, FileAccess access, FileShare share Debug.Assert(Interop.Sys.LSeek(this, 0, Interop.Sys.SeekWhence.SEEK_CUR) >= 0); } - readStatus = true; + isReadOnly = true; } IsAsync = (options & FileOptions.Asynchronous) != 0; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 8fb46a76eb842..8b8b8df70825b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -11,37 +11,22 @@ internal static partial class FileSystem { public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - // Attempt to clone the file: - - // Helper function to throw an error for copying onto self - [StackTraceHidden] - void CopyOntoSelfError() - { - // Throw an appropriate error - if (overwrite) throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); - else throw new IOException(SR.Format(SR.IO_FileExists_Name, destFullPath)); - } - - // Helper function to throw an error when the destination exists and overwrite = false. - [StackTraceHidden] - static void DestinationExistsError() - { - throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); - } - // Fail fast for blatantly copying onto self if (sourceFullPath == destFullPath) { - CopyOntoSelfError(); + if (overwrite) + { + throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); + } + + throw new IOException(SR.Format(SR.IO_FileExists_Name, destFullPath)); } - // Start the file copy and prepare for finalization + // Start the copy StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite, openDst: false); - - // Ensure we dispose startedCopyFile. try { - // Read filestatus of destination file to determine how we continue + // Read FileStatus of destination file to determine how to continue int destError = Interop.Sys.Stat(destFullPath, out var destStat); // Interpret the error from stat @@ -65,7 +50,7 @@ static void DestinationExistsError() if (!overwrite) { // Throw an error if we're not overriding - DestinationExistsError(); + throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); } if (startedCopyFile.fileDev != destStat.Dev) { @@ -75,7 +60,7 @@ static void DestinationExistsError() else if (startedCopyFile.fileIno == destStat.Ino) { // Copying onto itself - CopyOntoSelfError(); + throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); } else { @@ -101,21 +86,11 @@ static void DestinationExistsError() return; } - // Read error - Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo(); - - // Check error - if (error.Error == Interop.Error.EEXIST) + // Throw if the file already exists and we're not overwriting + if (Interop.Sys.GetLastError() == Interop.Error.EEXIST && !overwrite) { - if (!overwrite) - { - // Throw an error if we're not overriding - DestinationExistsError(); - } + throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); } - - // Try fallback - // goto tryFallback; } // Try fallback: diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index 74f371217faff..92240959b40c8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -7,7 +7,6 @@ internal static partial class FileSystem { public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - // Start the file copy and prepare for finalization using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); // Copy the file using the standard unix implementation diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 3448e0493b3aa..d79b39bb4d1ff 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -67,7 +67,7 @@ private static StartedCopyFileState StartCopyFile(string sourceFullPath, string { startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var fileStatus); startedCopyFile.fileLength = fileStatus.Size; - startedCopyFile.filePermissions = SafeFileHandle.FilePermissionsForStatus(fileStatus); + startedCopyFile.filePermissions = SafeFileHandle.GetFileMode(fileStatus); startedCopyFile.fileDev = fileStatus.Dev; startedCopyFile.fileIno = fileStatus.Ino; if (openDst) startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); From 3d4a962681bd1e3ad6ef9a3525b3ec2083ecbaf4 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Sun, 14 May 2023 10:50:31 +1000 Subject: [PATCH 19/81] Implement feedback to remove StartedCopyFileState --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 10 ++-- .../IO/FileSystem.CopyFile.OtherUnix.cs | 16 ++++-- .../src/System/IO/FileSystem.Unix.cs | 49 ++++--------------- 3 files changed, 28 insertions(+), 47 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 8b8b8df70825b..aed2e9480afe2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -23,7 +23,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } // Start the copy - StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite, openDst: false); + (long fileLength, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite, openDst: false); try { // Read FileStatus of destination file to determine how to continue @@ -97,15 +97,17 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, tryFallback: { // Open the dst handle - startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); + startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, openNewFile: true); // Copy the file using the standard unix implementation - StandardCopyFile(startedCopyFile); + // dst! because dst is not null if OpenCopyFileDstHandle's openNewFile is true. + StandardCopyFile(startedCopyFile.src, startedCopyFile.dst!, startedCopyFile.fileLength); } } finally { - startedCopyFile.Dispose(); + startedCopyFile.src?.Dispose(); + startedCopyFile.dst?.Dispose(); } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index 92240959b40c8..0f74e89cb1bd4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -7,10 +7,20 @@ internal static partial class FileSystem { public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite); + var (fileLength, _, _, src, dst) = StartCopyFile(sourceFullPath, destFullPath, overwrite); - // Copy the file using the standard unix implementation - StandardCopyFile(startedCopyFile); + try + { + // Copy the file using the standard unix implementation + // dst! because dst is not null if StartCopyFile's openDst is true (which is the default value) + StandardCopyFile(src, dst!, fileLength); + } + finally + { + // Dipose relevant file handles + src.Dispose(); + dst?.Dispose(); + } } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index d79b39bb4d1ff..2731eecc30b8a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,53 +28,25 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - // Helper type to facilitate returning values from StartCopyFile without having - // to declare a massive tuple multiple times, and making it easier to dispose. - private struct StartedCopyFileState : IDisposable + private static (long fileLength, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite, bool openDst = true) { - public long fileLength; - public UnixFileMode filePermissions; - public long fileDev; - public long fileIno; - public SafeFileHandle? src; - public SafeFileHandle? dst; - - public StartedCopyFileState(long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle dst) - { - this.fileLength = fileLength; - this.filePermissions = filePermissions; - this.fileDev = fileDev; - this.fileIno = fileIno; - this.src = src; - this.dst = dst; - } - - public void Dispose() - { - src?.Dispose(); - dst?.Dispose(); - } - } - - private static StartedCopyFileState StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite, bool openDst = true) - { - // The return value is expected to be Disposed by the caller (unless this method throws) once the copy is complete. + // The return value has SafeFileHandles, which are expected to be Disposed by the caller (unless this method throws) once the copy is complete. // Begins 'CopyFile' by locking and creating the relevant file handles. // If 'openDst' is false, it doesn't open the destination file handle, nor check anything to do with it (used in macOS implementation). - StartedCopyFileState startedCopyFile = default; + (long fileLength, long fileDev, long fileIno, SafeFileHandle? src, SafeFileHandle? dst) startedCopyFile = default; try { startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var fileStatus); startedCopyFile.fileLength = fileStatus.Size; - startedCopyFile.filePermissions = SafeFileHandle.GetFileMode(fileStatus); startedCopyFile.fileDev = fileStatus.Dev; startedCopyFile.fileIno = fileStatus.Ino; if (openDst) startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); } catch { - startedCopyFile.Dispose(); + startedCopyFile.src?.Dispose(); + startedCopyFile.dst?.Dispose(); throw; } @@ -116,15 +88,12 @@ private static StartedCopyFileState StartCopyFile(string sourceFullPath, string } } - private static void StandardCopyFile(StartedCopyFileState startedCopyFile) + private static void StandardCopyFile(SafeFileHandle src, SafeFileHandle dst, long fileLength) { // Copy the file in a way that works on all Unix Operating Systems. - // The 'startedCopyFile' parameter should take the output from 'StartCopyFile'. - // 'startedCopyFile' should be disposed by the caller. Assumes src and dst - // are non-null, caller must check this, return values from StartCopyFile - // are non-null (except dst when openDst is true), and from return values from - // OpenCopyFileDstHandle are non-null (except possibly when openNewFile is false). - Interop.CheckIo(Interop.Sys.CopyFile(startedCopyFile.src!, startedCopyFile.dst!, startedCopyFile.fileLength)); + // The 'src', 'dst', and 'fileLength' parameters should take the output from 'StartCopyFile'. + // 'src' and 'dst' should be disposed by the caller. + Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileLength)); } // CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs From a908a79de105efe9f17bece3ec8606951bfa0718 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Sun, 14 May 2023 15:33:18 +1000 Subject: [PATCH 20/81] Fix compilation issues by me thinking I was smart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Add missing filePermissions field to return of StartCopyFile (which I removed because I didn't see it being used anywhere, but of course I didn't search for it) --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 6 +++--- .../src/System/IO/FileSystem.CopyFile.OtherUnix.cs | 2 +- .../src/System/IO/FileSystem.Unix.cs | 11 ++++++----- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index aed2e9480afe2..9f280a4a82806 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -23,7 +23,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } // Start the copy - (long fileLength, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite, openDst: false); + (long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite, openDst: false); try { // Read FileStatus of destination file to determine how to continue @@ -73,7 +73,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, { // Delete the destination. This should fail on directories. And update the mode. // Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false); + using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile.filePermissions, false); File.Delete(destFullPath); } @@ -97,7 +97,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, tryFallback: { // Open the dst handle - startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, openNewFile: true); + startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile.filePermissions, openNewFile: true); // Copy the file using the standard unix implementation // dst! because dst is not null if OpenCopyFileDstHandle's openNewFile is true. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index 0f74e89cb1bd4..5bd69e487523a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -7,7 +7,7 @@ internal static partial class FileSystem { public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - var (fileLength, _, _, src, dst) = StartCopyFile(sourceFullPath, destFullPath, overwrite); + var (fileLength, _, _, _, src, dst) = StartCopyFile(sourceFullPath, destFullPath, overwrite); try { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 2731eecc30b8a..792f448201297 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,20 +28,21 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - private static (long fileLength, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite, bool openDst = true) + private static (long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite, bool openDst = true) { // The return value has SafeFileHandles, which are expected to be Disposed by the caller (unless this method throws) once the copy is complete. // Begins 'CopyFile' by locking and creating the relevant file handles. // If 'openDst' is false, it doesn't open the destination file handle, nor check anything to do with it (used in macOS implementation). - (long fileLength, long fileDev, long fileIno, SafeFileHandle? src, SafeFileHandle? dst) startedCopyFile = default; + (long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle? src, SafeFileHandle? dst) startedCopyFile = default; try { startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var fileStatus); startedCopyFile.fileLength = fileStatus.Size; + startedCopyFile.filePermissions = SafeFileHandle.GetFileMode(fileStatus); startedCopyFile.fileDev = fileStatus.Dev; startedCopyFile.fileIno = fileStatus.Ino; - if (openDst) startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true); + if (openDst) startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile.filePermissions, true); } catch { @@ -54,7 +55,7 @@ private static (long fileLength, long fileDev, long fileIno, SafeFileHandle src, return startedCopyFile; } - private static SafeFileHandle? OpenCopyFileDstHandle(string destFullPath, bool overwrite, StartedCopyFileState startedCopyFile, bool openNewFile) + private static SafeFileHandle? OpenCopyFileDstHandle(string destFullPath, bool overwrite, UnixFileMode filePermissions, bool openNewFile) { // This function opens the 'dst' file handle for 'CopyFile', it is // split out since the logic on OSX-like OSes is a bit different. @@ -64,7 +65,7 @@ private static (long fileLength, long fileDev, long fileIno, SafeFileHandle src, try { return SafeFileHandle.Open(destFullPath, FileMode.Open, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, startedCopyFile.filePermissions, + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, CreateOpenException); } catch (FileNotFoundException) From 16cedc652aa18347ab32e89a94784aa1e094a8a0 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Sun, 14 May 2023 16:43:48 +1000 Subject: [PATCH 21/81] Fix nullability --- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 792f448201297..5b38b0c77c466 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -34,7 +34,7 @@ private static (long fileLength, UnixFileMode filePermissions, long fileDev, lon // Begins 'CopyFile' by locking and creating the relevant file handles. // If 'openDst' is false, it doesn't open the destination file handle, nor check anything to do with it (used in macOS implementation). - (long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle? src, SafeFileHandle? dst) startedCopyFile = default; + (long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) startedCopyFile = default; try { startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var fileStatus); From d15fed39b8924f7442988e7cb30a3d1bdea7bc99 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 16 May 2023 11:33:26 +1000 Subject: [PATCH 22/81] Implement feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Implement feedback to remove StandardCopyFile, and to remove StartCopyFile • Add new line before CLONE_ACL line --- .../Common/src/Interop/OSX/Interop.libc.cs | 1 + .../src/System/IO/FileSystem.CopyFile.OSX.cs | 135 +++++++++--------- .../IO/FileSystem.CopyFile.OtherUnix.cs | 20 +-- .../src/System/IO/FileSystem.Unix.cs | 35 ----- 4 files changed, 74 insertions(+), 117 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs index c33dcdf3ecd2f..adf70d390872d 100644 --- a/src/libraries/Common/src/Interop/OSX/Interop.libc.cs +++ b/src/libraries/Common/src/Interop/OSX/Interop.libc.cs @@ -47,6 +47,7 @@ internal static unsafe int fsetattrlist(SafeHandle handle, AttrList* attrList, v [LibraryImport(Libraries.libc, EntryPoint = "clonefile", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)] internal static unsafe partial int clonefile(string src, string dst, int flags); + internal const int CLONE_ACL = 0x0004; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 9f280a4a82806..df39568fe1274 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -22,92 +22,89 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, throw new IOException(SR.Format(SR.IO_FileExists_Name, destFullPath)); } - // Start the copy - (long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite, openDst: false); - try + // Start by locking, creating relevant file handles, and reading out file status info. + using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var fileStatus); + UnixFileMode filePermissions = SafeFileHandle.GetFileMode(fileStatus); + + // Read FileStatus of destination file to determine how to continue + int destError = Interop.Sys.Stat(destFullPath, out var destStat); + + // Interpret the error from stat + if (destError != 0) { - // Read FileStatus of destination file to determine how to continue - int destError = Interop.Sys.Stat(destFullPath, out var destStat); + // Some error, let's see what it is: + Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo(); - // Interpret the error from stat - if (destError != 0) + // Destination not existing is expected, so if this is the case we try clonefile, + // otherwise we got some other error that the fallback code can deal with. + if (error.Error == Interop.Error.ENOENT) { - // Some error, let's see what it is: - Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo(); - - if (error.Error == Interop.Error.ENOENT) - { - goto tryCloneFile; - } - else - { - goto tryFallback; - } + goto tryCloneFile; } else { - // Destination exists: - if (!overwrite) - { - // Throw an error if we're not overriding - throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); - } - if (startedCopyFile.fileDev != destStat.Dev) - { - // On different device - goto tryFallback; - } - else if (startedCopyFile.fileIno == destStat.Ino) - { - // Copying onto itself - throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); - } - else - { - goto tryDelete; - } + goto tryFallback; } - - // Try deleting destination: - tryDelete: + } + else + { + // Destination exists: + if (!overwrite) { - // Delete the destination. This should fail on directories. And update the mode. - // Get a lock to the dest file for compat reasons, and then delete it. - using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile.filePermissions, false); - File.Delete(destFullPath); + // Throw an error if we're not overriding + throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); } - - // Try clonefile: - tryCloneFile: + if (fileStatus.Dev != destStat.Dev) { - if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) - { - // Success - return; - } - - // Throw if the file already exists and we're not overwriting - if (Interop.Sys.GetLastError() == Interop.Error.EEXIST && !overwrite) - { - throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); - } + // On different device + goto tryFallback; + } + else if (fileStatus.Ino == destStat.Ino) + { + // Copying onto itself + throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); } + else + { + goto tryDelete; + } + } + + // Try deleting destination: + tryDelete: + { + // Delete the destination. This should fail on directories. And update the mode. + // Get a lock to the dest file for compat reasons, and then delete it. + using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, filePermissions, false); + File.Delete(destFullPath); + } - // Try fallback: - tryFallback: + // Try clonefile: + tryCloneFile: + { + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) { - // Open the dst handle - startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile.filePermissions, openNewFile: true); + // Success + return; + } - // Copy the file using the standard unix implementation - // dst! because dst is not null if OpenCopyFileDstHandle's openNewFile is true. - StandardCopyFile(startedCopyFile.src, startedCopyFile.dst!, startedCopyFile.fileLength); + // Throw if the file already exists and we're not overwriting + if (Interop.Sys.GetLastError() == Interop.Error.EEXIST && !overwrite) + { + throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); } } - finally + + // Try fallback: + tryFallback: { - startedCopyFile.src?.Dispose(); - startedCopyFile.dst?.Dispose(); + // Open the dst handle + // ! because OpenCopyFileDstHandle doesn't return null when openNewFile is true + using SafeFileHandle dst = OpenCopyFileDstHandle(destFullPath, overwrite, filePermissions, openNewFile: true)!; + + // Copy the file using the standard unix implementation. + // Note: this code needs to be kept in sync with the code in FileSystem.CopyFile.OtherUnix.cs. + StandardCopyFile(src, dst, fileStatus.Size); } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index 5bd69e487523a..fbe567be47837 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -7,20 +7,14 @@ internal static partial class FileSystem { public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - var (fileLength, _, _, _, src, dst) = StartCopyFile(sourceFullPath, destFullPath, overwrite); + // Open src and dest file handles. + // ! because OpenCopyFileDstHandle doesn't return null when openNewFile is true + using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var srcFileStatus); + using SafeFileHandle dst = OpenCopyFileDstHandle(destFullPath, overwrite, SafeFileHandle.GetFileMode(srcFileStatus), true)!; - try - { - // Copy the file using the standard unix implementation - // dst! because dst is not null if StartCopyFile's openDst is true (which is the default value) - StandardCopyFile(src, dst!, fileLength); - } - finally - { - // Dipose relevant file handles - src.Dispose(); - dst?.Dispose(); - } + // Copy the file in a way that works on all Unix Operating Systems. + // Note: the fallback code in FileSystem.CopyFile.OSX.cs needs to be kept in sync with this. + Interop.CheckIo(Interop.Sys.CopyFile(src, dst, srcFileStatus.Size)); } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 5b38b0c77c466..0dbc87ee8a29f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,33 +28,6 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - private static (long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite, bool openDst = true) - { - // The return value has SafeFileHandles, which are expected to be Disposed by the caller (unless this method throws) once the copy is complete. - // Begins 'CopyFile' by locking and creating the relevant file handles. - // If 'openDst' is false, it doesn't open the destination file handle, nor check anything to do with it (used in macOS implementation). - - (long fileLength, UnixFileMode filePermissions, long fileDev, long fileIno, SafeFileHandle src, SafeFileHandle? dst) startedCopyFile = default; - try - { - startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var fileStatus); - startedCopyFile.fileLength = fileStatus.Size; - startedCopyFile.filePermissions = SafeFileHandle.GetFileMode(fileStatus); - startedCopyFile.fileDev = fileStatus.Dev; - startedCopyFile.fileIno = fileStatus.Ino; - if (openDst) startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile.filePermissions, true); - } - catch - { - startedCopyFile.src?.Dispose(); - startedCopyFile.dst?.Dispose(); - throw; - } - - // Return the collection of information we have gotten back. - return startedCopyFile; - } - private static SafeFileHandle? OpenCopyFileDstHandle(string destFullPath, bool overwrite, UnixFileMode filePermissions, bool openNewFile) { // This function opens the 'dst' file handle for 'CopyFile', it is @@ -89,14 +62,6 @@ private static (long fileLength, UnixFileMode filePermissions, long fileDev, lon } } - private static void StandardCopyFile(SafeFileHandle src, SafeFileHandle dst, long fileLength) - { - // Copy the file in a way that works on all Unix Operating Systems. - // The 'src', 'dst', and 'fileLength' parameters should take the output from 'StartCopyFile'. - // 'src' and 'dst' should be disposed by the caller. - Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileLength)); - } - // CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs // The implementations on OSX-like Operating Systems attempts to clone the file first. public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); From eb97585618759f3d9bf9ecaa231523978da23750 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 16 May 2023 18:09:58 +1000 Subject: [PATCH 23/81] Fix compilation issues --- .../src/System/IO/FileSystem.CopyFile.OtherUnix.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index fbe567be47837..b5489fef12c5e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using Microsoft.Win32.SafeHandles; + namespace System.IO { internal static partial class FileSystem From 70c6e8b9a719ad83d7b83f710b80a0c64d4ea541 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 16 May 2023 19:06:46 +1000 Subject: [PATCH 24/81] Fix missed line in FileSystem.CopyFile.OSX.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Missed line in FileSystem.CopyFile.OSX.cs referencing the now-removed StandardCopyFile --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index df39568fe1274..95319a9e8a8dd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -104,7 +104,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // Copy the file using the standard unix implementation. // Note: this code needs to be kept in sync with the code in FileSystem.CopyFile.OtherUnix.cs. - StandardCopyFile(src, dst, fileStatus.Size); + Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileStatus.Size)); } } } From f866a3ddf0b81551e4fcde50826e4745d4754739 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 17 May 2023 07:42:27 +1000 Subject: [PATCH 25/81] Update src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs Co-authored-by: Stephen Toub --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 95319a9e8a8dd..0cd788fed59fd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -59,7 +59,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // On different device goto tryFallback; } - else if (fileStatus.Ino == destStat.Ino) + if (fileStatus.Ino == destStat.Ino) { // Copying onto itself throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); From bf676497a21508d67ade826bed11fbd7ba575efe Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 17 May 2023 07:42:47 +1000 Subject: [PATCH 26/81] Update src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs Co-authored-by: Stephen Toub --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 0cd788fed59fd..6286476804190 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -64,10 +64,6 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // Copying onto itself throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); } - else - { - goto tryDelete; - } } // Try deleting destination: From ff108dbffdc4241cfc5f21505fadb9280c5ca1b9 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 17 May 2023 07:43:45 +1000 Subject: [PATCH 27/81] Update src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs Co-authored-by: Stephen Toub --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 6286476804190..36a5b92e8dff2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -33,11 +33,11 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, if (destError != 0) { // Some error, let's see what it is: - Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo(); + Interop.Error error = Interop.Sys.GetLastError(); // Destination not existing is expected, so if this is the case we try clonefile, // otherwise we got some other error that the fallback code can deal with. - if (error.Error == Interop.Error.ENOENT) + if (error == Interop.Error.ENOENT) { goto tryCloneFile; } From 69071eb83c86609dedbc9bd3ff0c536884a23adc Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 17 May 2023 07:44:13 +1000 Subject: [PATCH 28/81] Update src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs Co-authored-by: Stephen Toub --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 36a5b92e8dff2..62f1d8e1583d7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -14,12 +14,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // Fail fast for blatantly copying onto self if (sourceFullPath == destFullPath) { - if (overwrite) - { - throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); - } - - throw new IOException(SR.Format(SR.IO_FileExists_Name, destFullPath)); + throw new IOException(SR.Format(overwrite ? SR.IO_SharingViolation_File : SR.IO_FileExists_Name, destFullPath)); } // Start by locking, creating relevant file handles, and reading out file status info. From 20c42f905fa14b89c231dac23b5d1dd04ccc43be Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 17 May 2023 07:48:48 +1000 Subject: [PATCH 29/81] Replace var with actual type as per feedback --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 62f1d8e1583d7..43a281249d16e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -18,11 +18,11 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } // Start by locking, creating relevant file handles, and reading out file status info. - using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var fileStatus); + using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus fileStatus); UnixFileMode filePermissions = SafeFileHandle.GetFileMode(fileStatus); // Read FileStatus of destination file to determine how to continue - int destError = Interop.Sys.Stat(destFullPath, out var destStat); + int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); // Interpret the error from stat if (destError != 0) From 1280a8ac507bc66df734d0d24187da042506d842 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 17 May 2023 07:53:07 +1000 Subject: [PATCH 30/81] Update comment for clarity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Explain more clearly why we open a lock to the destination file before deleting it --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 43a281249d16e..b3454c1c03c7b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -65,7 +65,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, tryDelete: { // Delete the destination. This should fail on directories. And update the mode. - // Get a lock to the dest file for compat reasons, and then delete it. + // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, filePermissions, false); File.Delete(destFullPath); } From f33ae9c58591da3c9b1ee16207da74c664778ae5 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 17 May 2023 08:07:34 +1000 Subject: [PATCH 31/81] Implement feedback to remove OpenCopyFileDstHandle --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 31 ++++++++++++++--- .../IO/FileSystem.CopyFile.OtherUnix.cs | 22 ++++++++++-- .../src/System/IO/FileSystem.Unix.cs | 34 ------------------- 3 files changed, 46 insertions(+), 41 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index b3454c1c03c7b..f63a3dfee26b7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -66,8 +66,17 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, { // Delete the destination. This should fail on directories. And update the mode. // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. - using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, filePermissions, false); - File.Delete(destFullPath); + SafeFileHandle? dstHandle = null; + try + { + using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, + FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions); + File.Delete(destFullPath); + } + catch (FileNotFoundException) + { + // We don't want to throw if it's just the file not existing, since we're trying to delete it. + } } // Try clonefile: @@ -90,8 +99,22 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, tryFallback: { // Open the dst handle - // ! because OpenCopyFileDstHandle doesn't return null when openNewFile is true - using SafeFileHandle dst = OpenCopyFileDstHandle(destFullPath, overwrite, filePermissions, openNewFile: true)!; + // Note: this code needs to be kept in sync with the code in FileSystem.CopyFile.OtherUnix.cs. + using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, + CreateOpenException); + + // Exception handler for SafeFileHandle.Open failing. + static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) + { + // If the destination path points to a directory, we throw to match Windows behaviour. + if (error.Error == Interop.Error.EEXIST && DirectoryExists(path)) + { + return new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); + } + + return null; // Let SafeFileHandle create the exception for this error. + } // Copy the file using the standard unix implementation. // Note: this code needs to be kept in sync with the code in FileSystem.CopyFile.OtherUnix.cs. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index b5489fef12c5e..caa57cb1fefca 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -9,10 +9,26 @@ internal static partial class FileSystem { public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - // Open src and dest file handles. - // ! because OpenCopyFileDstHandle doesn't return null when openNewFile is true + // Open the src file handle. using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var srcFileStatus); - using SafeFileHandle dst = OpenCopyFileDstHandle(destFullPath, overwrite, SafeFileHandle.GetFileMode(srcFileStatus), true)!; + + // Open the dst file handle. + // Note: the code in FileSystem.CopyFile.OSX.cs needs to be kept in sync with this section. + using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, + CreateOpenException); + + // Exception handler for SafeFileHandle.Open failing. + static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) + { + // If the destination path points to a directory, we throw to match Windows behaviour. + if (error.Error == Interop.Error.EEXIST && DirectoryExists(path)) + { + return new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); + } + + return null; // Let SafeFileHandle create the exception for this error. + } // Copy the file in a way that works on all Unix Operating Systems. // Note: the fallback code in FileSystem.CopyFile.OSX.cs needs to be kept in sync with this. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 0dbc87ee8a29f..cb5864f0d66c0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,40 +28,6 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - private static SafeFileHandle? OpenCopyFileDstHandle(string destFullPath, bool overwrite, UnixFileMode filePermissions, bool openNewFile) - { - // This function opens the 'dst' file handle for 'CopyFile', it is - // split out since the logic on OSX-like OSes is a bit different. - // 'openNewFile' = false is used when we want to try to find the file only. - if (!openNewFile) - { - try - { - return SafeFileHandle.Open(destFullPath, FileMode.Open, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, - CreateOpenException); - } - catch (FileNotFoundException) - { - return null; - } - } - return SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, - CreateOpenException); - - static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) - { - // If the destination path points to a directory, we throw to match Windows behaviour. - if (error.Error == Interop.Error.EEXIST && DirectoryExists(path)) - { - return new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); - } - - return null; // Let SafeFileHandle create the exception for this error. - } - } - // CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs // The implementations on OSX-like Operating Systems attempts to clone the file first. public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); From 068a8aa3f3820679f05bf1ff19dcfd02ddf65e97 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 17 May 2023 08:43:32 +1000 Subject: [PATCH 32/81] Fix compilation errors --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index f63a3dfee26b7..4833de3ad9d98 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -62,11 +62,9 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } // Try deleting destination: - tryDelete: { // Delete the destination. This should fail on directories. And update the mode. // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. - SafeFileHandle? dstHandle = null; try { using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, From 2c48aad50859cf0d3bd6ad5bcb056cf1eaa32aff Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 08:27:38 +1000 Subject: [PATCH 33/81] Remove some comments as per feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Remove comments saying to keep code in sync as per feedback --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 -- .../src/System/IO/FileSystem.CopyFile.OtherUnix.cs | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 4833de3ad9d98..5a43967f4d03a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -97,7 +97,6 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, tryFallback: { // Open the dst handle - // Note: this code needs to be kept in sync with the code in FileSystem.CopyFile.OtherUnix.cs. using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, CreateOpenException); @@ -115,7 +114,6 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } // Copy the file using the standard unix implementation. - // Note: this code needs to be kept in sync with the code in FileSystem.CopyFile.OtherUnix.cs. Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileStatus.Size)); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs index caa57cb1fefca..3e994642962cc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs @@ -13,7 +13,6 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var srcFileStatus); // Open the dst file handle. - // Note: the code in FileSystem.CopyFile.OSX.cs needs to be kept in sync with this section. using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, CreateOpenException); @@ -31,7 +30,6 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } // Copy the file in a way that works on all Unix Operating Systems. - // Note: the fallback code in FileSystem.CopyFile.OSX.cs needs to be kept in sync with this. Interop.CheckIo(Interop.Sys.CopyFile(src, dst, srcFileStatus.Size)); } } From ef840101595fe0a896b1bd94f67c8db3260c4339 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 08:41:52 +1000 Subject: [PATCH 34/81] Changes from feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Update some comments to make things clear • Shorten comment and code that interprets the error as per feedback --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index 5a43967f4d03a..b2bcf6fecd596 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -27,12 +27,9 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // Interpret the error from stat if (destError != 0) { - // Some error, let's see what it is: - Interop.Error error = Interop.Sys.GetLastError(); - - // Destination not existing is expected, so if this is the case we try clonefile, - // otherwise we got some other error that the fallback code can deal with. - if (error == Interop.Error.ENOENT) + // stat failed. If the destination doesn't exist (which is the expected + // cause), try clonefile; otherwise, fall back to a normal copy. + if (Interop.Sys.GetLastError() == Interop.Error.ENOENT) { goto tryCloneFile; } @@ -86,7 +83,8 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, return; } - // Throw if the file already exists and we're not overwriting + // Clonefile fails if the destination exists (which we try to avoid), so throw if error is + // the destination exists and we're not overwriting, otherwise we will go to fallback path. if (Interop.Sys.GetLastError() == Interop.Error.EEXIST && !overwrite) { throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); From 8924afe2cd0f80e5c428f329d969b2bbe23dfcc2 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 08:42:28 +1000 Subject: [PATCH 35/81] Remove redundant line as per feedback --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index b2bcf6fecd596..a2285a89cf2ff 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -23,8 +23,6 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // Read FileStatus of destination file to determine how to continue int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); - - // Interpret the error from stat if (destError != 0) { // stat failed. If the destination doesn't exist (which is the expected From 1649d43ab9a8f7c58599de0096c39f9ad1a9bba7 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 08:44:59 +1000 Subject: [PATCH 36/81] Remove old comment --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index a2285a89cf2ff..a7746dfa3b335 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -58,7 +58,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // Try deleting destination: { - // Delete the destination. This should fail on directories. And update the mode. + // Delete the destination. This should fail on directories. // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. try { From a82767b340eb4b0cfc141f2f0f72f60eafebb8dd Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 17:43:09 +1000 Subject: [PATCH 37/81] Implement some feedback --- .../src/System/IO/FileSystem.CopyFile.OSX.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs index a7746dfa3b335..dcee5d19adea5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs @@ -14,6 +14,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // Fail fast for blatantly copying onto self if (sourceFullPath == destFullPath) { + if (!File.Exists(sourceFullPath)) throw new FileNotFoundException(SR.Format(SR.IO_FileNotFound_FileName, sourceFullPath), sourceFullPath); throw new IOException(SR.Format(overwrite ? SR.IO_SharingViolation_File : SR.IO_FileExists_Name, destFullPath)); } @@ -94,7 +95,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, { // Open the dst handle using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: filePermissions, CreateOpenException); // Exception handler for SafeFileHandle.Open failing. From c0c7e33e772f77fd9061e253dc4465a050027ff5 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 18:01:38 +1000 Subject: [PATCH 38/81] Implement suggestion that cuts down on duplication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Implement suggestion from @tmds that greatly cuts down on code duplication :) --- .../IO/FileSystem.CopyFile.OtherUnix.cs | 36 -------------- ....OSX.cs => FileSystem.TryCloneFile.OSX.cs} | 49 ++++--------------- .../IO/FileSystem.TryCloneFile.OtherUnix.cs | 16 ++++++ .../src/System/IO/FileSystem.Unix.cs | 37 ++++++++++++-- 4 files changed, 60 insertions(+), 78 deletions(-) delete mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs rename src/libraries/System.Private.CoreLib/src/System/IO/{FileSystem.CopyFile.OSX.cs => FileSystem.TryCloneFile.OSX.cs} (58%) create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs deleted file mode 100644 index 3e994642962cc..0000000000000 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs +++ /dev/null @@ -1,36 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using Microsoft.Win32.SafeHandles; - -namespace System.IO -{ - internal static partial class FileSystem - { - public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) - { - // Open the src file handle. - using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out var srcFileStatus); - - // Open the dst file handle. - using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, - CreateOpenException); - - // Exception handler for SafeFileHandle.Open failing. - static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) - { - // If the destination path points to a directory, we throw to match Windows behaviour. - if (error.Error == Interop.Error.EEXIST && DirectoryExists(path)) - { - return new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); - } - - return null; // Let SafeFileHandle create the exception for this error. - } - - // Copy the file in a way that works on all Unix Operating Systems. - Interop.CheckIo(Interop.Sys.CopyFile(src, dst, srcFileStatus.Size)); - } - } -} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs similarity index 58% rename from src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs rename to src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index dcee5d19adea5..368f3d59085a4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -9,18 +9,10 @@ namespace System.IO { internal static partial class FileSystem { - public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { - // Fail fast for blatantly copying onto self - if (sourceFullPath == destFullPath) - { - if (!File.Exists(sourceFullPath)) throw new FileNotFoundException(SR.Format(SR.IO_FileNotFound_FileName, sourceFullPath), sourceFullPath); - throw new IOException(SR.Format(overwrite ? SR.IO_SharingViolation_File : SR.IO_FileExists_Name, destFullPath)); - } - - // Start by locking, creating relevant file handles, and reading out file status info. - using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus fileStatus); - UnixFileMode filePermissions = SafeFileHandle.GetFileMode(fileStatus); + // Get the file permissions. + UnixFileMode filePermissions = SafeFileHandle.GetFileMode(srcStat); // Read FileStatus of destination file to determine how to continue int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); @@ -34,7 +26,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } else { - goto tryFallback; + return false; } } else @@ -45,12 +37,12 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, // Throw an error if we're not overriding throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); } - if (fileStatus.Dev != destStat.Dev) + if (srcStat.Dev != destStat.Dev) { // On different device - goto tryFallback; + return false; } - if (fileStatus.Ino == destStat.Ino) + if (srcStat.Ino == destStat.Ino) { // Copying onto itself throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); @@ -79,7 +71,7 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) { // Success - return; + return true; } // Clonefile fails if the destination exists (which we try to avoid), so throw if error is @@ -90,29 +82,8 @@ public static partial void CopyFile(string sourceFullPath, string destFullPath, } } - // Try fallback: - tryFallback: - { - // Open the dst handle - using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: filePermissions, - CreateOpenException); - - // Exception handler for SafeFileHandle.Open failing. - static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) - { - // If the destination path points to a directory, we throw to match Windows behaviour. - if (error.Error == Interop.Error.EEXIST && DirectoryExists(path)) - { - return new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); - } - - return null; // Let SafeFileHandle create the exception for this error. - } - - // Copy the file using the standard unix implementation. - Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileStatus.Size)); - } + // Otherwise we want to go the the fallback + return false; } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs new file mode 100644 index 0000000000000..79803144291fa --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.Win32.SafeHandles; + +namespace System.IO +{ + internal static partial class FileSystem + { + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) + { + // No such functionality is available on unix OSes (other than OSX-like ones). + return false; + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index cb5864f0d66c0..5c840621a459d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,9 +28,40 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - // CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs - // The implementations on OSX-like Operating Systems attempts to clone the file first. - public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite); + // TryCloneFile is defined in either FileSystem.TryCloneFile.OSX.cs or FileSystem.TryCloneFile.OtherUnix.cs. + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite); + + public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) + { + // Open the src file handle. + using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus srcFileStatus); + + // Try to clone the file first. + if (TryCloneFile(sourceFullPath, in srcFileStatus, destFullPath, overwrite)) + { + return; + } + + // Open the dst file handle. + using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, + CreateOpenException); + + // Exception handler for SafeFileHandle.Open failing. + static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) + { + // If the destination path points to a directory, we throw to match Windows behaviour. + if (error.Error == Interop.Error.EEXIST && DirectoryExists(path)) + { + return new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); + } + + return null; // Let SafeFileHandle create the exception for this error. + } + + // Copy the file. + Interop.CheckIo(Interop.Sys.CopyFile(src, dst, srcFileStatus.Size)); + } #pragma warning disable IDE0060 public static void Encrypt(string path) From 18b8c2ef30bf4affebf0dae953817d904d475cca Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 18:04:56 +1000 Subject: [PATCH 39/81] Remove unnecessary using --- .../src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs index 79803144291fa..cd129c321eb61 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Microsoft.Win32.SafeHandles; - namespace System.IO { internal static partial class FileSystem From 00b68ed4c259bf681cf442ef9fcfc67a60656be4 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 18:08:33 +1000 Subject: [PATCH 40/81] Remove some other unnecessary usings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • This time in FileSystem.TryCloneFile.OSX.cs --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 368f3d59085a4..771ba3c898c3f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics; -using System.Runtime.InteropServices; using Microsoft.Win32.SafeHandles; namespace System.IO From afa73ca50038bdcbb98f83de9ab7f6d3ff4abbca Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 18:21:51 +1000 Subject: [PATCH 41/81] Fix filePermissions missing as per feedback --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 5 +---- .../src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs | 2 +- .../src/System/IO/FileSystem.Unix.cs | 9 +++++---- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 771ba3c898c3f..d088caa27df14 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -7,11 +7,8 @@ namespace System.IO { internal static partial class FileSystem { - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, UnixFileMode filePermissions, string destFullPath, bool overwrite) { - // Get the file permissions. - UnixFileMode filePermissions = SafeFileHandle.GetFileMode(srcStat); - // Read FileStatus of destination file to determine how to continue int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); if (destError != 0) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs index cd129c321eb61..e2c868ec3f390 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -5,7 +5,7 @@ namespace System.IO { internal static partial class FileSystem { - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, UnixFileMode filePermissions, string destFullPath, bool overwrite) { // No such functionality is available on unix OSes (other than OSX-like ones). return false; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 5c840621a459d..c9b44b5444134 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -29,22 +29,23 @@ internal static partial class FileSystem UnixFileMode.OtherExecute; // TryCloneFile is defined in either FileSystem.TryCloneFile.OSX.cs or FileSystem.TryCloneFile.OtherUnix.cs. - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite); + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, UnixFileMode filePermissions, string destFullPath, bool overwrite); public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - // Open the src file handle. + // Open the src file handle, and read the file permissions. using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus srcFileStatus); + UnixFileMode filePermissions = SafeFileHandle.GetFileMode(srcStat); // Try to clone the file first. - if (TryCloneFile(sourceFullPath, in srcFileStatus, destFullPath, overwrite)) + if (TryCloneFile(sourceFullPath, in srcFileStatus, filePermissions, destFullPath, overwrite)) { return; } // Open the dst file handle. using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: null, + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: filePermissions, CreateOpenException); // Exception handler for SafeFileHandle.Open failing. From a684785644c7b3084c23ccaf87569e40c6596a0f Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 18:23:57 +1000 Subject: [PATCH 42/81] Fix compilation issues from files having wrong name in projitems --- .../src/System.Private.CoreLib.Shared.projitems | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index a628eef64b7e8..e193fb0adaafb 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -2373,7 +2373,7 @@ - + @@ -2428,7 +2428,7 @@ Common\Interop\OSX\Interop.libc.cs - + From 61c0576c07b88f0cc7bfa10079bc50732e1c3360 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 18:26:40 +1000 Subject: [PATCH 43/81] Make the CopyFile implementation more similar to how it was before --- .../src/System/IO/FileSystem.Unix.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index c9b44b5444134..527d1b5f8345b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -43,10 +43,11 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove return; } - // Open the dst file handle. + // Open the dst file handle, and copy the file. using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, - FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, unixCreateMode: filePermissions, - CreateOpenException); + FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, + CreateOpenException); + Interop.CheckIo(Interop.Sys.CopyFile(src, dst, srcFileStatus.Size)); // Exception handler for SafeFileHandle.Open failing. static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) @@ -59,9 +60,6 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove return null; // Let SafeFileHandle create the exception for this error. } - - // Copy the file. - Interop.CheckIo(Interop.Sys.CopyFile(src, dst, srcFileStatus.Size)); } #pragma warning disable IDE0060 From 7c6c80b1a0e15771837315146ca908e30bff6222 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Thu, 18 May 2023 18:43:53 +1000 Subject: [PATCH 44/81] Fix copied code compilation issue --- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 527d1b5f8345b..1f9dc08011672 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -35,7 +35,7 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove { // Open the src file handle, and read the file permissions. using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus srcFileStatus); - UnixFileMode filePermissions = SafeFileHandle.GetFileMode(srcStat); + UnixFileMode filePermissions = SafeFileHandle.GetFileMode(srcFileStatus); // Try to clone the file first. if (TryCloneFile(sourceFullPath, in srcFileStatus, filePermissions, destFullPath, overwrite)) From 9cdb4b31188c63bc3437dfd8b0b200c9a9fbc54e Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 23 May 2023 11:13:25 +1000 Subject: [PATCH 45/81] Remove filePermissions parameter as per feedback --- .../src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs | 4 ---- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 5 ++++- .../src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs | 2 +- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 4 ++-- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index d3f6c36b153de..c09b5bd904264 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs @@ -174,10 +174,6 @@ internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options return handle; } - // Used by callers of OpenReadOnly. - internal static UnixFileMode GetFileMode(Interop.Sys.FileStatus status) => - ((UnixFileMode)status.Mode) & PermissionMask; - internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode? unixCreateMode = null, Func? createOpenException = null) { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index d088caa27df14..351b112d75e04 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -7,8 +7,11 @@ namespace System.IO { internal static partial class FileSystem { - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, UnixFileMode filePermissions, string destFullPath, bool overwrite) + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { + //get the file permissions for source file + UnixFileMode filePermissions = srcStat.Mode & SafeFileHandle.PermissionMask; + // Read FileStatus of destination file to determine how to continue int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); if (destError != 0) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs index e2c868ec3f390..cd129c321eb61 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -5,7 +5,7 @@ namespace System.IO { internal static partial class FileSystem { - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, UnixFileMode filePermissions, string destFullPath, bool overwrite) + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { // No such functionality is available on unix OSes (other than OSX-like ones). return false; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 1f9dc08011672..6c1624668e7ca 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -29,13 +29,13 @@ internal static partial class FileSystem UnixFileMode.OtherExecute; // TryCloneFile is defined in either FileSystem.TryCloneFile.OSX.cs or FileSystem.TryCloneFile.OtherUnix.cs. - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, UnixFileMode filePermissions, string destFullPath, bool overwrite); + private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite); public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { // Open the src file handle, and read the file permissions. using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus srcFileStatus); - UnixFileMode filePermissions = SafeFileHandle.GetFileMode(srcFileStatus); + UnixFileMode filePermissions = srcFileStatus.Mode & SafeFileHandle.PermissionMask; // Try to clone the file first. if (TryCloneFile(sourceFullPath, in srcFileStatus, filePermissions, destFullPath, overwrite)) From 2dc08e4efd3fe0398013b052c57063105c6447e3 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 23 May 2023 11:14:42 +1000 Subject: [PATCH 46/81] Remove isReadOnly as per feedback --- .../Win32/SafeHandles/SafeFileHandle.Unix.cs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index c09b5bd904264..5755ae3242180 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs @@ -169,8 +169,7 @@ public override bool IsInvalid // This information is retrieved from the 'stat' syscall that must be performed to ensure the path is not a directory. internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options, out Interop.Sys.FileStatus status) { - SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out status, out bool isReadOnly); - Debug.Assert(isReadOnly); + SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out status); return handle; } @@ -181,7 +180,7 @@ internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess a } private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode openPermissions, - out Interop.Sys.FileStatus status, out bool isReadOnly, + out Interop.Sys.FileStatus status, Func? createOpenException = null) { // Translate the arguments into arguments for an open call. @@ -196,7 +195,7 @@ private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess ac // When Init return false, the path has changed to another file entry, and // we need to re-open the path to reflect that. - if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize, out status, out isReadOnly)) + if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize, out status)) { return safeFileHandle; } @@ -294,12 +293,11 @@ private static Interop.Sys.OpenFlags PreOpenConfigurationFromOptions(FileMode mo } private bool Init(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, - out Interop.Sys.FileStatus status, out bool isReadOnly) + out Interop.Sys.FileStatus status) { bool statusHasValue = false; status = default; - isReadOnly = false; // Make sure our handle is not a directory. // We can omit the check when write access is requested. open will have failed with EISDIR. @@ -321,8 +319,6 @@ private bool Init(string path, FileMode mode, FileAccess access, FileShare share _canSeek = NullableBool.True; Debug.Assert(Interop.Sys.LSeek(this, 0, Interop.Sys.SeekWhence.SEEK_CUR) >= 0); } - - isReadOnly = true; } IsAsync = (options & FileOptions.Asynchronous) != 0; From 02cfaf647151dfddc94ee5f52914f114701e8f36 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 23 May 2023 11:18:20 +1000 Subject: [PATCH 47/81] Fix whitespace --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 351b112d75e04..02c3b7493bb79 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -9,7 +9,7 @@ internal static partial class FileSystem { private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { - //get the file permissions for source file + // get the file permissions for source file UnixFileMode filePermissions = srcStat.Mode & SafeFileHandle.PermissionMask; // Read FileStatus of destination file to determine how to continue From 1fc02f7f5d20a09d17c1cc63f3d882cc17143eb6 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 23 May 2023 11:30:39 +1000 Subject: [PATCH 48/81] Fix compilation errors --- .../src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs | 4 ++-- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index 5755ae3242180..5b00a5cdef9c9 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs @@ -11,7 +11,7 @@ namespace Microsoft.Win32.SafeHandles { public sealed partial class SafeFileHandle : SafeHandleZeroOrMinusOneIsInvalid { - private const UnixFileMode PermissionMask = + internal const UnixFileMode PermissionMask = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | @@ -176,7 +176,7 @@ internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode? unixCreateMode = null, Func? createOpenException = null) { - return Open(fullPath, mode, access, share, options, preallocationSize, unixCreateMode ?? DefaultCreateMode, out _, out _, createOpenException); + return Open(fullPath, mode, access, share, options, preallocationSize, unixCreateMode ?? DefaultCreateMode, out _, createOpenException); } private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode openPermissions, diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 6c1624668e7ca..e881c23d209e4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -35,15 +35,15 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove { // Open the src file handle, and read the file permissions. using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus srcFileStatus); - UnixFileMode filePermissions = srcFileStatus.Mode & SafeFileHandle.PermissionMask; // Try to clone the file first. - if (TryCloneFile(sourceFullPath, in srcFileStatus, filePermissions, destFullPath, overwrite)) + if (TryCloneFile(sourceFullPath, in srcFileStatus, destFullPath, overwrite)) { return; } // Open the dst file handle, and copy the file. + UnixFileMode filePermissions = srcFileStatus.Mode & SafeFileHandle.PermissionMask; using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, CreateOpenException); From 71bc96e867c021c20b59b3aa8b1986d854e25866 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 23 May 2023 11:40:10 +1000 Subject: [PATCH 49/81] Fix more compilation issues --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 02c3b7493bb79..ffedf654dae73 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -10,7 +10,7 @@ internal static partial class FileSystem private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { // get the file permissions for source file - UnixFileMode filePermissions = srcStat.Mode & SafeFileHandle.PermissionMask; + UnixFileMode filePermissions = (UnixFileMode)srcStat.Mode & SafeFileHandle.PermissionMask; // Read FileStatus of destination file to determine how to continue int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index e881c23d209e4..2c714bac8ab5c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -43,7 +43,7 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove } // Open the dst file handle, and copy the file. - UnixFileMode filePermissions = srcFileStatus.Mode & SafeFileHandle.PermissionMask; + UnixFileMode filePermissions = (UnixFileMode)srcFileStatus.Mode & SafeFileHandle.PermissionMask; using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, CreateOpenException); From aa76f240230bdc56e4791993557b32e8370a3f85 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 23 May 2023 16:46:32 +1000 Subject: [PATCH 50/81] Try clonefile immediately as per feedback --- .../System/IO/FileSystem.TryCloneFile.OSX.cs | 116 +++++++++--------- 1 file changed, 60 insertions(+), 56 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index ffedf654dae73..e087f07047679 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using Microsoft.Win32.SafeHandles; +using System.Diagnostics; namespace System.IO { @@ -9,78 +10,81 @@ internal static partial class FileSystem { private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { - // get the file permissions for source file - UnixFileMode filePermissions = (UnixFileMode)srcStat.Mode & SafeFileHandle.PermissionMask; - - // Read FileStatus of destination file to determine how to continue - int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); - if (destError != 0) + // Try to clone the file immediately, this will only succeed if the destination doesn't exist, + // so we don't worry about locking for this one. + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) { - // stat failed. If the destination doesn't exist (which is the expected - // cause), try clonefile; otherwise, fall back to a normal copy. - if (Interop.Sys.GetLastError() == Interop.Error.ENOENT) - { - goto tryCloneFile; - } - else - { - return false; - } - } - else - { - // Destination exists: - if (!overwrite) - { - // Throw an error if we're not overriding - throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); - } - if (srcStat.Dev != destStat.Dev) - { - // On different device - return false; - } - if (srcStat.Ino == destStat.Ino) - { - // Copying onto itself - throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); - } + // Success + return true; } + Interop.Error error = Interop.Sys.GetLastError(); - // Try deleting destination: + // Try to delete the destination file if we're overwriting. + if (error == Interop.Error.EEXIST && overwrite) { - // Delete the destination. This should fail on directories. - // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. - try + // Get the file permissions for source file + UnixFileMode filePermissions = srcStat.Mode & SafeFileHandle.PermissionMask; + + // Read FileStatus of destination file to determine how to continue (we need to check that + // destination doesn't point to the same file as the source file so we can fail appropriately) + int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); + if (destError != 0) { - using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, - FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions); - File.Delete(destFullPath); + // stat failed. If the destination doesn't exist anymore, + // try clonefile; otherwise, fall back to a normal copy. + if (Interop.Sys.GetLastError() == Interop.Error.ENOENT) + { + goto tryCloneFile; + } + else + { + return false; + } } - catch (FileNotFoundException) + else { - // We don't want to throw if it's just the file not existing, since we're trying to delete it. + // Destination exists: + if (srcStat.Dev != destStat.Dev) + { + // On different device, so fall back to normal copy + return false; + } + if (srcStat.Ino == destStat.Ino) + { + // Copying onto itself + throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); + } } - } - // Try clonefile: - tryCloneFile: - { - if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) + // Try deleting destination: { - // Success - return true; + // Delete the destination. This should fail on directories. + // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. + try + { + using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, + FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions); + File.Delete(destFullPath); + } + catch (FileNotFoundException) + { + // We don't want to throw if it's just the file not existing, since we're trying to delete it. + } } - // Clonefile fails if the destination exists (which we try to avoid), so throw if error is - // the destination exists and we're not overwriting, otherwise we will go to fallback path. - if (Interop.Sys.GetLastError() == Interop.Error.EEXIST && !overwrite) + // Try clonefile: + tryCloneFile: { - throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.EEXIST)); + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) + { + // Success + return true; + } + error = Interop.Sys.GetLastError(); } } - // Otherwise we want to go the the fallback + // Fall back to regular copy return false; } } From 3945a9ff50b643fd105d0df800b95c8b957c58ca Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 23 May 2023 16:51:53 +1000 Subject: [PATCH 51/81] Add error handling to it, instead of always falling back --- .../System/IO/FileSystem.TryCloneFile.OSX.cs | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index e087f07047679..afca2f2cfc6da 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -84,8 +84,25 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F } } - // Fall back to regular copy - return false; + // Check if it's not supported, or if they're on different filesystems. + if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV) + { + // Fall back to normal copy + return false; + } + + // Check we didn't get an error due to 'invalid flags' (which should never happen) + Debug.Assert(error != Interop.Error.EINVAL); + if (error == Interop.Error.EINVAL) + { + // If we do somehow get here in release, we probably don't want + // copying to completely fail, so fall back to regular copy. + return false; + } + + // Throw the appropriate exception + Debug.Assert(error != Interop.Error.SUCCESS); // We shouldn't fail with success + throw Interop.GetExceptionForIoErrno(error.Info()); } } } From 3457dccf5165216cd58b7451337f415a87a6bf43 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 23 May 2023 17:39:24 +1000 Subject: [PATCH 52/81] Add missing cast --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index afca2f2cfc6da..1b885716707bf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -23,7 +23,7 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F if (error == Interop.Error.EEXIST && overwrite) { // Get the file permissions for source file - UnixFileMode filePermissions = srcStat.Mode & SafeFileHandle.PermissionMask; + UnixFileMode filePermissions = (UnixFileMode)srcStat.Mode & SafeFileHandle.PermissionMask; // Read FileStatus of destination file to determine how to continue (we need to check that // destination doesn't point to the same file as the source file so we can fail appropriately) From 788216ed461716a8afffe80d05b6264cb17b2a8f Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 02:29:46 +1000 Subject: [PATCH 53/81] Remove unneeded filePermissions specification --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 1b885716707bf..86c7813604f04 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -22,9 +22,6 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F // Try to delete the destination file if we're overwriting. if (error == Interop.Error.EEXIST && overwrite) { - // Get the file permissions for source file - UnixFileMode filePermissions = (UnixFileMode)srcStat.Mode & SafeFileHandle.PermissionMask; - // Read FileStatus of destination file to determine how to continue (we need to check that // destination doesn't point to the same file as the source file so we can fail appropriately) int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); @@ -63,7 +60,7 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F try { using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, - FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions); + FileShare.None, FileOptions.None, preallocationSize: 0); File.Delete(destFullPath); } catch (FileNotFoundException) From 233217a873c4b3f04d3a00e380a63737b16257bd Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 02:32:11 +1000 Subject: [PATCH 54/81] Change indentation of delete section --- .../System/IO/FileSystem.TryCloneFile.OSX.cs | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 86c7813604f04..1226a140d07fd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -54,19 +54,17 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F } // Try deleting destination: + // Delete the destination. This should fail on directories. + // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. + try { - // Delete the destination. This should fail on directories. - // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. - try - { - using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, - FileShare.None, FileOptions.None, preallocationSize: 0); - File.Delete(destFullPath); - } - catch (FileNotFoundException) - { - // We don't want to throw if it's just the file not existing, since we're trying to delete it. - } + using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, + FileShare.None, FileOptions.None, preallocationSize: 0); + File.Delete(destFullPath); + } + catch (FileNotFoundException) + { + // We don't want to throw if it's just the file not existing, since we're trying to delete it. } // Try clonefile: From 20caacd8fa506121ed4d251028a4eabaec667a62 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 02:32:24 +1000 Subject: [PATCH 55/81] Temporarily comment out the stat section --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 1226a140d07fd..4539010132f86 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -22,6 +22,7 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F // Try to delete the destination file if we're overwriting. if (error == Interop.Error.EEXIST && overwrite) { + /* // Read FileStatus of destination file to determine how to continue (we need to check that // destination doesn't point to the same file as the source file so we can fail appropriately) int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); @@ -52,6 +53,7 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); } } + */ // Try deleting destination: // Delete the destination. This should fail on directories. From 6d404672931dfe3ce338cadd1bb688d9f42a435a Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 02:34:32 +1000 Subject: [PATCH 56/81] Remove some unneeded indentation --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 4539010132f86..5404d26810348 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -71,14 +71,12 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F // Try clonefile: tryCloneFile: + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) { - if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) - { - // Success - return true; - } - error = Interop.Sys.GetLastError(); + // Success + return true; } + error = Interop.Sys.GetLastError(); } // Check if it's not supported, or if they're on different filesystems. From 705aa4f88910d0249290280cc943374bbd224285 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 02:36:23 +1000 Subject: [PATCH 57/81] Remove special casing of EINVAL --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 5404d26810348..fcf4029f332a2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -86,15 +86,6 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F return false; } - // Check we didn't get an error due to 'invalid flags' (which should never happen) - Debug.Assert(error != Interop.Error.EINVAL); - if (error == Interop.Error.EINVAL) - { - // If we do somehow get here in release, we probably don't want - // copying to completely fail, so fall back to regular copy. - return false; - } - // Throw the appropriate exception Debug.Assert(error != Interop.Error.SUCCESS); // We shouldn't fail with success throw Interop.GetExceptionForIoErrno(error.Info()); From a5de06032a846afb76f9424709cfa5e6f7cca58c Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 02:38:51 +1000 Subject: [PATCH 58/81] Remove section which does stat as it's unneeded --- .../System/IO/FileSystem.TryCloneFile.OSX.cs | 35 ------------------- 1 file changed, 35 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index fcf4029f332a2..d5786c88dc614 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -22,40 +22,6 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F // Try to delete the destination file if we're overwriting. if (error == Interop.Error.EEXIST && overwrite) { - /* - // Read FileStatus of destination file to determine how to continue (we need to check that - // destination doesn't point to the same file as the source file so we can fail appropriately) - int destError = Interop.Sys.Stat(destFullPath, out Interop.Sys.FileStatus destStat); - if (destError != 0) - { - // stat failed. If the destination doesn't exist anymore, - // try clonefile; otherwise, fall back to a normal copy. - if (Interop.Sys.GetLastError() == Interop.Error.ENOENT) - { - goto tryCloneFile; - } - else - { - return false; - } - } - else - { - // Destination exists: - if (srcStat.Dev != destStat.Dev) - { - // On different device, so fall back to normal copy - return false; - } - if (srcStat.Ino == destStat.Ino) - { - // Copying onto itself - throw new IOException(SR.Format(SR.IO_SharingViolation_File, destFullPath)); - } - } - */ - - // Try deleting destination: // Delete the destination. This should fail on directories. // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. try @@ -70,7 +36,6 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F } // Try clonefile: - tryCloneFile: if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) { // Success From 7221560567da121a63ae825cc28b7e82521b306f Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 02:45:07 +1000 Subject: [PATCH 59/81] Improve some of the comments. --- .../System/IO/FileSystem.TryCloneFile.OSX.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index d5786c88dc614..c7fc2db33b053 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -10,11 +10,11 @@ internal static partial class FileSystem { private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { - // Try to clone the file immediately, this will only succeed if the destination doesn't exist, - // so we don't worry about locking for this one. + // Try to clone the file immediately, this will only succeed if the + // destination doesn't exist, so we don't worry about locking for this one. if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) { - // Success + // Success. return true; } Interop.Error error = Interop.Sys.GetLastError(); @@ -22,8 +22,8 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F // Try to delete the destination file if we're overwriting. if (error == Interop.Error.EEXIST && overwrite) { - // Delete the destination. This should fail on directories. - // Get a lock to the dest file to ensure we don't copy onto it when it's locked by something else, and then delete it. + // Delete the destination. This should fail on directories. Get a lock to the dest file to ensure we don't copy onto it when + // it's locked by something else, and then delete it. It should also fail if destination == source since it's already locked. try { using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, @@ -35,10 +35,10 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F // We don't want to throw if it's just the file not existing, since we're trying to delete it. } - // Try clonefile: + // Try clonefile now we've deleted the destination file. if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) { - // Success + // Success. return true; } error = Interop.Sys.GetLastError(); @@ -47,12 +47,12 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F // Check if it's not supported, or if they're on different filesystems. if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV) { - // Fall back to normal copy + // Fall back to normal copy. return false; } - // Throw the appropriate exception - Debug.Assert(error != Interop.Error.SUCCESS); // We shouldn't fail with success + // Throw the appropriate exception. + Debug.Assert(error != Interop.Error.SUCCESS); // We shouldn't fail with success. throw Interop.GetExceptionForIoErrno(error.Info()); } } From 1db28fde2baaf8aa9ad8a918c659da56127aa608 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 08:43:25 +1000 Subject: [PATCH 60/81] Add handler for case of EEXIST still --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index c7fc2db33b053..26e550fad0422 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -44,8 +44,8 @@ private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.F error = Interop.Sys.GetLastError(); } - // Check if it's not supported, or if they're on different filesystems. - if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV) + // Check if it's not supported, if they're on different filesystems, or the destination file still exists. + if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST) { // Fall back to normal copy. return false; From d360ae4fc7e7d37fc088b96635aa33899bb66cab Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 08:43:59 +1000 Subject: [PATCH 61/81] Remove partial method definitions --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- .../src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs | 2 +- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 3 --- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 26e550fad0422..72d243907ca2a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -8,7 +8,7 @@ namespace System.IO { internal static partial class FileSystem { - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) + private static bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { // Try to clone the file immediately, this will only succeed if the // destination doesn't exist, so we don't worry about locking for this one. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs index cd129c321eb61..f67c5fbe53ffa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -5,7 +5,7 @@ namespace System.IO { internal static partial class FileSystem { - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) + private static bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) { // No such functionality is available on unix OSes (other than OSX-like ones). return false; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 2c714bac8ab5c..0409ad6bd9cf9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,9 +28,6 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - // TryCloneFile is defined in either FileSystem.TryCloneFile.OSX.cs or FileSystem.TryCloneFile.OtherUnix.cs. - private static partial bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite); - public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { // Open the src file handle, and read the file permissions. From 323cf672f8c4b6a3b26113a78525eb51bfcf0be2 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 08:58:51 +1000 Subject: [PATCH 62/81] Add some code to deal with EINVAL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • We can get EINVAL resulting from trying to copy ACLs, so we retry with with the flag to copy ACLs disabled (up to 1 time) --- .../System/IO/FileSystem.TryCloneFile.OSX.cs | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 72d243907ca2a..9549cb9d92d3f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -12,13 +12,23 @@ private static bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatu { // Try to clone the file immediately, this will only succeed if the // destination doesn't exist, so we don't worry about locking for this one. - if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) + int flags = Interop.@libc.CLONE_ACL; + retry1: + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, flags) == 0) { // Success. return true; } Interop.Error error = Interop.Sys.GetLastError(); + // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. + // This will disable them and allow trying again (a maximum of 1 time). + if (flags != 0 && error == Interop.Error.EINVAL) + { + flags = 0; + goto retry1; + } + // Try to delete the destination file if we're overwriting. if (error == Interop.Error.EEXIST && overwrite) { @@ -36,16 +46,26 @@ private static bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatu } // Try clonefile now we've deleted the destination file. - if (Interop.@libc.clonefile(sourceFullPath, destFullPath, Interop.@libc.CLONE_ACL) == 0) + retry2: + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, flags) == 0) { // Success. return true; } error = Interop.Sys.GetLastError(); + + // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. + // This will disable them and allow trying again (a maximum of 1 time). + if (flags != 0 && error == Interop.Error.EINVAL) + { + flags = 0; + goto retry2; + } } - // Check if it's not supported, if they're on different filesystems, or the destination file still exists. - if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST) + // Check if it's not supported, if they're on different filesystems, the destination file still exists, or if we have unsupported flags still. + // EINVAL is checked again here since clonefile is implemented by filesystem-specific drivers, therefore it could potentially fail with flags=0. + if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST || error == Interop.Error.EINVAL) { // Fall back to normal copy. return false; From 44ef8f3544331bac44e17f9c522b8a6bd5ec42cd Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 08:59:44 +1000 Subject: [PATCH 63/81] Remove srcStat parameter as it's no longer needed --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- .../src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs | 2 +- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 9549cb9d92d3f..77b9bf31e6569 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -8,7 +8,7 @@ namespace System.IO { internal static partial class FileSystem { - private static bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) + private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) { // Try to clone the file immediately, this will only succeed if the // destination doesn't exist, so we don't worry about locking for this one. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs index f67c5fbe53ffa..cd08ef7670fa1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -5,7 +5,7 @@ namespace System.IO { internal static partial class FileSystem { - private static bool TryCloneFile(string sourceFullPath, in Interop.Sys.FileStatus srcStat, string destFullPath, bool overwrite) + private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) { // No such functionality is available on unix OSes (other than OSX-like ones). return false; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 0409ad6bd9cf9..49e78fa28ec54 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -34,7 +34,7 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus srcFileStatus); // Try to clone the file first. - if (TryCloneFile(sourceFullPath, in srcFileStatus, destFullPath, overwrite)) + if (TryCloneFile(sourceFullPath, destFullPath, overwrite)) { return; } From 50195afe8e0c29de4df990eecec70ea82b308865 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 09:03:26 +1000 Subject: [PATCH 64/81] Revert now-unnecessary changes to OpenReadOnly --- .../Win32/SafeHandles/SafeFileHandle.Unix.cs | 25 +++++++++++-------- .../src/System/IO/FileSystem.Unix.cs | 9 ++++--- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs index 5b00a5cdef9c9..fb5c8faad4688 100644 --- a/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs @@ -11,7 +11,7 @@ namespace Microsoft.Win32.SafeHandles { public sealed partial class SafeFileHandle : SafeHandleZeroOrMinusOneIsInvalid { - internal const UnixFileMode PermissionMask = + private const UnixFileMode PermissionMask = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | @@ -165,22 +165,23 @@ public override bool IsInvalid } } - // Specialized Open that returns the FileStatus of the opened file. + // Specialized Open that returns the file length and permissions of the opened file. // This information is retrieved from the 'stat' syscall that must be performed to ensure the path is not a directory. - internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options, out Interop.Sys.FileStatus status) + internal static SafeFileHandle OpenReadOnly(string fullPath, FileOptions options, out long fileLength, out UnixFileMode filePermissions) { - SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out status); + SafeFileHandle handle = Open(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read, options, preallocationSize: 0, DefaultCreateMode, out fileLength, out filePermissions, null); + Debug.Assert(fileLength >= 0); return handle; } internal static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode? unixCreateMode = null, Func? createOpenException = null) { - return Open(fullPath, mode, access, share, options, preallocationSize, unixCreateMode ?? DefaultCreateMode, out _, createOpenException); + return Open(fullPath, mode, access, share, options, preallocationSize, unixCreateMode ?? DefaultCreateMode, out _, out _, createOpenException); } private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, UnixFileMode openPermissions, - out Interop.Sys.FileStatus status, + out long fileLength, out UnixFileMode filePermissions, Func? createOpenException = null) { // Translate the arguments into arguments for an open call. @@ -195,7 +196,7 @@ private static SafeFileHandle Open(string fullPath, FileMode mode, FileAccess ac // When Init return false, the path has changed to another file entry, and // we need to re-open the path to reflect that. - if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize, out status)) + if (safeFileHandle.Init(fullPath, mode, access, share, options, preallocationSize, out fileLength, out filePermissions)) { return safeFileHandle; } @@ -293,11 +294,12 @@ private static Interop.Sys.OpenFlags PreOpenConfigurationFromOptions(FileMode mo } private bool Init(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize, - out Interop.Sys.FileStatus status) + out long fileLength, out UnixFileMode filePermissions) { + Interop.Sys.FileStatus status = default; bool statusHasValue = false; - - status = default; + fileLength = -1; + filePermissions = 0; // Make sure our handle is not a directory. // We can omit the check when write access is requested. open will have failed with EISDIR. @@ -319,6 +321,9 @@ private bool Init(string path, FileMode mode, FileAccess access, FileShare share _canSeek = NullableBool.True; Debug.Assert(Interop.Sys.LSeek(this, 0, Interop.Sys.SeekWhence.SEEK_CUR) >= 0); } + + fileLength = status.Size; + filePermissions = ((UnixFileMode)status.Mode) & PermissionMask; } IsAsync = (options & FileOptions.Asynchronous) != 0; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 49e78fa28ec54..c929a1c56ab69 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -30,8 +30,10 @@ internal static partial class FileSystem public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - // Open the src file handle, and read the file permissions. - using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out Interop.Sys.FileStatus srcFileStatus); + // Open the src file handle. + long fileLength; + UnixFileMode filePermissions; + using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out fileLength, out filePermissions); // Try to clone the file first. if (TryCloneFile(sourceFullPath, destFullPath, overwrite)) @@ -40,11 +42,10 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove } // Open the dst file handle, and copy the file. - UnixFileMode filePermissions = (UnixFileMode)srcFileStatus.Mode & SafeFileHandle.PermissionMask; using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, CreateOpenException); - Interop.CheckIo(Interop.Sys.CopyFile(src, dst, srcFileStatus.Size)); + Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileLength)); // Exception handler for SafeFileHandle.Open failing. static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) From 53f52bc8a3753867ad3314bf10465b0ee9657809 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 09:06:23 +1000 Subject: [PATCH 65/81] Use Unlink instead of File.Delete --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 77b9bf31e6569..9b47926c149d2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -38,7 +38,15 @@ private static bool TryCloneFile(string sourceFullPath, string destFullPath, boo { using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0); - File.Delete(destFullPath); + if (Interop.Sys.Unlink(destFullPath) < 0) + { + Interop.ErrorInfo errorInfo = Interop.Sys.GetLastError(); + if (error != Interop.Error.ENOENT) + { + // Fall back to standard copy as an unexpected error has occured. + return false; + } + } } catch (FileNotFoundException) { From aee3ea8456b059ab5a33020130fdf496405c6383 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 09:09:23 +1000 Subject: [PATCH 66/81] Remove unnecessary diffs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Let me know if I should add these comments back --- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index c929a1c56ab69..c5d11cce257ba 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -30,7 +30,6 @@ internal static partial class FileSystem public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { - // Open the src file handle. long fileLength; UnixFileMode filePermissions; using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out fileLength, out filePermissions); @@ -41,13 +40,12 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove return; } - // Open the dst file handle, and copy the file. using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, CreateOpenException); + Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileLength)); - // Exception handler for SafeFileHandle.Open failing. static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) { // If the destination path points to a directory, we throw to match Windows behaviour. From c6174e64f29b3df090bb9d71077ad5d66f509d7d Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 10:02:41 +1000 Subject: [PATCH 67/81] Fix compilation errors --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- .../src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 9b47926c149d2..757cb34dd42ea 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -40,7 +40,7 @@ private static bool TryCloneFile(string sourceFullPath, string destFullPath, boo FileShare.None, FileOptions.None, preallocationSize: 0); if (Interop.Sys.Unlink(destFullPath) < 0) { - Interop.ErrorInfo errorInfo = Interop.Sys.GetLastError(); + Interop.Error errorInfo = Interop.Sys.GetLastError(); if (error != Interop.Error.ENOENT) { // Fall back to standard copy as an unexpected error has occured. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs index cd08ef7670fa1..8eed887aa1c9b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -5,7 +5,9 @@ namespace System.IO { internal static partial class FileSystem { +#pragma warning disable IDE0060 // Remove unused parameter private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) +#pragma warning restore IDE0060 // Remove unused parameter { // No such functionality is available on unix OSes (other than OSX-like ones). return false; From 0838d9f89a7cdb79762f7d98828484ed395e93a0 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 11:00:46 +1000 Subject: [PATCH 68/81] Update src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs Co-authored-by: Dan Moseley --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 757cb34dd42ea..fbf77bcc9c06f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -43,7 +43,7 @@ private static bool TryCloneFile(string sourceFullPath, string destFullPath, boo Interop.Error errorInfo = Interop.Sys.GetLastError(); if (error != Interop.Error.ENOENT) { - // Fall back to standard copy as an unexpected error has occured. + // Fall back to standard copy as an unexpected error has occurred. return false; } } From 013c7757ce075ed2db25954f76113a90134f8bc3 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 11:03:57 +1000 Subject: [PATCH 69/81] Reorder flags check as per feedback --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index fbf77bcc9c06f..3b76b62f04e56 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -23,7 +23,7 @@ private static bool TryCloneFile(string sourceFullPath, string destFullPath, boo // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. // This will disable them and allow trying again (a maximum of 1 time). - if (flags != 0 && error == Interop.Error.EINVAL) + if (error == Interop.Error.EINVAL && flags != 0) { flags = 0; goto retry1; @@ -64,7 +64,7 @@ private static bool TryCloneFile(string sourceFullPath, string destFullPath, boo // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. // This will disable them and allow trying again (a maximum of 1 time). - if (flags != 0 && error == Interop.Error.EINVAL) + if (error == Interop.Error.EINVAL && flags != 0) { flags = 0; goto retry2; From 487aee95ff35f613174b9c4b84fad93b8625c976 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 14:21:10 +1000 Subject: [PATCH 70/81] Fix test failures --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 3b76b62f04e56..970920b9bbc1a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -81,7 +81,7 @@ private static bool TryCloneFile(string sourceFullPath, string destFullPath, boo // Throw the appropriate exception. Debug.Assert(error != Interop.Error.SUCCESS); // We shouldn't fail with success. - throw Interop.GetExceptionForIoErrno(error.Info()); + throw Interop.GetExceptionForIoErrno(error.Info(), destFullPath); } } } From 37b518382e04bb964ec9267d337ef0b5658b0d60 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 14:25:41 +1000 Subject: [PATCH 71/81] Extract attempting clonefile to a separate function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • As per feedback. It allows us to avoid the gotos. • Also reorder the check on the second retry. --- .../System/IO/FileSystem.TryCloneFile.OSX.cs | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 970920b9bbc1a..65219843817cf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -10,23 +10,30 @@ internal static partial class FileSystem { private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) { + // This helper function calls out to clonefile, and returns the error. + static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, int flags, out Interop.Error error) + { + if (Interop.@libc.clonefile(sourceFullPath, destFullPath, flags) == 0) + { + // Success. + return true; + } + Interop.Error error = Interop.Sys.GetLastError(); + return false; + } + // Try to clone the file immediately, this will only succeed if the // destination doesn't exist, so we don't worry about locking for this one. int flags = Interop.@libc.CLONE_ACL; - retry1: - if (Interop.@libc.clonefile(sourceFullPath, destFullPath, flags) == 0) - { - // Success. - return true; - } - Interop.Error error = Interop.Sys.GetLastError(); + Interop.Error error; + if (TryCloneFile(sourceFullPath, destFullPath, overwrite, flags, out error)) return true; // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. // This will disable them and allow trying again (a maximum of 1 time). - if (error == Interop.Error.EINVAL && flags != 0) + if (error == Interop.Error.EINVAL) { flags = 0; - goto retry1; + if (TryCloneFile(sourceFullPath, destFullPath, overwrite, flags, out error)) return true; } // Try to delete the destination file if we're overwriting. @@ -54,20 +61,14 @@ private static bool TryCloneFile(string sourceFullPath, string destFullPath, boo } // Try clonefile now we've deleted the destination file. - retry2: - if (Interop.@libc.clonefile(sourceFullPath, destFullPath, flags) == 0) - { - // Success. - return true; - } - error = Interop.Sys.GetLastError(); + if (TryCloneFile(sourceFullPath, destFullPath, overwrite, flags, out error)) return true; // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. // This will disable them and allow trying again (a maximum of 1 time). - if (error == Interop.Error.EINVAL && flags != 0) + if (flags != 0 && error == Interop.Error.EINVAL) { flags = 0; - goto retry2; + if (TryCloneFile(sourceFullPath, destFullPath, overwrite, flags, out error)) return true; } } From bc99ec9eaaaf0a1467e82f0b131d551856d17881 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 14:40:44 +1000 Subject: [PATCH 72/81] Use CreateOpenException when deleting the destination file --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 4 ++-- .../src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs | 2 +- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 65219843817cf..10446ac20269c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -8,7 +8,7 @@ namespace System.IO { internal static partial class FileSystem { - private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) + private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, Func createOpenException) { // This helper function calls out to clonefile, and returns the error. static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, int flags, out Interop.Error error) @@ -44,7 +44,7 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwr try { using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, - FileShare.None, FileOptions.None, preallocationSize: 0); + FileShare.None, FileOptions.None, preallocationSize: 0, createOpenException: createOpenException); if (Interop.Sys.Unlink(destFullPath) < 0) { Interop.Error errorInfo = Interop.Sys.GetLastError(); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs index 8eed887aa1c9b..b47fdef2b4fba 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -6,7 +6,7 @@ namespace System.IO internal static partial class FileSystem { #pragma warning disable IDE0060 // Remove unused parameter - private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) + private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, Func createOpenException) #pragma warning restore IDE0060 // Remove unused parameter { // No such functionality is available on unix OSes (other than OSX-like ones). diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index c5d11cce257ba..dcbeebc56b52c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -35,7 +35,7 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out fileLength, out filePermissions); // Try to clone the file first. - if (TryCloneFile(sourceFullPath, destFullPath, overwrite)) + if (TryCloneFile(sourceFullPath, destFullPath, overwrite, CreateOpenException)) { return; } From e1c773987cd193ae5069e711351e37df7b3c2771 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 14:46:38 +1000 Subject: [PATCH 73/81] Use a debug statement to check EINVAL instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Since it should only happen for an invalid filesystem anyway --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 10446ac20269c..855c5e746edb2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -73,8 +73,8 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwr } // Check if it's not supported, if they're on different filesystems, the destination file still exists, or if we have unsupported flags still. - // EINVAL is checked again here since clonefile is implemented by filesystem-specific drivers, therefore it could potentially fail with flags=0. - if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST || error == Interop.Error.EINVAL) + Debug.Assert(error != Interop.Error.EINVAL); + if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST) { // Fall back to normal copy. return false; From aec1eac289bc4b50646dfcaccef3781c249f31aa Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 14:54:19 +1000 Subject: [PATCH 74/81] Improve & fix comment --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 855c5e746edb2..fe0e13ba41e61 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -72,7 +72,7 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwr } } - // Check if it's not supported, if they're on different filesystems, the destination file still exists, or if we have unsupported flags still. + // Check if it's not supported, if files are on different filesystems, or if the destination file still exists. Debug.Assert(error != Interop.Error.EINVAL); if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST) { From 483858d21ee9acb0990e03864e1ba95be68b1ce6 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 15:26:30 +1000 Subject: [PATCH 75/81] Fix compilation errors --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index fe0e13ba41e61..cf9a8cb5c943d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -11,14 +11,15 @@ internal static partial class FileSystem private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, Func createOpenException) { // This helper function calls out to clonefile, and returns the error. - static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, int flags, out Interop.Error error) + static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, out Interop.Error error) { if (Interop.@libc.clonefile(sourceFullPath, destFullPath, flags) == 0) { // Success. + error = default; return true; } - Interop.Error error = Interop.Sys.GetLastError(); + error = Interop.Sys.GetLastError(); return false; } @@ -26,14 +27,14 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwr // destination doesn't exist, so we don't worry about locking for this one. int flags = Interop.@libc.CLONE_ACL; Interop.Error error; - if (TryCloneFile(sourceFullPath, destFullPath, overwrite, flags, out error)) return true; + if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. // This will disable them and allow trying again (a maximum of 1 time). if (error == Interop.Error.EINVAL) { flags = 0; - if (TryCloneFile(sourceFullPath, destFullPath, overwrite, flags, out error)) return true; + if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; } // Try to delete the destination file if we're overwriting. @@ -61,14 +62,14 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwr } // Try clonefile now we've deleted the destination file. - if (TryCloneFile(sourceFullPath, destFullPath, overwrite, flags, out error)) return true; + if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. // This will disable them and allow trying again (a maximum of 1 time). if (flags != 0 && error == Interop.Error.EINVAL) { flags = 0; - if (TryCloneFile(sourceFullPath, destFullPath, overwrite, flags, out error)) return true; + if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; } } From f67bd18b5237853b3b22f9ea7a74fe524240c75a Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 16:42:38 +1000 Subject: [PATCH 76/81] Implement feedback --- .../System/IO/FileSystem.TryCloneFile.OSX.cs | 14 +++---------- .../IO/FileSystem.TryCloneFile.OtherUnix.cs | 4 ++-- .../src/System/IO/FileSystem.Unix.cs | 20 +++++++++---------- 3 files changed, 15 insertions(+), 23 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index cf9a8cb5c943d..4f3d4237e8198 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -8,7 +8,7 @@ namespace System.IO { internal static partial class FileSystem { - private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, Func createOpenException) + private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) { // This helper function calls out to clonefile, and returns the error. static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, out Interop.Error error) @@ -16,7 +16,7 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, if (Interop.@libc.clonefile(sourceFullPath, destFullPath, flags) == 0) { // Success. - error = default; + error = Interop.Error.SUCCESS; return true; } error = Interop.Sys.GetLastError(); @@ -45,7 +45,7 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, try { using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, - FileShare.None, FileOptions.None, preallocationSize: 0, createOpenException: createOpenException); + FileShare.None, FileOptions.None, preallocationSize: 0, createOpenException: CreateOpenExceptionForCopyFile); if (Interop.Sys.Unlink(destFullPath) < 0) { Interop.Error errorInfo = Interop.Sys.GetLastError(); @@ -63,14 +63,6 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, // Try clonefile now we've deleted the destination file. if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; - - // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. - // This will disable them and allow trying again (a maximum of 1 time). - if (flags != 0 && error == Interop.Error.EINVAL) - { - flags = 0; - if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; - } } // Check if it's not supported, if files are on different filesystems, or if the destination file still exists. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs index b47fdef2b4fba..105a5ecccc4b6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs @@ -6,10 +6,10 @@ namespace System.IO internal static partial class FileSystem { #pragma warning disable IDE0060 // Remove unused parameter - private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, Func createOpenException) + private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) #pragma warning restore IDE0060 // Remove unused parameter { - // No such functionality is available on unix OSes (other than OSX-like ones). + // Cloning is implemented in PAL on unix OSes (other than OSX-like ones). return false; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index dcbeebc56b52c..25c2cbb1106bf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -35,27 +35,27 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out fileLength, out filePermissions); // Try to clone the file first. - if (TryCloneFile(sourceFullPath, destFullPath, overwrite, CreateOpenException)) + if (TryCloneFile(sourceFullPath, destFullPath, overwrite)) { return; } using SafeFileHandle dst = SafeFileHandle.Open(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, filePermissions, - CreateOpenException); + CreateOpenExceptionForCopyFile); Interop.CheckIo(Interop.Sys.CopyFile(src, dst, fileLength)); + } - static Exception? CreateOpenException(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) + private static Exception? CreateOpenExceptionForCopyFile(Interop.ErrorInfo error, Interop.Sys.OpenFlags flags, string path) + { + // If the destination path points to a directory, we throw to match Windows behaviour. + if (error.Error == Interop.Error.EEXIST && DirectoryExists(path)) { - // If the destination path points to a directory, we throw to match Windows behaviour. - if (error.Error == Interop.Error.EEXIST && DirectoryExists(path)) - { - return new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); - } - - return null; // Let SafeFileHandle create the exception for this error. + return new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path)); } + + return null; // Let SafeFileHandle create the exception for this error. } #pragma warning disable IDE0060 From 5cf74af8969df5a850721c2c148ada4348c0d129 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Wed, 24 May 2023 16:51:11 +1000 Subject: [PATCH 77/81] Move the Debug.Assert calls together --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 4f3d4237e8198..1c3c1784c816b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -66,7 +66,6 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, } // Check if it's not supported, if files are on different filesystems, or if the destination file still exists. - Debug.Assert(error != Interop.Error.EINVAL); if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST) { // Fall back to normal copy. @@ -74,6 +73,7 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, } // Throw the appropriate exception. + Debug.Assert(error != Interop.Error.EINVAL); // We shouldn't fail due to an invalid parameter. Debug.Assert(error != Interop.Error.SUCCESS); // We shouldn't fail with success. throw Interop.GetExceptionForIoErrno(error.Info(), destFullPath); } From 10011a5dceb6caf24e9ec8441cafc826fd8b58b2 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Jun 2023 17:02:38 +1000 Subject: [PATCH 78/81] Implement feedback to use partial functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Implement feedback to use partial functions so that TryCloneFile doesn't have to exist other than when it's actually needed --- .../System.Private.CoreLib.Shared.projitems | 1 - .../System/IO/FileSystem.TryCloneFile.OSX.cs | 24 ++++++++++++++----- .../IO/FileSystem.TryCloneFile.OtherUnix.cs | 16 ------------- .../src/System/IO/FileSystem.Unix.cs | 6 ++++- 4 files changed, 23 insertions(+), 24 deletions(-) delete mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index e193fb0adaafb..73cc437f0a915 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -2373,7 +2373,6 @@ - diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 1c3c1784c816b..402dda449093e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -8,7 +8,7 @@ namespace System.IO { internal static partial class FileSystem { - private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) + private static partial void TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, ref bool cloned) { // This helper function calls out to clonefile, and returns the error. static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, out Interop.Error error) @@ -27,14 +27,22 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, // destination doesn't exist, so we don't worry about locking for this one. int flags = Interop.@libc.CLONE_ACL; Interop.Error error; - if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; + if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) + { + cloned = true; + return; + } // Some filesystems don't support ACLs, so may fail due to trying to copy ACLs. // This will disable them and allow trying again (a maximum of 1 time). if (error == Interop.Error.EINVAL) { flags = 0; - if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; + if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) + { + cloned = true; + return; + } } // Try to delete the destination file if we're overwriting. @@ -52,7 +60,7 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, if (error != Interop.Error.ENOENT) { // Fall back to standard copy as an unexpected error has occurred. - return false; + return; } } } @@ -62,14 +70,18 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, } // Try clonefile now we've deleted the destination file. - if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) return true; + if (TryCloneFile(sourceFullPath, destFullPath, flags, out error)) + { + cloned = true; + return; + } } // Check if it's not supported, if files are on different filesystems, or if the destination file still exists. if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST) { // Fall back to normal copy. - return false; + return; } // Throw the appropriate exception. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs deleted file mode 100644 index 105a5ecccc4b6..0000000000000 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OtherUnix.cs +++ /dev/null @@ -1,16 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -namespace System.IO -{ - internal static partial class FileSystem - { -#pragma warning disable IDE0060 // Remove unused parameter - private static bool TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite) -#pragma warning restore IDE0060 // Remove unused parameter - { - // Cloning is implemented in PAL on unix OSes (other than OSX-like ones). - return false; - } - } -} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 25c2cbb1106bf..f8e68a34a24c7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,6 +28,8 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; + private static partial void TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, ref bool cloned); + public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) { long fileLength; @@ -35,7 +37,9 @@ public static void CopyFile(string sourceFullPath, string destFullPath, bool ove using SafeFileHandle src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out fileLength, out filePermissions); // Try to clone the file first. - if (TryCloneFile(sourceFullPath, destFullPath, overwrite)) + bool cloned = false; + TryCloneFile(sourceFullPath, destFullPath, overwrite, ref cloned); + if (cloned) { return; } From a793bb70c66673aa289af414dc521de04c26cee8 Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Jun 2023 17:06:47 +1000 Subject: [PATCH 79/81] Implement feedback to use pattern matching, and move comments --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 402dda449093e..23c9bd5e1dcdc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -77,8 +77,9 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, } } - // Check if it's not supported, if files are on different filesystems, or if the destination file still exists. - if (error == Interop.Error.ENOTSUP || error == Interop.Error.EXDEV || error == Interop.Error.EEXIST) + if (error is Interop.Error.ENOTSUP // Check if it's not supported, + or Interop.Error.EXDEV // if files are on different filesystems, + or Interop.Error.EEXIST) // or if the destination file still exists. { // Fall back to normal copy. return; From 550a76a6aeafd814ff2820b364fd23863092e83e Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Jun 2023 17:09:17 +1000 Subject: [PATCH 80/81] Implement other feedback --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 23c9bd5e1dcdc..9375215f38cd0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -19,6 +19,7 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, error = Interop.Error.SUCCESS; return true; } + error = Interop.Sys.GetLastError(); return false; } @@ -54,14 +55,11 @@ static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, { using SafeFileHandle? dstHandle = SafeFileHandle.Open(destFullPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None, FileOptions.None, preallocationSize: 0, createOpenException: CreateOpenExceptionForCopyFile); - if (Interop.Sys.Unlink(destFullPath) < 0) + if (Interop.Sys.Unlink(destFullPath) < 0 && + Interop.Sys.GetLastError() != Interop.Error.ENOENT) { - Interop.Error errorInfo = Interop.Sys.GetLastError(); - if (error != Interop.Error.ENOENT) - { - // Fall back to standard copy as an unexpected error has occurred. - return; - } + // Fall back to standard copy as an unexpected error has occurred. + return; } } catch (FileNotFoundException) From 32c546374d94efc581ed1cc9353b204bf6fa6b2d Mon Sep 17 00:00:00 2001 From: Hamish Arblaster Date: Tue, 6 Jun 2023 17:20:06 +1000 Subject: [PATCH 81/81] Fix visibility of partial method --- .../src/System/IO/FileSystem.TryCloneFile.OSX.cs | 2 +- .../System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs index 9375215f38cd0..8bfb60414a707 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.TryCloneFile.OSX.cs @@ -8,7 +8,7 @@ namespace System.IO { internal static partial class FileSystem { - private static partial void TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, ref bool cloned) + static partial void TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, ref bool cloned) { // This helper function calls out to clonefile, and returns the error. static bool TryCloneFile(string sourceFullPath, string destFullPath, int flags, out Interop.Error error) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index f8e68a34a24c7..be6505d9bd93f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -28,7 +28,7 @@ internal static partial class FileSystem UnixFileMode.OtherWrite | UnixFileMode.OtherExecute; - private static partial void TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, ref bool cloned); + static partial void TryCloneFile(string sourceFullPath, string destFullPath, bool overwrite, ref bool cloned); public static void CopyFile(string sourceFullPath, string destFullPath, bool overwrite) {