Replace string-compare-based test for copying to same file with more … by jeffbi · Pull Request #3441 · PowerShell/PowerShell · GitHub

Fix #1930.

Rather than relying on case-insensitive string compares of source and destination paths, use operating system calls to determine whether two paths refer to the same file. This solves not only the case-insensitivity issue but also allows the cmdlet to operate properly if the destination is a hard or symbolic link to the source.

The Windows side is implemented in C#. The Unix side is implemented partially in native code.

jeffbi I updated the description as this seems the fix for #1930, but please correct it if I'm wrong.
Could you please add more description about how your fix works? It will be helpful for reviewing the changes.

The last comment of #1930 shows:

edit: fixed, thanks Ilya (@iSazonov)

Steve Lee (@SteveL-MSFT), do you mean that bug had already been fixed by Ilya (@iSazonov)?

Dongbo Wang (@daxian-dbw) Yes, it is a fix for #1930. I had that at the end of the pull-request title, but it looks like it got clipped off.

I've updated the description with more information.

I believe Steve Lee (@SteveL-MSFT)'s comment referred to fixing the wording on an earlier comment.

Dongbo Wang (@daxian-dbw)

Steve Lee (@SteveL-MSFT), do you mean that bug had already been fixed by Ilya (@iSazonov)?

It was just a typo.

I tested

Test-Path /etc/sysctl.conf
Test-Path /etc/sysctl.CONF

on WSL and Test-Path is case-sensitive. Do we really need additional code here?

Dongbo Wang (@daxian-dbw) yeah, the 'fixed' statement referred to a typo in the comment Ilya (@iSazonov) caught, nit that the issue was fixed

Ilya (@iSazonov) The goal of the original code was to ensure that the cmdlet did not attempt to copy a file onto itself, and it did so by doing a case-insensitive compare of the two normalized paths. That worked fine on FAT volumes, on NTFS volumes under Windows in its default mode, and on HFS+ volumes on OS X. In your example, it would consider /etc/sysctl.conf and /etc/sysctl.CONF to be the same file and would not allow the copy. On a case-sensitive file system such as ext under Linux, or NTFS under either Linux or OS X, those two paths are generally not the same file, so the copy should be allowed to continue, which is the complaint in issue #1930. I'm fairly confident that if you ran copy-item /etc/sysctl.conf /etc/sysctl.CONF under WSL it would fail with an error saying it cannot copy the file onto itself.

Then consider links. If file A is a file on a volume, and file B is a hard or symbolic link to file A, copying A to B would actually be copying A onto itself, which the cmdlet does not allow but cannot be detected by merely comparing two path strings.

The new code determines whether two paths refer to the same file (the goal of the original code) regardless of what the paths appear to be. It works on HFS+, ext, FAT, and NTFS file systems. For NTFS file systems, it does the right thing under Windows, where the volume is case-insensitive, and on Mac/Linux where the volume is case-sensitive. And it works for hard and symbolic links on those file systems that support them.

My question was only whether we can use existing code without new low level code?
I believe yes.
Currently .Net Core works uniformly on all platforms with hard and soft links. (We can even remove this code for Unix and perhaps even more code can be replaced with .Net Core code there)

Name           : powershell
Length         : 0
CreationTime   : 3/21/2017 11:18:52 AM
LastWriteTime  : 3/21/2017 11:18:52 AM
LastAccessTime : 3/21/2017 11:18:52 AM
Mode           : -a---l
LinkType       : SymbolicLink
Target         : C:\Users\sie\Documents\GitHub\iSazonov\PowerShell\debug\powershell.exe
VersionInfo    : File:             C:\Users\sie\Documents\GitHub\iSazonov\PowerShell\debug\powershell

Here Target contains true value for soft and hard links. We can use Mode or LinkType to detect links.
Another option is [Microsoft.PowerShell.Commands.InternalSymbolicLinkLinkCodeMethods]::GetLinkType

So here we can make the code more simple.

We should create a new API to compare path and place it in engen\Utils.cs or PathUtils.cs

This works differently in Linux.

PS> new-item junk.txt -value "stuff"
PS> get-item j* | select name,mode,attributes,length,linktype,target
Name       : junk.txt
Mode       : ------
Attributes : Normal
Length     : 5
LinkType   : 
Target     : 

PS> new-item -itemtype symboliclink -path junk-sym.txt -value junk.txt 
PS> new-item -itemtype hardlink -path junk-hard.txt -value junk.txt
PS> get-item j* | select name,mode,attributes,length,linktype,target
Name       : junk-hard.txt
Mode       : -----l
Attributes : Normal
Length     : 5
LinkType   : HardLink
Target     : 

Name       : junk-sym.txt
Mode       : -----l
Attributes : ReparsePoint
Length     : 24
LinkType   : SymbolicLink
Target     : {/home/jeff/junk/junk.txt}

Name       : junk.txt
Mode       : -----l
Attributes : Normal
Length     : 5
LinkType   : HardLink
Target     :

Here, the symbolic link points to its target, but the hard link does not. Also, once the hard link was created, the original file became a hard link as well. Linux (or the ext file system) does not distinguish between a hard link and the "original" file.

A slightly off-topic question: Note that the symbolic link's attributes property says "ReparsePoint". That makes sense for Windows, but does it for Linux?

jeffbi Sorry for mesh in my previous comment.

This works differently in Linux.

I believe we should fix this locally - behavior on all platforms must be the same. So my suggestion is don't block the PR, continue the code review and open new Issue (if you confirm that we have inconsistency in file provider with soft/hard links on different platforms).

Is this being reviewed, or is it awaiting something from me?

I originally thought that we already have full compliance with links on all platforms as associated problems has been closed both locally and in CoreFX. As you pointed out it is not. I propose to open a new issue and collect the inconsistencies there. Anyway we need to fix it now or later. I need your help with this on Unix.
I suppose this is the preferred way to reduce the places where we use low level API, PInvoke and "If $Platform".
If we can fix Target, then it will be a more fundamental and useful solution and this will simplify many things including this PR (yes?). If it will be difficult to fix (Although I am assuming that your low-level code would simply be used in another API), we will take the PR "as is". Confirm if I don't wrong again and you agree with the road map.

Dongbo Wang (@daxian-dbw) Could you please review my thoughts?

Ilya (@iSazonov) I don't think we can "fix" Target, at least for Unix. Short of walking the file system, like the UNIX find command does, I don't believe there is a way to discover all the hard links to a given inode. An inode has a count of links that refer to it, but not a list of the actual links.

My low-level code does not attempt to find all paths to an inode, just whether two paths point to the same inode. There is an open request issue in corefx (dotnet/corefx#10120) to implement this in corefx.

In PowerShell 5.1 on my Windows 10 box, the Target property contains a collection of other hard links to the same file. On PowerShell for CoreCLR, Target is empty. The code for discovering the links on Windows is #if-ed out, with a comment saying the Windows API functions FindFirstFileName and FindNextFileName are not supported in CoreCLR. Is that no longer the case? I re-enabled that code, using the appropriate DLL name from here, and I'm able to populate Target on CoreCLR. That code, though, does have another instance of case-insensitive path comparison, which is what started this PR in the first place.

I have come across a few other issues that are related to links, one in New-Item and two that directly affect #621 (the latter two are, I believe, corefx issues: see dotnet/corefx#17843 and dotnet/corefx#17844). Do we want to create a single issue as a catch-all for link-related items?

jeffbi Great ** 2! 😄 Thanks!
I see that:

We cannot fix Targets on Unix (at least effectively). Now we leave it as is and wait CoreFX solution.
We should open new Issue to fix targets on PowerShell Core on Windows to get the same behavior as Windows PowerShell (for NTFS).
We would open new Meta-Issue with your research results. (Although it is necessary to comprehend the already open ones )

Sorry for the delay of the PR.

Please add tests.

Please renew the comments.

Fixed

path_two?

Fixed

Please correct the comment.

Fixed

I believe it works for both files and directories. Parameters is "path" too. So maybe rename to IsSamePath?

I agree that this could use a better name, since it does work for both files and directories, but I'm not sure that IsSamePath is the best choice. I avoided IsSamePath because two different paths can arrive at the same destination. I went with IsSameFile just because the docs for the stat function and the GetFileInformationByHandle both just use the term "file" even though they both work for files and directories.

Maybe good comments (on high level API too) and/or IsSameTarget or something like that?

Renamed to IsSameFileSystemItem

The same as for IsSameFile - maybe rename to WinIsSamePath

Renamed to IsSameFileSystemItem

Please remove the comment or make it full and clear.

Fixed

Can we use NativeMethods.CreateFile here?

Not really, but that code could have been a lot better. I've updated it.

Unneeded formatting. Please renew the comments.

Fixed

Unneeded formatting. Please renew the comments.

Fixed

Dongbo Wang (@daxian-dbw) This has been waiting for approval/merge of PR #3509 because the Pester tests should be using New-Item to create directory symbolic links.

I can get around that by shelling out to cmd.exe to run the mklink program, but that would mean revisiting the test script once the PR has been merged. Perhaps I could open a new issue to update the test.

Would this be acceptable?

jeffbi Thanks for letting me know. I noticed that Ilya (@iSazonov) has signed off #3509. I will quickly go through it and merge the PR if I don't spot anything obvious.

Dongbo Wang (@daxian-dbw) I think there are discussions required for #3509 regarding the proper way to create symlinks in various circumstances under Windows, and that that's why approval/merging of that PR is taking a while.

jeffbi #3509 has been merged. Please sync and carry on with your test changes for this PR.

…accurate, cross-platform test (#1930) Changes per code review * Cleaned up comments in .cpp file * Renamed IsSameFile to IsSameFileSystemItem * Removed comment about PosixSemantics * Added PosixSemantics to CreateFile call * Provide additional information in exception when paths point to same destination. * Added tests

Usually we set -Tags @('CI', 'RequireAdminOnWindows').

Please use ShouldBeErrorId

$exc = {
     ... } | ShouldBeErrorId ...
$exc.Exception | Should BeOfType System.IO.IOException
...

If previous Should Be is failed the cleanup code will be unreacheable.
Please move the cleanup code to AfterEach.

With moving to 'RequireAdminOnWindows' I believe we can use It "" -TestCase $testCases directly without TestSelfCopy

It distracts attention. We should make separate PR for formatting.

Extra line.

Discarded.

returns what?
Below the same.

Please correct this.

Extra line.

Discarded.

Ilya (@iSazonov) In response to your review:

RequireAdminOnWindows: I knew about this, but decided against it in this case.
- Whether a test requires elevation on Windows is not about the test itself, but about the precondition for the test---the test can run without elevation, but it requires elevation (on Windows) to create the symbolic links the test works with.
- Using this tag would mean that we would need a separate Describe for almost every test, in some cases with the tests duplicated only because we couldn't do part of the setup without elevation. If I understand this tag correctly, this would also require creating identical tests that do no have this tag so they can be run on Unix, and possibly coming up with a mechanism for ensuring that only one of those tests is run. This seems to me to be a lot of extra code for little gain. Since all these tests are validating the same basic thing, I thought they should be contained in a single Describe.
Using It "" -TestCase $testCases: If we end up having multiple Describe blocks as discussed above, using the $testCases might not be practical since we'd probably end up with the logic contained within the It block in each Describe block.
Remove-Item after the ShouldBe: You're right about that, of course. However, we can't move the Remove-Item $destination into the AfterAll block because most of the time the source and the destination object are the same object and removing it would likely cause the next test to fail because its source object no longer exists. I can, though, do something like this:

$pathExists = Test-Path $testCase.Destaination
Remove-Item -Path $testCase.Destination -Force -ErrorAction SilentlyContinue
$pathExists | Should Be $true

Native code: For consistency I modeled the coding style of the existing isdirectory.h and isdirectory.cpp files. I'm happy to make your suggested changes if you prefer.

RequireAdminOnWindows

We don't test Windows behavior, we test PowerShell behavior. So the tests is the same for elevated and non-elevated sessions. In other words if you perform these tests with RequireAdminOnWindows, we will not miss any bug. So we can safely put RequireAdminOnWindows tag.

Unix tests is not skipped if RequireAdminOnWindows present!

Using It "" -TestCase $testCases

Based on my comments we can use this.

Remove-Item after the ShouldBe

My suggesion is AfterEach not AfterAll. The Remove-Item is in foreach and it is executed for every test case.

Native code

I agree with using current pattern.

Please clarify - do we really need PosixSemantics here?

That's for if/when we find ourselves in running in a Windows environment in which case-sensitivity has been turned on. Since the purpose of the function is to see if two files/directories are the same, it made sense to me to give it every advantage in making a correct assessment.

Ilya (@iSazonov) I've re-worked the structure of the tests.

The Travis CI build failure puzzles me. It appears to have succeeded on Linux but failed on OS X.

It appears that the offending line is

Remove-Item -Path $destinationPath -Force -ErrorAction SilentlyContinue

and it looks to me like it's ignoring the -ErrorAction SilentlyContinue part. Have I missed something?

jeffbi That error seems to indicate a bug in the product code. The error string is from FileSystemProviderStrings.resx:

  <data name="ItemDoesNotExist" xml:space="preserve">
    <value>An object at the specified path {0} does not exist.</value>
  </data>

It looks to me an exception with this resource string got thrown. Searching where this resource string is used in code might lead you somewhere.

jeffbi Sorry, I was pretty sure that we can already use the template:

$exc = { .. } | ShouldBeErrorId "..."

but we still blocked by #3456
😕 I asked PS team to help.

This is unneeded function because RequireAdminOnWindows ensures elevated rights.

Function removed, along with use of $canSymLink

This comment is obvious and useless. Please remove.

Fixed.

This comment is obvious and useless. Please remove.

Fixed.

We cannot use -ErrorAction SilentlyContinue with ShouldBeErrorId and we should use script block:

{ Copy-Item -Path $sourcePath -Destination $destinationPath -ErrorAction Stop } | ShouldBeErrorId "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand"

The template is:

$exc = {
   { Copy-Item -Path $sourcePath -Destination $destinationPath -ErrorAction Stop } | ShouldBeErrorId "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand" 
}
$exc.Exception.Data[$selfCopyKey] | Should Not Be $null

I've changed the test code to use $Error[0] rather than $_. I'll gladly update to use your template when ShouldBeErrorId returns something other than Boolean.

The same about comment.

Dongbo Wang (@daxian-dbw) Thanks for the suggestion. I'll take a look tomorrow. It's odd because I've run the Pester tests on all three platforms, and run the Remove-Item with -ErrorAction SilentlyContinue alone, and they all work.

It is not used and can be removed.

Please clarify 'ShouldBeErrorId' below don't fail if the items don't exist?
If failed please remove the "if-return".
If not failed we should replace the if-return with Should.

Removed if for source file existence.

Ilya (@iSazonov) Can you look this over please?

Oh, we shouldn't skip tests silently 😕
Maybe define the test body (below in "It" block) as function and make two "It" blocks (second "skip on non-Windows")?

Done.

Please correct https://github.com/PowerShell/PowerShell/pull/3441/files#r111716269

Revise comments in C++ code.

C++ comments updated

jeffbi I don't see last commit (with C++)

Ilya (@iSazonov) Commit 19702a0

Thanks! I see now.

LGTM.

Ilya (@iSazonov) Thanks for the review! 👍

jeffbi Do you specifically choose a work with the most sophisticated tests? 😄

jeffbi Ilya (@iSazonov) This is awesome!!! This is not a trivial issue to resolve, thank you both very much for the great fix and the thorough review discussion!

…ointing the same file (PowerShell#3441) Rather than relying on case-insensitive string compares of source and destination paths, use operating system calls to determine whether two paths refer to the same file. This solves not only the case-insensitivity issue but also allows the cmdlet to operate properly if the destination is a hard or symbolic link to the source. The Windows side is implemented in C#. The Unix side is implemented partially in native code.

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Mar 28, 2017

Dongbo Wang (daxian-dbw) requested a review from Ilya (iSazonov) March 28, 2017 01:03

Dongbo Wang (daxian-dbw) self-assigned this Mar 28, 2017

Ilya (iSazonov) reviewed Apr 4, 2017

View reviewed changes

jeffbi mentioned this pull request Apr 11, 2017

New-Item -ItemType SymbolicLink folder cannot be opened #2915

Closed

Dongbo Wang (daxian-dbw) added the Review - Waiting on Author label Apr 11, 2017

Ilya (iSazonov) mentioned this pull request Apr 14, 2017

Fix New-Item to create correct symlink type (#2915) #3509

Merged

Ilya (iSazonov) reviewed Apr 17, 2017

View reviewed changes

Ilya (iSazonov) reviewed Apr 18, 2017

View reviewed changes

Update tests per code review

3263893

Fix Pester test for Windows

a1e905b

Ilya (iSazonov) reviewed Apr 19, 2017

View reviewed changes

More test changes per code review

9fff05b

Ilya (iSazonov) reviewed Apr 20, 2017

View reviewed changes

More changes per code review, and fix Travis CI failure on macOS

8814054

Ilya (iSazonov) reviewed Apr 21, 2017

View reviewed changes

Jeff Bienstadt added 2 commits April 21, 2017 02:13

Move Windows-only test into separate It block.

19702a0

Revise comments in C++ code.

Move test body from It block into function.

07b1594

Dongbo Wang (daxian-dbw) merged commit 8183732 into PowerShell:master Apr 21, 2017

jeffbi deleted the copy-item-1930 branch April 21, 2017 23:50

Ilya (iSazonov) removed the Review - Waiting on Author label Apr 22, 2017

Joey Aiello (joeyaiello) mentioned this pull request May 15, 2017

File extension and path checks need to take into account case-sensitive file systems #3218

Closed

Ilya (iSazonov) mentioned this pull request May 12, 2018

Should we do path normalizations on Unix well as on Windows? #6833

Closed

@@ @@ -0,0 +1,48 @@ @@
+              //! @file issamefilesystemitem.cpp
+              //! @author Jeff Bienstadt <v-jebien@microsoft.com>
+              //! @brief returns if two paths ultimately point to the same filesystem object

+                          var access = FileAccess.Read;
+                          var share = FileShare.Read;
+                          var creation = FileMode.Open;
+                          var attributes = FileAttributes.BackupSemantics | FileAttributes.PosixSemantics;

                   BeforeAll {
                       # In Windows, only Administrators can create symbolic links.
                       # In Unix, anyone can.
                       function CanMakeSymlink

                       $isCaseSensitive = $IsLinux
-                      $canSymlink = CanMakeSymlink
-                      $canDirSymLink = $IsWindows -And (CanMakeSymlink)
+                      $canDirSymLink = $IsWindows

+                          # The source and destination paths must exist.
+                          # If either do not, it's because they could not be created in the BeforeAll block
+                          # This is not a test failure, but rather a precondition failure.
+                          if ((-Not (Test-Path -Path $Source)) -or (-Not (Test-Path -Path $Destination)))

+                              $principal = New-Object System.Security.Principal.WindowsPrincipal($WinId)
+                              $admin = [System.Security.Principal.WindowsBuiltInRole]::Administrator
+                              return $principal.IsInRole($admin)

+                              catch
+                              {
+                                  # we expected the copy to fail and it did. make sure it failed the right way
+                                  $_.FullyQualifiedErrorId | Should Be "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand"

+                                  {
+                                      # we expected the copy to succeed. make sure it did
+                                      Test-Path $testCase.Destination | Should Be $true
+                                      Remove-Item -Path $testCase.Destination -Force -ErrorAction SilentlyContinue

+                      # Test the ability to avoid an item copying onto itself
+                      function TestSelfCopy($testCase)
+                      {
+                          It "$($testCase.Name)" -Skip:($testCase.SkipIf) {

		@@ -280,7 +277,8 @@ Describe "Copy-Item can avoid copying an item onto itself" -Tags "CI" {
		# Names of files, directories, and links we'll be using to test

                       $junctionToOther = "$subDir/junction-to-other"
-                      # Set up our directories and links
+                      # Set up our files, directories, links

-                              SelfCopyExpected = $true
+                          else
+                          {
+                              Copy-Item -Path $sourcePath -Destination $destinationPath -ErrorAction SilentlyContinue | ShouldBeErrorId "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand"

+                          {
+                              Copy-Item -Path $sourcePath -Destination $destinationPath -ErrorAction SilentlyContinue | ShouldBeErrorId "CopyError,Microsoft.PowerShell.Commands.CopyItemCommand"
+                              $_.Exception | Should BeOfType System.IO.IOException
+                              $_.Exception.Data[$selfCopyKey] | Should Not Be $null

-                          }
+                  Context "Copy-Item avoids copying an item onto itself" {
+                      BeforeAll {
+                          # Set up our test cases

+                          )
+                          # Junctions and directory symbolic links are Windows and NTFS only
+                          if ($IsWindows)

		$result \| Should Be $level1_0
		}

		#pragma once

		#include "pal.h"


		#include "getstat.h"
		#include "issamefilesystemitem.h"

Replace string-compare-based test for copying to same file with more … by jeffbi · Pull Request #3441 · PowerShell/PowerShell · GitHub

Navigation Menu

Uh oh!

Replace string-compare-based test for copying to same file with more … - #3441

Replace string-compare-based test for copying to same file with more …#3441Dongbo Wang (daxian-dbw) merged 7 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom jeffbi:copy-item-1930Copy head branch name to clipboard

Conversation

jeffbi commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dongbo Wang (daxian-dbw) commented Mar 28, 2017

Uh oh!

jeffbi commented Mar 28, 2017

Uh oh!

Ilya (iSazonov) commented Mar 28, 2017

Uh oh!

Ilya (iSazonov) commented Mar 28, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Mar 28, 2017

Uh oh!

jeffbi commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) commented Mar 28, 2017

Uh oh!

jeffbi commented Mar 28, 2017

Uh oh!

Ilya (iSazonov) commented Mar 29, 2017

Uh oh!

jeffbi commented Mar 30, 2017

Uh oh!

Ilya (iSazonov) commented Mar 31, 2017

Uh oh!

jeffbi commented Apr 3, 2017

Uh oh!

Ilya (iSazonov) commented Apr 4, 2017

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffbi commented Apr 14, 2017

Replace string-compare-based test for copying to same file with more …#3441
Dongbo Wang (daxian-dbw) merged 7 commits into
PowerShell:masterfrom
jeffbi:copy-item-1930

jeffbi commented Mar 28, 2017 •
edited
Loading

jeffbi commented Mar 28, 2017 •
edited
Loading

Ilya (iSazonov) commented Apr 18, 2017 •
edited
Loading