Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

path/filepath: TestIssue29372 fails on windows-arm builder #29746

Closed
alexbrainman opened this issue Jan 15, 2019 · 15 comments

Comments

@alexbrainman
Copy link
Member

commented Jan 15, 2019

Since 44cf595 windows-arm builder fails with

https://build.golang.org/log/c00262fb40118e6917815b61ce7e1643b702bdc3

--- FAIL: TestIssue29372 (0.02s)
    path_test.go:1398: test#0: want "The system cannot find the path specified.", got %!q(<nil>)
FAIL
FAIL	path/filepath	26.728s

I don't see how TestIssue29372 can fail this way. @jordanrh1 please help to debug this.

We can skip the test on windows-arm, but I would like to understand what is wrong here first.

Thank you.

Alex

@jordanrh1

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

  • On ARM, the temporary file gets created at C:\Data\Users\administrator\AppData\Local\Temp\issue29372940456735.
  • C:\Data is actually a volume mount point.
  • When walkSymlinks() tests to see if C:\Data is a symlink, it finds that it is, and Readlink returns Volume{ae420040-0000-0000-0000-008200000000}.
  • walkSymlinks then tries to Lstat this path, but Lstat fails because the path name is missing the required \\?\ prefix. The path must look like \\?\Volume{ae420040-0000-0000-0000-008200000000}.
  • walkSymlinks then returns an error, which causes evalSymlinks to call evalSymlinksUsingGetFinalPathNameByHandle, which succeeds.
  • Since the test is expecting EvalSymlinks to fail with ENOTDIR, but EvalSymlinks succeeded, the test fails.

I'm looking into Readlink to see if the returned value should be prefixed with \\?\.

@jordanrh1

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

@alexbrainman Do you know why walkSymlinks is used? Why is it not sufficient to use evalSymlinksUsingGetFinalPathNameByHandle ?

After fixing Readlink, walkSymlinks is now succeeding in cases where it didn't before, which causes evalSymlinks not to fall back to evalSymlinksUsingGetFinalPathNameByHandle. evalSymlinksUsingGetFinalPathNameByHandle returns paths with a drive letter when possible, but walkSymlinks returns volume-style names. There is a lot of code that relies on EvalSymlinks to return drive letter style names.

@jordanrh1

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

It is worth noting that none of this is ARM-specific. It is showing up on the ARM builder because %tmp% is a path that contains a volume mount point.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

  • C:\Data is actually a volume mount point. ...

Nice debugging. So %TMP% directory path has symlink in it. And it is symlink into nt namespace. I can reproduce it here to debug this myself. Thank you very much.

Do you know why walkSymlinks is used? Why is it not sufficient to use evalSymlinksUsingGetFinalPathNameByHandle ?

From what I remember, we had walkSymlinks first. And that worked for awhile. But then we discovered symlinks that points to unusual places, like symlink into nt namespace. Just like you have discovered.

We could have adjusted os.Readlink to return these paths as is. But I don't think you could use nt namespace path in all normal Windows APIs. Could you?

So we came up with current approach to use CreateFile+GetFinalPathNameByHandle to implement path/filepath.EvalSymlinks. And that worked, but it works differently in some corner cases. For example, see path/filepath.TestIssue29372. I suspect many others. So, for backward compatibility, we decided to use walkSymlinks first, and, if that fails, use evalSymlinksUsingGetFinalPathNameByHandle. That covers most current scenarios.

After fixing Readlink, walkSymlinks is now succeeding in cases where it didn't before, which causes evalSymlinks not to fall back to evalSymlinksUsingGetFinalPathNameByHandle. evalSymlinksUsingGetFinalPathNameByHandle returns paths with a drive letter when possible, but walkSymlinks returns volume-style names. There is a lot of code that relies on EvalSymlinks to return drive letter style names.

I don't think we could return nt namespace paths. These will not work properly with current os and path/filepath package implementations. But I could be wrong about that. You are welcome to corect me.

It is worth noting that none of this is ARM-specific. It is showing up on the ARM builder because %tmp% is a path that contains a volume mount point.

Yes, I agree. This is not windows-arm specific. Let me think about this for a couple of days. If I won't come up with anything, I will skip the test for go1.13.

Alex

@jordanrh1

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

I don't think we could return nt namespace paths

Are you sure GetFinalPathNameByHandle will return nt namespace paths? It looks like there is an option to request DOS-style paths (VOLUME_NAME_DOS).

@gopherbot

This comment has been minimized.

Copy link

commented Jan 25, 2019

Change https://golang.org/cl/159578 mentions this issue: path/filepath: skip TestIssue29372 on windows, if /tmp has symilinks

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

Are you sure GetFinalPathNameByHandle will return nt namespace paths? It looks like there is an option to request DOS-style paths (VOLUME_NAME_DOS).

I was not talking not about GetFinalPathNameByHandle when I spoke about nt namespace paths. I was talking about os.Readlink implementation.

If you look at syscal.Readlink, you will see that it uses DeviceIoControl(..., FSCTL_GET_REPARSE_POINT ... to find symlink contents. When I use mklink /j tmplink \\?\Volume{....} command to create symlink, and then use DeviceIoControl to fetch that symlink contents, I get \??\Volume{...}\. But syscal.Readlink cannot return \??\Volume{...}\ as is. If syscal.Readlink would return \??\Volume{...}\ as is, then that path might get passed to other Windows API functions. I don't think all Windows API functions we use will work with path like \??\Volume{...}\.

Anyway I will make TestIssue29372 pass for the moment (see https://golang.org/cl/159578). We would have to deal with this later. I don't see any quick solutions here.

Alex

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

Anyway I will make TestIssue29372 pass for the moment (see https://golang.org/cl/159578).

@jordanrh1 I forgotten to ask, if you could verify that https://golang.org/cl/159578 makes TestIssue29372 pass.

Thank you.

Alex

gopherbot pushed a commit that referenced this issue Jan 27, 2019
path/filepath: skip TestIssue29372 on windows, if /tmp has symilinks
TestIssue29372 is broken on windows when temporary directory has
symlink in its path.

Adjust the test to use filepath.EvalSymlinks of temporary directory,
instead of temporary directory on windows. This change is not a
proper fix, but at least it makes TestIssue29372 pass on windows-arm.

See issue for details.

Updates #29746

Change-Id: I2af8ebb89da7cb9daf027a5e49e32ee22dbd0e3d
Reviewed-on: https://go-review.googlesource.com/c/159578
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@bcmills bcmills added this to the Go1.12 milestone Jan 29, 2019

@bcmills bcmills added the Testing label Jan 29, 2019

@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Feb 28, 2019

Change https://golang.org/cl/164202 mentions this issue: path/filepath: do not call GetFinalPathNameByHandle from EvalSymlinks

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@jordanrh1 can you, please, test https://golang.org/cl/164202 on your windows-arm computer, before we submit it.

Thank you.

Alex

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@jhowardmsft FYI, please, see https://golang.org/cl/164202 - hopefully it will be part of go1.13.

Alex

@jhowardmsft

This comment has been minimized.

Copy link

commented Feb 28, 2019

@alexbrainman Some comments on the CL (I don't have an account on googlesource.com)

  • There's multiple references to \??\…. which I believe should be \\?\….. Particularly src/os/file_windows.go L428 in CL164201. See https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file
  • normiliseLinkPath should be normaliseLinkPath (with :GB: spelling 😄)
  • What does normaliseLinkPath return with input (after correction above) for normalizing \\?\Volume\{GUID}\path\to\file when there isn't a valid DOS path for it (such as a mounted volume without a drive letter)? I'd need to look at what GetFinalPathNameByHandle does under those circumstances.
@jhowardmsft

This comment has been minimized.

Copy link

commented Feb 28, 2019

A follow-up comment is when a program calls os.ReadLink explicity using a long filepath such as \\?\C:\very\long\path\to\some\file which is more than 260 characters (MAX_PATH) https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file#maximum-path-length-limitation. Would this still work?

@jordanrh1

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

@jhowardmsft I was also initially confused by \??\ but it is correct in the context in which it's being used.

@alexbrainman TestIssue2937 is now passing. Here is the raw output:

administrator@MINWINPC C:\Data>filepath.test.exe -test.v
=== RUN   TestMatch
--- PASS: TestMatch (0.00s)
=== RUN   TestGlob
--- FAIL: TestGlob (0.78s)
    match_test.go:142: Glob(`match.go`) = []string(nil) want match.go
    match_test.go:142: Glob(`mat?h.go`) = []string(nil) want match.go
    match_test.go:142: Glob(`*`) = []string{"CrashDump", "DedicatedDumpFile.sys"
, "FirstBoot.Complete", "GpioTestTool.exe", "I2cTestTool.exe", "Logfiles", "MinC
omm.exe", "ProgramData", "Programs", "SharedData", "SpiTestTool.exe", "System Vo
lume Information", "SystemData", "Users", "Windows", "filepath.test.exe", "hello
2.exe", "imxuart.sys", "mx6pep.sys", "path.test.exe", "test", "uefi.fit"} want m
atch.go
    match_test.go:142: Glob(`..\*\match.go`) = []string(nil) want ..\filepath\ma
tch.go
=== RUN   TestGlobError
--- PASS: TestGlobError (0.00s)
=== RUN   TestGlobUNC
--- PASS: TestGlobUNC (0.00s)
=== RUN   TestGlobSymlink
--- PASS: TestGlobSymlink (0.04s)
=== RUN   TestWindowsGlob
--- PASS: TestWindowsGlob (0.28s)
=== RUN   TestNonWindowsGlobEscape
--- SKIP: TestNonWindowsGlobEscape (0.00s)
    match_test.go:378: skipping non-windows specific test
=== RUN   TestClean
--- PASS: TestClean (0.00s)
    path_test.go:119: skipping AllocsPerRun checks; GOMAXPROCS>1
=== RUN   TestFromAndToSlash
--- PASS: TestFromAndToSlash (0.00s)
=== RUN   TestSplitList
--- PASS: TestSplitList (0.00s)
=== RUN   TestSplit
--- PASS: TestSplit (0.00s)
=== RUN   TestJoin
--- PASS: TestJoin (0.00s)
=== RUN   TestExt
--- PASS: TestExt (0.00s)
=== RUN   TestWalk
--- PASS: TestWalk (0.05s)
=== RUN   TestWalkSkipDirOnFile
--- PASS: TestWalkSkipDirOnFile (0.02s)
=== RUN   TestWalkFileError
--- PASS: TestWalkFileError (0.03s)
=== RUN   TestBase
--- PASS: TestBase (0.00s)
=== RUN   TestDir
--- PASS: TestDir (0.00s)
=== RUN   TestIsAbs
--- PASS: TestIsAbs (0.00s)
=== RUN   TestEvalSymlinks
--- PASS: TestEvalSymlinks (0.31s)
=== RUN   TestEvalSymlinksIsNotExist
--- PASS: TestEvalSymlinksIsNotExist (0.01s)
=== RUN   TestIssue13582
--- PASS: TestIssue13582 (0.13s)
=== RUN   TestAbs
--- PASS: TestAbs (0.06s)
=== RUN   TestAbsEmptyString
--- PASS: TestAbsEmptyString (0.01s)
=== RUN   TestRel
--- PASS: TestRel (0.00s)
=== RUN   TestVolumeName
--- PASS: TestVolumeName (0.00s)
=== RUN   TestDriveLetterInEvalSymlinks
--- PASS: TestDriveLetterInEvalSymlinks (0.01s)
=== RUN   TestBug3486
--- FAIL: TestBug3486 (0.00s)
    path_test.go:1292: CreateFile /home: The system cannot find the file specifi
ed.
=== RUN   TestWalkSymlink
--- PASS: TestWalkSymlink (0.02s)
=== RUN   TestIssue29372
--- PASS: TestIssue29372 (0.03s)
=== RUN   TestWinSplitListTestsAreValid
--- PASS: TestWinSplitListTestsAreValid (1.47s)
=== RUN   TestWindowsEvalSymlinks
--- PASS: TestWindowsEvalSymlinks (0.10s)
=== RUN   TestEvalSymlinksCanonicalNames
--- PASS: TestEvalSymlinksCanonicalNames (0.09s)
=== RUN   TestEvalSymlinksCanonicalNamesWith8dot3Disabled
--- SKIP: TestEvalSymlinksCanonicalNamesWith8dot3Disabled (0.00s)
    path_windows_test.go:294: skipping test that modifies file system setting; e
nable with -run_fs_modify_tests
=== RUN   TestToNorm
--- PASS: TestToNorm (0.07s)
=== RUN   TestUNC
--- PASS: TestUNC (0.00s)
=== RUN   TestWalkDirectoryJunction
--- PASS: TestWalkDirectoryJunction (0.18s)
=== RUN   TestWalkDirectorySymlink
--- PASS: TestWalkDirectorySymlink (0.14s)
=== RUN   TestNTNamespaceSymlink
--- PASS: TestNTNamespaceSymlink (0.34s)
=== RUN   ExampleExt
--- PASS: ExampleExt (0.00s)
FAIL

administrator@MINWINPC C:\Data>path.test.exe -test.v
=== RUN   TestMatch
--- PASS: TestMatch (0.00s)
=== RUN   TestClean
--- PASS: TestClean (0.00s)
=== RUN   TestCleanMallocs
--- PASS: TestCleanMallocs (0.00s)
    path_test.go:82: skipping AllocsPerRun checks; GOMAXPROCS>1
=== RUN   TestSplit
--- PASS: TestSplit (0.00s)
=== RUN   TestJoin
--- PASS: TestJoin (0.00s)
=== RUN   TestExt
--- PASS: TestExt (0.00s)
=== RUN   TestBase
--- PASS: TestBase (0.00s)
=== RUN   TestDir
--- PASS: TestDir (0.00s)
=== RUN   TestIsAbs
--- PASS: TestIsAbs (0.00s)
=== RUN   ExampleBase
--- PASS: ExampleBase (0.00s)
=== RUN   ExampleClean
--- PASS: ExampleClean (0.00s)
=== RUN   ExampleDir
--- PASS: ExampleDir (0.00s)
=== RUN   ExampleExt
--- PASS: ExampleExt (0.00s)
=== RUN   ExampleIsAbs
--- PASS: ExampleIsAbs (0.00s)
=== RUN   ExampleJoin
--- PASS: ExampleJoin (0.00s)
=== RUN   ExampleMatch
--- PASS: ExampleMatch (0.00s)
=== RUN   ExampleSplit
--- PASS: ExampleSplit (0.00s)
PASS

administrator@MINWINPC C:\Data>

@alexbrainman

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

Some comments on the CL (I don't have an account on googlesource.com)

I will take review comments from anywhere.

  • There's multiple references to \??\…. which I believe should be \\?\….

What @jordanrh1 said. If you run mountvol c: /l command, it returns \\?\…., but if you call DeviceIoControl with FSCTL_GET_REPARSE_POINT, it returns \??\….. Try it. I am confused as you are.

  • normiliseLinkPath should be normaliseLinkPath

Done.

(with :GB: spelling smile)

Thanks. I am actually in Australia, but GB will do.

  • What does normaliseLinkPath return with input (after correction above) for normalizing \\?\Volume\{GUID}\path\to\file when there isn't a valid DOS path for it (such as a mounted volume without a drive letter)? I'd need to look at what GetFinalPathNameByHandle does under those circumstances.

I am pretty sure GetFinalPathNameByHandle will fail. I am pretty sure I tried calling GetFinalPathNameByHandle on Docker mounted volumes from inside container. And it fails. And it is fine with me for now.

A follow-up comment is when a program calls os.ReadLink explicity using a long filepath such as \\?\C:\very\long\path\to\some\file which is more than 260 characters (MAX_PATH) https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file#maximum-path-length-limitation. Would this still work?

If symlink target path is \\?\C:\very\long\path\to\some\file, it should go here https://go-review.googlesource.com/c/go/+/164201/1/src/os/file_windows.go#436

TestIssue2937 is now passing.

Thank you, Jordan, for checking.

Alex

gopherbot pushed a commit that referenced this issue Mar 1, 2019
os: make Readlink work with symlinks with target like \??\Volume{ABCD}\
windows-arm TMP directory live inside such link (see
#29746 (comment) for
details), so symlinks like that will be common at least on windows-arm.

This CL builds on current syscall.Readlink implementation. Main
difference between the two is how new code handles symlink targets,
like \??\Volume{ABCD}\.

New implementation uses Windows CreateFile API with
FILE_FLAG_OPEN_REPARSE_POINT flag to get \??\Volume{ABCD}\ file handle.
And then it uses Windows GetFinalPathNameByHandle with VOLUME_NAME_DOS
flag to convert that handle into standard Windows path.
FILE_FLAG_OPEN_REPARSE_POINT flag ensures that symlink is not followed
when CreateFile opens the file.

Fixes #30463

Change-Id: I33b18227ce36144caed694169ef2e429fd995fb4
Reviewed-on: https://go-review.googlesource.com/c/164201
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@gopherbot gopherbot closed this in 44d3bb9 Mar 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.