From db94e9381a5a7757e303f96de7b4c80ea7702f3e Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Sat, 20 Mar 2021 20:07:57 -0700 Subject: [PATCH 1/8] Restore check for common drive root on Windows for `Move-Item` to work across volumes --- .../namespaces/FileSystemProvider.cs | 43 +++++++++++++------ .../FileSystem.Tests.ps1 | 27 ++++++++++++ 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index 9499bf95289..e013eb8a5fb 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -6074,24 +6074,43 @@ private void MoveDirectoryInfoItem( /// If true, force move the directory, overwriting anything at the destination. private void MoveDirectoryInfoUnchecked(DirectoryInfo directory, string destinationPath, bool force) { - try + if (!IsSameWindowsVolume(directory.FullName, destinationPath)) { - if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory) - { - throw new IOException("Invalid cross-device link", hresult: MOVE_FAILED_ERROR); - } - - directory.MoveTo(destinationPath); + CopyAndDelete(directory, destinationPath, force); } - catch (IOException e) when (e.HResult == MOVE_FAILED_ERROR) + else { - // 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); + try + { + if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory) + { + throw new IOException("Invalid cross-device link", hresult: MOVE_FAILED_ERROR); + } + + directory.MoveTo(destinationPath); + } + 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); + } } } + private static bool IsSameWindowsVolume(string source, string destination) + { +#if UNIX + return true; +#endif + + FileInfo src = new FileInfo(source); + FileInfo dest = new FileInfo(destination); + + return (src.Directory.Root.Name == dest.Directory.Root.Name); + } + private void CopyAndDelete(DirectoryInfo directory, string destination, bool force) { if (!ItemExists(destination)) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 index a9d35d7f789..ec53cae579d 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 @@ -153,6 +153,33 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { } } + It "Verify Move-Item for directory across drives on Windows" -Skip:(!$IsWindows) { + try { + # find first available drive letter, unfortunately need to use both function: and Win32_LogicalDisk to cover + # both subst drives and bitlocker drives + $drive = Get-ChildItem function:[d-z]: -Name | Where-Object { !(Test-Path -Path $_) -and !(Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='$_'") } | Select-Object -First 1 + if ($null -eq $drive) { + throw "Test cannot continue as no drive letter available" + } + + $dest = (Resolve-Path -Path $TestDrive).ProviderPath + $null = New-Item -ItemType Directory -Path $dest -Name test + subst $drive $dest + if ($LASTEXITCODE -ne 0) { + throw "subst failed with exit code $LASTEXITCODE" + } + + $testdir = New-Item -ItemType Directory -Path $drive -Name testmovedir -Force + 1 > $testdir\test.txt + Move-Item $drive\testmovedir $dest\test + "$drive\testmovedir" | Should -Not -Exist + "$dest\test\testmovedir\test.txt" | Should -FileContentMatchExactly 1 + } + finally { + subst $drive /d + } + } + 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" $error[0].Exception | Should -BeOfType System.IO.IOException From 52eb274b74b568e8e44b269fb1818416876f7f0b Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Sun, 21 Mar 2021 06:36:59 -0700 Subject: [PATCH 2/8] fix codefactor and build on non-Windows --- .../namespaces/FileSystemProvider.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index e013eb8a5fb..61c7a77e6d0 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -6103,12 +6103,12 @@ private static bool IsSameWindowsVolume(string source, string destination) { #if UNIX return true; -#endif - +#else FileInfo src = new FileInfo(source); FileInfo dest = new FileInfo(destination); - return (src.Directory.Root.Name == dest.Directory.Root.Name); + return src.Directory.Root.Name == dest.Directory.Root.Name; +#endif } private void CopyAndDelete(DirectoryInfo directory, string destination, bool force) From 403e5d905b1062ae8e67192b978d0686de3a39dd Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Sun, 21 Mar 2021 08:40:31 -0700 Subject: [PATCH 3/8] add tracing to see why subst is failing --- .../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 ec53cae579d..9eff6000166 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 @@ -164,9 +164,9 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { $dest = (Resolve-Path -Path $TestDrive).ProviderPath $null = New-Item -ItemType Directory -Path $dest -Name test - subst $drive $dest + $out = subst $drive $dest 2>&1 if ($LASTEXITCODE -ne 0) { - throw "subst failed with exit code $LASTEXITCODE" + throw "subst failed with exit code ${LASTEXITCODE}: $out" } $testdir = New-Item -ItemType Directory -Path $drive -Name testmovedir -Force From 90232b794f475f5fed5ddb763a0818954f3d6133 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Sun, 21 Mar 2021 09:27:19 -0700 Subject: [PATCH 4/8] change range of drive letters --- .../Microsoft.PowerShell.Management/FileSystem.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 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 9eff6000166..955d1474bbd 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 @@ -157,7 +157,7 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { try { # find first available drive letter, unfortunately need to use both function: and Win32_LogicalDisk to cover # both subst drives and bitlocker drives - $drive = Get-ChildItem function:[d-z]: -Name | Where-Object { !(Test-Path -Path $_) -and !(Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='$_'") } | Select-Object -First 1 + $drive = Get-ChildItem function:[f-z]: -Name | Where-Object { !(Test-Path -Path $_) -and !(Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='$_'") } | Select-Object -First 1 if ($null -eq $drive) { throw "Test cannot continue as no drive letter available" } From d51fc3db290410fb35ea95028ca0ec131401f7fa Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Sun, 21 Mar 2021 10:02:49 -0700 Subject: [PATCH 5/8] add a bit more tracing --- .../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 955d1474bbd..da751341fe6 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 @@ -157,7 +157,7 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { try { # find first available drive letter, unfortunately need to use both function: and Win32_LogicalDisk to cover # both subst drives and bitlocker drives - $drive = Get-ChildItem function:[f-z]: -Name | Where-Object { !(Test-Path -Path $_) -and !(Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='$_'") } | Select-Object -First 1 + $drive = Get-ChildItem function:[h-z]: -Name | Where-Object { !(Test-Path -Path $_) -and !(Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='$_'") } | Select-Object -First 1 if ($null -eq $drive) { throw "Test cannot continue as no drive letter available" } @@ -166,7 +166,7 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { $null = New-Item -ItemType Directory -Path $dest -Name test $out = subst $drive $dest 2>&1 if ($LASTEXITCODE -ne 0) { - throw "subst failed with exit code ${LASTEXITCODE}: $out" + throw "subst failed with exit code ${LASTEXITCODE} for drive '$drive': $out" } $testdir = New-Item -ItemType Directory -Path $drive -Name testmovedir -Force From 5982608c66db18bdc69cffbe9ad55e4d47f74eb8 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Sun, 21 Mar 2021 17:34:00 -0700 Subject: [PATCH 6/8] make test require admin on Windows --- .../FileSystem.Tests.ps1 | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 index da751341fe6..5a7a0625bcb 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 @@ -153,33 +153,6 @@ Describe "Basic FileSystem Provider Tests" -Tags "CI" { } } - It "Verify Move-Item for directory across drives on Windows" -Skip:(!$IsWindows) { - try { - # find first available drive letter, unfortunately need to use both function: and Win32_LogicalDisk to cover - # both subst drives and bitlocker drives - $drive = Get-ChildItem function:[h-z]: -Name | Where-Object { !(Test-Path -Path $_) -and !(Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='$_'") } | Select-Object -First 1 - if ($null -eq $drive) { - throw "Test cannot continue as no drive letter available" - } - - $dest = (Resolve-Path -Path $TestDrive).ProviderPath - $null = New-Item -ItemType Directory -Path $dest -Name test - $out = subst $drive $dest 2>&1 - if ($LASTEXITCODE -ne 0) { - throw "subst failed with exit code ${LASTEXITCODE} for drive '$drive': $out" - } - - $testdir = New-Item -ItemType Directory -Path $drive -Name testmovedir -Force - 1 > $testdir\test.txt - Move-Item $drive\testmovedir $dest\test - "$drive\testmovedir" | Should -Not -Exist - "$dest\test\testmovedir\test.txt" | Should -FileContentMatchExactly 1 - } - finally { - subst $drive /d - } - } - 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" $error[0].Exception | Should -BeOfType System.IO.IOException @@ -1527,3 +1500,32 @@ Describe "Verify sub-directory creation under root" -Tag 'CI','RequireSudoOnUnix $dirPath | Should -Exist } } + +Describe "Windows admin tests" -Tag 'RequireAdminOnWindows' { + It "Verify Move-Item for directory across drives on Windows" -Skip:(!$IsWindows) { + try { + # find first available drive letter, unfortunately need to use both function: and Win32_LogicalDisk to cover + # both subst drives and bitlocker drives + $drive = Get-ChildItem function:[h-z]: -Name | Where-Object { !(Test-Path -Path $_) -and !(Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='$_'") } | Select-Object -First 1 + if ($null -eq $drive) { + throw "Test cannot continue as no drive letter available" + } + + $dest = (Resolve-Path -Path $TestDrive).ProviderPath + $null = New-Item -ItemType Directory -Path $dest -Name test + $out = subst $drive $dest 2>&1 + if ($LASTEXITCODE -ne 0) { + throw "subst failed with exit code ${LASTEXITCODE} for drive '$drive': $out" + } + + $testdir = New-Item -ItemType Directory -Path $drive -Name testmovedir -Force + 1 > $testdir\test.txt + Move-Item $drive\testmovedir $dest\test + "$drive\testmovedir" | Should -Not -Exist + "$dest\test\testmovedir\test.txt" | Should -FileContentMatchExactly 1 + } + finally { + subst $drive /d + } + } +} From 7d4a0f637a643125fe559ad7c4aa0b1210ba5979 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Tue, 23 Mar 2021 16:50:49 -0700 Subject: [PATCH 7/8] resolve Ilya's feedback --- .../namespaces/FileSystemProvider.cs | 52 +++++-------------- 1 file changed, 12 insertions(+), 40 deletions(-) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index 61c7a77e6d0..be53fc75d6c 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -51,15 +51,6 @@ public sealed partial class FileSystemProvider : NavigationCmdletProvider, ISecurityDescriptorCmdletProvider, ICmdletProviderSupportsHelp { -#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 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. // NOTE: The script used to copy file data from session (PSCopyFromSessionHelper) has a // maximum fragment size value for security. If FILETRANSFERSIZE changes make sure the @@ -6074,41 +6065,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 (!IsSameWindowsVolume(directory.FullName, destinationPath)) - { - CopyAndDelete(directory, destinationPath, force); - } - else + try { - try - { - if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory) - { - throw new IOException("Invalid cross-device link", hresult: MOVE_FAILED_ERROR); - } - - directory.MoveTo(destinationPath); - } - catch (IOException e) when (e.HResult == MOVE_FAILED_ERROR) + if (InternalTestHooks.ThrowExdevErrorOnMoveDirectory) { - // 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); + throw new IOException("Invalid cross-device link"); } - } - } - - private static bool IsSameWindowsVolume(string source, string destination) - { -#if UNIX - return true; -#else - FileInfo src = new FileInfo(source); - FileInfo dest = new FileInfo(destination); - return src.Directory.Root.Name == dest.Directory.Root.Name; -#endif + directory.MoveTo(destinationPath); + } + catch (IOException) + { + // 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); + } } private void CopyAndDelete(DirectoryInfo directory, string destination, bool force) From 1a9ea3cbf76214bac4f8f12226591c46a861194d Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL)" Date: Tue, 23 Mar 2021 18:08:38 -0700 Subject: [PATCH 8/8] update test --- .../Microsoft.PowerShell.Management/FileSystem.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 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 5a7a0625bcb..f1b352a16bc 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 }