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

os: Stat on symlink fails on windows #19922

Closed
alexbrainman opened this issue Apr 11, 2017 · 21 comments
Closed

os: Stat on symlink fails on windows #19922

alexbrainman opened this issue Apr 11, 2017 · 21 comments

Comments

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Apr 11, 2017

What version of Go are you using (go version)?

go version devel +423e7e6 Mon Apr 10 20:57:08 2017 +0000 windows/amd64

What operating system and processor architecture are you using (go env)?

Windows 7

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=c:\dev
set GORACE=
set GOROOT=C:\dev\go
set GOTOOLDIR=C:\dev\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\brainman\AppData\Local\Temp\go-build979456703=/tmp/go-build -gno-record-gcc-switches
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config

What did you do?

You will need 2 computers to reproduce this issue.

On computer 1 run these commands:

mkdir C:\alex\localdir
mkdir C:\alex\mountpoint
net share mountpoint=C:\alex\mountpoint
mklink /J C:\alex\mountpoint\junc c:\alex\localdir

On computer 2 run:

net use x: \\computer1\mountpoint

then run this https://play.golang.org/p/5m0ZwZFB3z program.

What did you expect to see?

I expect to see no output.

What did you see instead?

I see:

panic: GetFileAttributesEx c:\alex\localdir: The system cannot find the path specified.

goroutine 1 [running]:
main.main()
        c:/dev/src/t/main.go:10 +0x6a
exit status 2

It appears Go program on computer 2 fallowing link with the target of c:\alex\localdir that only exists on computer 1.

On computer 2:

C:\>dir x:
 Volume in drive X has no label.
 Volume Serial Number is 1234-4321

 Directory of X:\

10/04/2017  12:46 PM    <DIR>          .
10/04/2017  12:46 PM    <DIR>          ..
10/04/2017  12:46 PM    <JUNCTION>     junc [c:\alex\localdir]
               0 File(s)              0 bytes
               3 Dir(s)   952 bytes free

C:\>dir c:\alex\localdir
The system cannot find the file specified.

C:\>

I have discovered this while investigating issue #18555. Original idea comes from nodejs/node#8897 (comment) Perhaps this issue and #18555 are duplicates. But I am creating new issue just in case there is something different to learn from this. Also it is easier to reproduce - you don't need Docker for Windows.

I have no idea what to do here. In issue #18555 and here link targets are paths that do not exist.

Perhaps os.Stat should not follow symlinks on windows. But that will break some programs. It will break our tests.

On the other hand, I was planning to change windows os.FileInfo.Mode() to return either os.ModeSymlink or os.ModeDir instead of their union (see #17540 (comment) ). That should fix issues #10424, #17540 and #17541. Not sure what to do now.

Also there is filepath.EvalSymlinks to consider.

@rsc @ianlancetaylor what do you think?

Thank you.

Alex

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 11, 2017

SMB2 protocol doesn't support junctions. It only support symlinks.
(i.e. https://msdn.microsoft.com/en-us/library/cc246542.aspx)

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Apr 12, 2017

SMB2 protocol doesn't support junctions. It only support symlinks.

I do not see how your statement is relevant here. Care to explain?
Thank you.

Alex

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 12, 2017

I think this issue is not "Stat fails on all symlinks on network paths",
but "Stat cannot recognize invalid links, thus try to construct invalid paths, then it fails".

https://msdn.microsoft.com/en-us/library/cc246542.aspx says:

  • For an absolute target that is on a remote machine, the server MUST return the path in the format "\??\UNC\server\share..." where server is replaced by the target server name, share is replaced by the target share name, and ... is replaced by the remainder of the path to the target.

  • The server SHOULD NOT return symbolic link information with an absolute target that is a local resource, because local evaluation will vary based on client operating system (OS).<6>

  • For a relative target, the server MUST return a path that does not start with "\". The path MUST be evaluated, by the calling application, relative to the directory containing the symbolic link. The path can contain either "." to refer to the current directory or ".." to refer to the parent directory, and could contain multiple elements.

These constraints seems to remove semantic ambiguities of links on network paths.
In other words, symlinks (and junctions) that don't satisfy above constraints are invalid.
Those cannot be expected to work. We have no way to deal with.
(All junctions don't satisfy the second constraint)

Conversely, we could make the detection algorithm of invalid links in syscall.Readlink from constraints:

diff --git a/src/syscall/syscall_windows.go b/src/syscall/syscall_windows.go
index 19a7deb230..e57a7f8dd5 100644
--- a/src/syscall/syscall_windows.go
+++ b/src/syscall/syscall_windows.go
@@ -1030,17 +1030,38 @@ func Readlink(path string, buf []byte) (n int, err error) {
 				s = s[4:]
 				switch {
 				case len(s) >= 2 && s[1] == ':': // \??\C:\foo\bar
-					// do nothing
+					if PathIsNetworkPath(path) {
+						// invalid link
+					}
 				case len(s) >= 4 && s[:4] == `UNC\`: // \??\UNC\foo\bar
 					s = `\\` + s[4:]
+					if PathIsNetworkPath(path) {
+						if !SameDrive(path, s) { // don't know how implement
+							// invalid link
+						}
+					}
 				default:
 					// unexpected; do nothing
 				}
 			} else {
 				// unexpected; do nothing
 			}
+		} else {
+			if PathIsNetworkPath(path) {
+				if len(s) > 0 && (s[0] == '/' || s[0] == '\\') {
+					// invalid link
+				}
+				for _, r := range s {
+					if r == '.' {
+						// invalid link
+					}
+				}
+			}
 		}
 	case _IO_REPARSE_TAG_MOUNT_POINT:
+		if PathIsNetworkPath(path) {
+			// invalid link
+		}
 		data := (*mountPointReparseBuffer)(unsafe.Pointer(&rdb.reparseBuffer))
 		p := (*[0xffff]uint16)(unsafe.Pointer(&data.PathBuffer[0]))
 		s = UTF16ToString(p[data.SubstituteNameOffset/2 : (data.SubstituteNameOffset+data.SubstituteNameLength)/2])

So I suppose the solution is

  1. define a variant of syscall.Readlink which can detect invalid links. (in internal/windows)
  2. use it in Stat, if we see invalid links, stop following links then return os.FileInfo.
  3. do the same things for filepath.EvalSymlinks

I'm not sure this is worth trying, though.

Thanks.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Apr 13, 2017

Sorry but I don't see how what you suggest helps this issue and #18555.

In other words, symlinks (and junctions) that don't satisfy above constraints are invalid.

If I am trying to os.Stat("x:\junc"), how can I verify "above constraints" to see if x:\junc symlink is "invalid"?

we could make the detection algorithm of invalid links in syscall.Readlink from constraints

Have you actually tried to run your changes against setup described in this issue and #18555 to see if it actually works?

Alex

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 13, 2017

Sorry but I don't see how what you suggest helps this issue and #18555.

I just explained the cause and a possible solution for this issue.
I don't see any relationship between this issue and #18555.
Probably, #18555 is related to NT namespaces.

If I am trying to os.Stat("x:\junc"), how can I verify "above constraints" to see if x:\junc symlink is "invalid"?

Junctions are always invalid on network path. You can check whether x: is network path or not.

Have you actually tried to run your changes against setup described in this issue and #18555 to see if it actually works?

Actually, above code is just an outline. I don't know how to check whether two UNC shares are pointing the same place or not. I tried to implement PathIsNetworkPath and it works as expected.

However, I'm inclined to say this is not worth solving.
For example, if you mount smb2 share on unix, then you may see an invalid link.

www -> C:\Users\Public\www

So, should we handle this case in EvalSymlinks on unix?

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Apr 14, 2017

I just explained the cause and a possible solution for this issue.

I must have missed your explanation. How are you going to fix this current issue?

You can check whether x: is network path or not.

How would I do that?

However, I'm inclined to say this is not worth solving.

I disagree. People always mount network shares locally and use them no different to C:. I do that all the time. I expect os.Stat (and the rest of Go) to work on x:\junc as well as on c:\junc.

Same for #18555 - just because you are running inside of Docker, does not mean your Go tools should stop working. And I suspect we'll see more Docker users in the future, so we better have some plan for that.

Alex

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 14, 2017

How would I do that?

If your drive is UNC path, that is network path. Otherwise, you can use GetDriveType.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa364939(v=vs.85).aspx
If drive type is DRIVE_REMOTE, that is network path.

I disagree. People always mount network shares locally and use them no different to C:. I do that all the time. I expect os.Stat (and the rest of Go) to work on x:\junc as well as on c:\junc.

Sorry, I cannot understand what are you expecting.
Which computer does x:\junc should point? Computer 1 or Computer 2?

Same for #18555 - just because you are running inside of Docker, does not mean your Go tools should stop working. And I suspect we'll see more Docker users in the future, so we better have some plan for that.

If you didn't play with mountvol command yet, I recommend to try it.
I think volume mount points are more like #18555, in the same category (NT namespaces).

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Apr 14, 2017

you can use GetDriveType

So do you propose we call GetDriveType before every os.Stat? What happens if GetDriveType returns DRIVE_REMOTE, what should os.Stat do then?

Sorry, I cannot understand what are you expecting.

I expect os.Stat("x:\junc") to work regardless if x: is local or mounted from network. If os.Chdir("x:\junc") works, so should os.Stat("x:\junc"). Go users shouldn't care (unless they do care) if c:\junc is directory junction on another computer or nt object or whatever.

Which computer does x:\junc should point? Computer 1 or Computer 2?

If you don't understand issue description, you should tell me which step you have failed at.

If you didn't play with mountvol command yet, I recommend to try it.

I have never used mountvol command before. But, I suspect, we might find more problems with our os.Stat. Like I said in the issue descripion - maybe os.Stat shouldn't follow symlinks on Windows - os.Stat knows the path is directory and it should stop there.

Alex

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 14, 2017

You are right. junctions are worked on my environment. I completely misunderstood the situation.
I'm really sorry about taking your time and thank you very much for your patience.

I've investigated junction's behavior by wireshark on macOS (connected to windows).

If the client try to access a file (via SMB2 CREATE request) under the symlink,
the server will returns the target path of the symlink.
Then, the client try to access given target path instead. it's like a URL redirection.
(to be exact, the client can also manually query the target path like os.Readlink)

On the other hand, if the client try to access a file under the junction,
the server will access the target file instead of the client and return result.
So the client's point of view, it's like a transparent proxy.

We can't manually handle junctions in client side, because translations are performed in server side.
It seems that only SMB2 CREATE request can do the right thing here.
And I think the corresponding Win32 API is CreateFile.
Thus If we can implement Fstat, we can solve this problem. I'd like to investigate this idea.
(I think #18555 is a different problem, but my conclusion is same for os.Stat)

For filepath.EvalSymlinks, I think checking drive type is a still valid method.
I said junctions on network paths are invalid, it turned out not true.
To be accurate, values of junctions are invalid. Anyway, we still cannot use those values.
(more accurately, we can use those values if the network path is pointing a local share. so this is a conservative approach)

Thank you.

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 15, 2017

We can't manually handle junctions in client side, because translations are performed in server side.

Actually, I see SMB2 QUERY_INFO response contains a finalized path.
https://msdn.microsoft.com/en-us/library/cc246557.aspx
https://msdn.microsoft.com/en-us/library/cc232099.aspx (via FileAllInformation)
So if we can find the corresponding win32 API, that will be more an ideal solution for EvalSymlinks.

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 15, 2017

@alexbrainman Can you try https://gist.github.com/hirochachacha/9357e505c6d3da0b6baf271258d92652 ?
I don't have two windows now, so I cannot test this now, but it might work.
usage: go run getfilename.go x:\junc

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Apr 15, 2017

Can you try https://gist.github.com/hirochachacha/9357e505c6d3da0b6baf271258d92652 ?

I prints this:

C:\>u:\test x:\junc
\computer1\mountpoint\junc

C:\>

Alex

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 15, 2017

If you create a file inside a junciton, say "x:\junc\foo".
Does u:\test x:\junc\foo print different one?

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Apr 15, 2017

If you create a file inside a junciton, say "x:\junc\foo".
Does u:\test x:\junc\foo print different one?

C:\>echo abc > x:\junc\foo

C:\>u:\test x:\junc\foo
\computer1\mountpoint\junc\foo

C:\>

Alex

@hirochachacha

This comment has been minimized.

Copy link
Contributor

@hirochachacha hirochachacha commented Apr 15, 2017

It seems that this approach (get path via SMB2 QUERY_INFO) isn't useful. Thank you.
(FileNameInformation isn't a relative path from share root)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 27, 2017

CL https://golang.org/cl/41834 mentions this issue.

@jstarks

This comment has been minimized.

Copy link

@jstarks jstarks commented May 9, 2017

@alexbrainman Have you measured the performance of this change? Just recently the msbuild folks switched from GetFileAttributesEx to CreateFile + GetFileTime to ensure they were getting file times from the target of symlinks, and they saw a significant drop in performance (microsoft/msbuild#2052). The internally suggested workaround is to first try with GetFileAttributesEx, and if the file ends up being a reparse point, fall back to the CreateFile path.

If you see the same performance problem here, probably the simple fix is to call Lstat first and fall back to your new code only when ModeSymlink is set.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented May 9, 2017

Just recently the msbuild folks switched from GetFileAttributesEx to CreateFile + GetFileTime to ensure they were getting file times from the target of symlinks, and they saw a significant drop in performance (microsoft/msbuild#2052). The internally suggested workaround is to first try with GetFileAttributesEx, and if the file ends up being a reparse point, fall back to the CreateFile path.

@jstarks thanks for the tip. I will measure and change if required.

Alex

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented May 11, 2017

I will measure and change if required.

The change is https://go-review.googlesource.com/#/c/43071/

Alex

gopherbot pushed a commit that referenced this issue May 19, 2017
Recent CL 41834 made windows Stat work for all symlinks.
But CL 41834 also made Stat slow.

John Starks sugested
(see #19922 (comment))
to use GetFileAttributesEx for files and directories instead.
This makes Stat as fast as at go1.9.

I see these improvements on my Windows 7

name       old time/op  new time/op  delta
StatDot    26.5µs ± 1%  20.6µs ± 2%  -22.37%  (p=0.000 n=9+10)
StatFile   22.8µs ± 2%   6.2µs ± 1%  -72.69%  (p=0.000 n=10+10)
StatDir    21.0µs ± 2%   6.1µs ± 3%  -71.12%  (p=0.000 n=10+9)
LstatDot   20.1µs ± 1%  20.7µs ± 6%   +3.37%  (p=0.000 n=9+10)
LstatFile  6.23µs ± 1%  6.36µs ± 8%     ~     (p=0.587 n=9+10)
LstatDir   6.10µs ± 0%  6.14µs ± 4%     ~     (p=0.590 n=9+10)

and on my Windows XP

name         old time/op  new time/op  delta
StatDot-2    20.6µs ± 0%  10.8µs ± 0%  -47.44%  (p=0.000 n=10+10)
StatFile-2   20.2µs ± 0%   7.9µs ± 0%  -60.91%  (p=0.000 n=8+10)
StatDir-2    19.3µs ± 0%   7.6µs ± 0%  -60.51%  (p=0.000 n=10+9)
LstatDot-2   10.8µs ± 0%  10.8µs ± 0%   -0.48%  (p=0.000 n=10+8)
LstatFile-2  7.83µs ± 0%  7.83µs ± 0%     ~     (p=0.844 n=10+8)
LstatDir-2   7.59µs ± 0%  7.56µs ± 0%   -0.46%  (p=0.000 n=10+10)

Updates #19922

Change-Id: Ice1fb5825defb05c79bab4dec0692e0fd1bcfcd5
Reviewed-on: https://go-review.googlesource.com/43071
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 12, 2017

Change https://golang.org/cl/55250 mentions this issue: path/filepath: re-implement windows EvalSymlinks

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 15, 2017

Change https://golang.org/cl/55612 mentions this issue: path/filepath: re-implement windows EvalSymlinks

gopherbot pushed a commit that referenced this issue Oct 5, 2017
CL 41834 used approach suggested by Raymond Chen in
https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/
to implement os.Stat by getting Windows I/O manager
follow symbolic links.

Do the same for filepath.EvalSymlinks, when existing
strategy fails.

Updates #19922
Fixes #20506

Change-Id: I15f3d3a80256bae86ac4fb321fd8877e84d8834f
Reviewed-on: https://go-review.googlesource.com/55612
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Aug 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.