From 543c73df70ef5f0ac196aceddb14a322b72a484b Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Fri, 26 Feb 2021 22:12:37 -0800 Subject: [PATCH 1/8] Fix `Move-Item` for FileSystemProvider to use copy-delete instead of move for UNC paths --- .../namespaces/FileSystemProvider.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index db619e3d15e..69ce7791728 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -6143,6 +6143,15 @@ private static bool IsSameWindowsVolume(string source, string destination) FileInfo src = new FileInfo(source); FileInfo dest = new FileInfo(destination); + // DFS shares will have same root name, but are actually different volumes + // Since we cannot determine a path is a DFS share or a SMB share, we + // treat all UNC paths as being on different volumes which results + // in a copy-delete operation instead of a move which will fail across volumes + if (Utils.PathIsUnc(source) || Utils.PathIsUnc(destination)) + { + return false; + } + return (src.Directory.Root.Name == dest.Directory.Root.Name); } #endif From 2513c2ff8a99e2e36f62b3d183fd009a617b04c5 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Mon, 1 Mar 2021 13:03:32 -0800 Subject: [PATCH 2/8] Change fix to use CopyAndDelete only if AccessDenied error comes up --- .../namespaces/FileSystemProvider.cs | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index 69ce7791728..4eafe44e288 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -55,6 +55,10 @@ public sealed partial class FileSystemProvider : NavigationCmdletProvider, // This is the errno returned by the rename() syscall // when an item is attempted to be renamed across filesystem mount boundaries. private const int UNIX_ERRNO_EXDEV = 18; +#else + // This is the HRESULT returned by move if it is + // attempted across disk volumes on Windows such as DFS shares. + private const int ERROR_ACCESS_DENIED = -2146232800; #endif // 4MB gives the best results without spiking the resources on the remote connection for file transfers between pssessions. @@ -6093,7 +6097,15 @@ private void MoveDirectoryInfoUnchecked(DirectoryInfo directory, string destinat // is just a question of the drive if (IsSameWindowsVolume(directory.FullName, destinationPath)) { - directory.MoveTo(destinationPath); + try + { + directory.MoveTo(destinationPath); + } + catch (IOException e) when (e.HResult == ERROR_ACCESS_DENIED) + { + // If move doesn't work because the source and destination are on different volumes (like DFS), fall back to CopyAndDelete + CopyAndDelete(directory, destinationPath, force); + } } else { @@ -6143,15 +6155,6 @@ private static bool IsSameWindowsVolume(string source, string destination) FileInfo src = new FileInfo(source); FileInfo dest = new FileInfo(destination); - // DFS shares will have same root name, but are actually different volumes - // Since we cannot determine a path is a DFS share or a SMB share, we - // treat all UNC paths as being on different volumes which results - // in a copy-delete operation instead of a move which will fail across volumes - if (Utils.PathIsUnc(source) || Utils.PathIsUnc(destination)) - { - return false; - } - return (src.Directory.Root.Name == dest.Directory.Root.Name); } #endif From 18d24721495bcf36fbf4eef1529d6e4672395578 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Tue, 2 Mar 2021 07:04:11 -0800 Subject: [PATCH 3/8] remove unncessary check for Windows if move is to same volume --- .../namespaces/FileSystemProvider.cs | 27 +++---------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index 4eafe44e288..d48db36c960 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -6093,22 +6093,13 @@ private void MoveDirectoryInfoUnchecked(DirectoryInfo directory, string destinat CopyAndDelete(directory, destinationPath, force); } #else - // On Windows, being able to rename vs copy/delete a file - // is just a question of the drive - if (IsSameWindowsVolume(directory.FullName, destinationPath)) + try { - try - { - directory.MoveTo(destinationPath); - } - catch (IOException e) when (e.HResult == ERROR_ACCESS_DENIED) - { - // If move doesn't work because the source and destination are on different volumes (like DFS), fall back to CopyAndDelete - CopyAndDelete(directory, destinationPath, force); - } + directory.MoveTo(destinationPath); } - else + catch (IOException e) when (e.HResult == ERROR_ACCESS_DENIED) { + // If move doesn't work because the source and destination are on different volumes (like DFS), fall back to CopyAndDelete CopyAndDelete(directory, destinationPath, force); } #endif @@ -6149,16 +6140,6 @@ private void CopyAndDelete(DirectoryInfo directory, string destination, bool for } } -#if !UNIX - private static bool IsSameWindowsVolume(string source, string destination) - { - FileInfo src = new FileInfo(source); - FileInfo dest = new FileInfo(destination); - - return (src.Directory.Root.Name == dest.Directory.Root.Name); - } -#endif - #endregion MoveItem #endregion NavigationCmdletProvider members From 27fb53a2a7607332aa30d759c4d9589b59d1f8f0 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Wed, 3 Mar 2021 07:12:43 -0800 Subject: [PATCH 4/8] change to try copydelete on any IOException as different HResult can be returned --- .../namespaces/FileSystemProvider.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index d48db36c960..e9fc9990283 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -55,10 +55,6 @@ public sealed partial class FileSystemProvider : NavigationCmdletProvider, // This is the errno returned by the rename() syscall // when an item is attempted to be renamed across filesystem mount boundaries. private const int UNIX_ERRNO_EXDEV = 18; -#else - // This is the HRESULT returned by move if it is - // attempted across disk volumes on Windows such as DFS shares. - private const int ERROR_ACCESS_DENIED = -2146232800; #endif // 4MB gives the best results without spiking the resources on the remote connection for file transfers between pssessions. @@ -6095,9 +6091,14 @@ private void MoveDirectoryInfoUnchecked(DirectoryInfo directory, string destinat #else try { + if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory) + { + throw new IOException("Invalid cross-device link"); + } + directory.MoveTo(destinationPath); } - catch (IOException e) when (e.HResult == ERROR_ACCESS_DENIED) + catch (IOException) { // If move doesn't work because the source and destination are on different volumes (like DFS), fall back to CopyAndDelete CopyAndDelete(directory, destinationPath, force); From 4ea411220a2e99a66f39f6593382e31937888006 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Wed, 3 Mar 2021 10:19:43 -0800 Subject: [PATCH 5/8] address Ilya's feedback by not falling back on all IOException types --- .../namespaces/FileSystemProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index e9fc9990283..10db8d2ecde 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -6098,7 +6098,7 @@ private void MoveDirectoryInfoUnchecked(DirectoryInfo directory, string destinat directory.MoveTo(destinationPath); } - catch (IOException) + catch (IOException e) when (e is not FileNotFoundException && e is not DirectoryNotFoundException && e is not PathTooLongException) { // If move doesn't work because the source and destination are on different volumes (like DFS), fall back to CopyAndDelete CopyAndDelete(directory, destinationPath, force); From 3de7bd2c6125fc5a72efd3ebc73d9cdd4ab516bf Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Wed, 3 Mar 2021 10:57:08 -0800 Subject: [PATCH 6/8] update tests --- .../Microsoft.PowerShell.Management/FileSystem.Tests.ps1 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 index 0e4adaeec70..def0aaf8a68 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 @@ -154,7 +154,7 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { } It "Verify Move-Item will not move to an existing file" { - { Move-Item -Path $testDir -Destination $testFile -ErrorAction Stop } | Should -Throw -ErrorId "MoveDirectoryItemIOError,Microsoft.PowerShell.Commands.MoveItemCommand" + { Move-Item -Path $testDir -Destination $testFile -ErrorAction Stop } | Should -Throw -ErrorId "DirectoryExist,Microsoft.PowerShell.Commands.MoveItemCommand" $error[0].Exception | Should -BeOfType System.IO.IOException $testDir | Should -Exist } @@ -296,7 +296,7 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { # @{cmdline = "New-Item -Type File -Path $newItemPath -ErrorAction Stop"; expectedError = "NewItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.NewItemCommand"} @{cmdline = "Get-ChildItem $protectedPath -ErrorAction Stop"; expectedError = "DirUnauthorizedAccessError,Microsoft.PowerShell.Commands.GetChildItemCommand"} @{cmdline = "Rename-Item -Path $protectedPath -NewName bar -ErrorAction Stop"; expectedError = "RenameItemIOError,Microsoft.PowerShell.Commands.RenameItemCommand"}, - @{cmdline = "Move-Item -Path $protectedPath -Destination bar -ErrorAction Stop"; expectedError = "MoveDirectoryItemIOError,Microsoft.PowerShell.Commands.MoveItemCommand"} + @{cmdline = "Move-Item -Path $protectedPath -Destination bar -ErrorAction Stop"; expectedError = "MoveDirectoryItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.MoveItemCommand"} ) { param ($cmdline, $expectedError) From 5c804aa7c845d6889472fbcbb64267d2936d9152 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Thu, 4 Mar 2021 13:58:49 -0800 Subject: [PATCH 7/8] Unified Unix and Windows paths, check HRESULT for 0x80070005 AccessDenied for Windows to use CopyAndDelete() --- .../namespaces/FileSystemProvider.cs | 26 +++++-------------- .../FileSystem.Tests.ps1 | 4 +-- 2 files changed, 8 insertions(+), 22 deletions(-) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index 10db8d2ecde..9499bf95289 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -54,7 +54,10 @@ public sealed partial class FileSystemProvider : NavigationCmdletProvider, #if UNIX // This is the errno returned by the rename() syscall // when an item is attempted to be renamed across filesystem mount boundaries. - private const int UNIX_ERRNO_EXDEV = 18; + private const int MOVE_FAILED_ERROR = 18; +#else + // 0x80070005 ACCESS_DENIED is returned when trying to move files across volumes like DFS + private const int MOVE_FAILED_ERROR = -2147024891; #endif // 4MB gives the best results without spiking the resources on the remote connection for file transfers between pssessions. @@ -6071,39 +6074,22 @@ private void MoveDirectoryInfoItem( /// If true, force move the directory, overwriting anything at the destination. private void MoveDirectoryInfoUnchecked(DirectoryInfo directory, string destinationPath, bool force) { -#if UNIX try { if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory) { - throw new IOException("Invalid cross-device link", hresult: UNIX_ERRNO_EXDEV); + throw new IOException("Invalid cross-device link", hresult: MOVE_FAILED_ERROR); } directory.MoveTo(destinationPath); } - catch (IOException e) when (e.HResult == UNIX_ERRNO_EXDEV) + catch (IOException e) when (e.HResult == MOVE_FAILED_ERROR) { // Rather than try to ascertain whether we can rename a directory ahead of time, // it's both faster and more correct to try to rename it and fall back to copy/deleting it // See also: https://github.com/coreutils/coreutils/blob/439741053256618eb651e6d43919df29625b8714/src/mv.c#L212-L216 CopyAndDelete(directory, destinationPath, force); } -#else - try - { - if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory) - { - throw new IOException("Invalid cross-device link"); - } - - directory.MoveTo(destinationPath); - } - catch (IOException e) when (e is not FileNotFoundException && e is not DirectoryNotFoundException && e is not PathTooLongException) - { - // If move doesn't work because the source and destination are on different volumes (like DFS), fall back to CopyAndDelete - CopyAndDelete(directory, destinationPath, force); - } -#endif } private void CopyAndDelete(DirectoryInfo directory, string destination, bool force) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 index def0aaf8a68..0e4adaeec70 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 @@ -154,7 +154,7 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { } It "Verify Move-Item will not move to an existing file" { - { Move-Item -Path $testDir -Destination $testFile -ErrorAction Stop } | Should -Throw -ErrorId "DirectoryExist,Microsoft.PowerShell.Commands.MoveItemCommand" + { Move-Item -Path $testDir -Destination $testFile -ErrorAction Stop } | Should -Throw -ErrorId "MoveDirectoryItemIOError,Microsoft.PowerShell.Commands.MoveItemCommand" $error[0].Exception | Should -BeOfType System.IO.IOException $testDir | Should -Exist } @@ -296,7 +296,7 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { # @{cmdline = "New-Item -Type File -Path $newItemPath -ErrorAction Stop"; expectedError = "NewItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.NewItemCommand"} @{cmdline = "Get-ChildItem $protectedPath -ErrorAction Stop"; expectedError = "DirUnauthorizedAccessError,Microsoft.PowerShell.Commands.GetChildItemCommand"} @{cmdline = "Rename-Item -Path $protectedPath -NewName bar -ErrorAction Stop"; expectedError = "RenameItemIOError,Microsoft.PowerShell.Commands.RenameItemCommand"}, - @{cmdline = "Move-Item -Path $protectedPath -Destination bar -ErrorAction Stop"; expectedError = "MoveDirectoryItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.MoveItemCommand"} + @{cmdline = "Move-Item -Path $protectedPath -Destination bar -ErrorAction Stop"; expectedError = "MoveDirectoryItemIOError,Microsoft.PowerShell.Commands.MoveItemCommand"} ) { param ($cmdline, $expectedError) From 4723a93ff848fa32600c281319794f5fcc870b95 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Thu, 4 Mar 2021 14:34:05 -0800 Subject: [PATCH 8/8] Fix test --- .../Microsoft.PowerShell.Management/FileSystem.Tests.ps1 | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 index 0e4adaeec70..a9d35d7f789 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 @@ -287,6 +287,13 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { $newItemPath = Join-Path $protectedPath "foo" $shouldSkip = -not (Test-Path $protectedPath) } + + if ($IsWindows) { + $fqaccessdenied = "MoveDirectoryItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.MoveItemCommand" + } + else { + $fqaccessdenied = "MoveDirectoryItemIOError,Microsoft.PowerShell.Commands.MoveItemCommand" + } } It "Access-denied test for " -Skip:(-not $IsWindows -or $shouldSkip) -TestCases @( @@ -296,7 +303,7 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { # @{cmdline = "New-Item -Type File -Path $newItemPath -ErrorAction Stop"; expectedError = "NewItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.NewItemCommand"} @{cmdline = "Get-ChildItem $protectedPath -ErrorAction Stop"; expectedError = "DirUnauthorizedAccessError,Microsoft.PowerShell.Commands.GetChildItemCommand"} @{cmdline = "Rename-Item -Path $protectedPath -NewName bar -ErrorAction Stop"; expectedError = "RenameItemIOError,Microsoft.PowerShell.Commands.RenameItemCommand"}, - @{cmdline = "Move-Item -Path $protectedPath -Destination bar -ErrorAction Stop"; expectedError = "MoveDirectoryItemIOError,Microsoft.PowerShell.Commands.MoveItemCommand"} + @{cmdline = "Move-Item -Path $protectedPath -Destination bar -ErrorAction Stop"; expectedError = $fqaccessdenied} ) { param ($cmdline, $expectedError)