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: Symlink on Windows with a to-be-created directory path silently creates a link of the wrong type #39183

Open
bcmills opened this issue May 21, 2020 · 46 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented May 21, 2020

(Discovered via debugging on #38772.)

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

gopher@SERVER-2016-V7- C:\workdir\go\src>..\bin\go version
go version devel gomote.XXXXX windows/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
gopher@SERVER-2016-V7- C:\workdir\go\src>..\bin\go version
go version devel gomote.XXXXX windows/amd64

gopher@SERVER-2016-V7- C:\workdir\go\src>..\bin\go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\gopher\AppData\Local\go-build
set GOENV=C:\Users\gopher\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\gopher\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\gopher\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\workdir\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\workdir\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set GOAMD64=alignedjumps
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set GOMOD=C:\workdir\go\src\go.mod
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
set GOGCCFLAGS=-m64 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=
C:\Users\gopher\AppData\Local\Temp\go-build141789646=/tmp/go-build -gno-record-gcc-switches

What did you do?

	err := os.Symlink(to, from)
	if err != nil {
		t.Fatalf(…)
	}

	err = os.Mkdir(to, 0755)
	if err != nil {
		t.Fatalf(…)
	}

	…

	file, err := os.Open(from)
	if err != nil {
		t.Fatalf(…)
	}
	file.Close()

CL 234737 contains a complete example.

What did you expect to see?

Either a non-nil error from the call to os.Symlink, or a successful call to os.Open.

What did you see instead?

gopher@SERVER-2016-V7- C:\workdir\go\src>..\bin\go test os -run=TestSymlink.*
--- FAIL: TestSymlinkBeforeTargetFileExists (0.00s)
    os_test.go:908: Open("earlysymlinktestfrom") failed: open earlysymlinktestfrom: Access is denied
.
FAIL
FAIL    os      0.052s
FAIL

Diagnosis

The Windows CreateSymbolicLinkA system call requires a flag to indicate whether the destination is a file or a directory. (If SYMBOLIC_LINK_FLAG_DIRECTORY is set, the symlink is a directory symlink; otherwise, it is a file symlink.)

The current implementation of os.Symlink on Windows uses a call to os.Stat to determine which kind of link to create. Unfortunately, if that Stat call fails, Symlink assumes that it is a file rather than reporting the error to the caller:

go/src/os/file_windows.go

Lines 337 to 338 in 567556d

fi, err := Stat(destpath)
isdir := err == nil && fi.IsDir()

That assumption seems like a mistake. If a Windows user needs to create a symlink to a not-yet-existing file or directory on Windows, they should be made aware that the call is missing essential information (the destination type), and can then make an explicit choice to use golang.org/x/sys/windows.CreateSymbolicLink instead of os.Syscall if they are able to supply the missing information.

I think we should change os.Symlink at the beginning of the Go 1.16 cycle so that it propagates the Stat error instead of implicitly assuming that the destination is a file.

CC @fraenkel @alexbrainman @ianlancetaylor

@gopherbot
Copy link

@gopherbot gopherbot commented May 21, 2020

Change https://golang.org/cl/234112 mentions this issue: go/packages/packagestest: make Export skip tests involving unsupported links

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 21, 2020

Sounds right to me, alas.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented May 21, 2020

That assumption seems like a mistake.

I don't see why it is mistake.

If a Windows user needs to create a symlink to a not-yet-existing file or directory on Windows, they should be made aware that the call is missing essential information (the destination type),

Sure. It is nice to have this information. But I don't see how you can provide information about the file / directory that has not been created yet.

and can then make an explicit choice to use golang.org/x/sys/windows.CreateSymbolicLink instead of os.Syscall if they are able to supply the missing information.

I don't see what you are proposing here. Please try again.

I think we should change os.Symlink at the beginning of the Go 1.16 cycle so that it propagates the Stat error instead of implicitly assuming that the destination is a file.

It looks like Windows CreateSymbolicLink API succeeds without target file or directory, I don't see why we should second guess API intentions and fail os.Symlink.

/cc @mattn since he loves symlinks

Alex

@mattn
Copy link
Member

@mattn mattn commented May 21, 2020

os.Symlink should return normal when the target does not exist. As you say, because Windows symbolic link itself has an attribute (file or directory), I feel that it is nonsense to change it dynamically or to make an empty directory. It is also not atomic. I feel that adding new Symlink or SymlinkTo, which allows us to specify attributes in x/sys/windows, is sufficient. The current os.Symlink is returning a proper error.

It's not possible to simulate all UNIX behavior on Windows perfectly.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 21, 2020

@mattn

The current os.Symlink is returning a proper error.

No, it isn't — and that's the point. os.Symlink is returning nil (indicating successful creation of the link); the error in the test comes from the subsequent os.Open attempting to read through the link.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 21, 2020

@alexbrainman

I don't see what you are proposing here. Please try again.

I am proposing that:

  • When the target does not exist, os.Symlink on Windows should return an explicit error (instead of creating a file-only symlink and returning a nil error as it does today).
  • When the target does exist, os.Symlink on Windows should create a link of the corresponding type (as it does today).

That will eliminate false-negative (nil) returns for symlinks to directories that do not yet exist, at the cost of some false-positive (error) returns for symlinks to files that do not yet exist. However, if the user knows that the link target will eventually be a file, they can use CreateSymbolicLink directly to eliminate the false-positive errors.

I believe that the majority of calls to os.Symlink are for targets that do exist, regardless of type, so I expect the number of false-positive errors in practice to be low.

It looks like Windows CreateSymbolicLink API succeeds without target file or directory,

That is correct.

I don't see why we should second guess API intentions and fail os.Symlink.

The API of os.Symlink is narrower than CreateSymbolicLink. When the link target does not exist, CreateSymbolicLink receives an extra bit of explicit information that os.Symlink lacks: the intended type of the link target, passed as a flag to CreateSymbolicLink but not available to os.Symlink.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 21, 2020

Heh, my investigation into a fix uncovered a related bug for symlinks starting with a slash or backslash (#19937). We were invoking Stat on the wrong path for those, but the bug was masked by the error never being surfaced.

isAbs returns false for such a path, but it is not appropriate to prefix the destination for such a path as we do today:

go/src/os/file_windows.go

Lines 331 to 335 in 567556d

// need the exact location of the oldname when it's relative to determine if it's a directory
destpath := oldname
if !isAbs(oldname) {
destpath = dirname(newname) + `\` + oldname
}

@gopherbot
Copy link

@gopherbot gopherbot commented May 21, 2020

Change https://golang.org/cl/234879 mentions this issue: os: reject Symlink with a nonexistent oldname on Windows

@gopherbot
Copy link

@gopherbot gopherbot commented May 21, 2020

Change https://golang.org/cl/234737 mentions this issue: os: add a unit-test for creating a directory symlink before the directory exists

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 21, 2020

https://golang.org/cl/234879 contains the proposed fix. I think it is telling that none of the test fixes required falling back to CreateSymbolicLink, although that may be necessary for a few test fixes in the x/ repos.

@networkimprov
Copy link

@networkimprov networkimprov commented May 22, 2020

The proposed change would break my app. I create symlinks to non-existent files to implement reliable storage when creating a new file.

Bryan, why shouldn't your use case use x/sys/windows to do this?

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 22, 2020

@networkimprov, if you know that you are creating symlinks to nonexistent files or directories on Windows, then it is indeed possible to use x/sys/windows to create them; see https://go-review.googlesource.com/c/tools/+/234112/9/go/packages/packagestest/export_windows.go.

We could consider an alternate change instead: fix the relative-link bug, but change the documentation for os.Symlink to describe the current behavior of creating file-only symlinks when oldpath does not yet exist. That would maintain support for your Windows-specific use-case, but allow more bugs in portable programs to go undetected, or to be detected only arbitrarily later during execution (at Open rather than Symlink).

On balance, I prefer making code that relies on Windows-specific behavior use a Windows-specific system call explicitly, and returning an error for portable programs when os.Syscall is unable to provide portable behavior.

@networkimprov
Copy link

@networkimprov networkimprov commented May 22, 2020

I also ship my app on MacOS & Linux; it works the same there. I never create files for those symlink targets; the symlinks are placeholders that are removed after the temp file for a new file is fsync()'d.

It's not common to create any symlinks in apps that run on Windows, because it requires Admin privs on Win7. What's your scenario that needs symlinks to future directories?

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 22, 2020

@networkimprov, for the code from which this issue was discovered, it is possible to restructure the code to avoid creating symlinks to nonexistent directories. However, discovering the current “assume file” behavior as the source of the bug was quite difficult due to the lack of error-reporting. It is the lack of error-reporting that needs to be fixed, not the inability to create dangling symlinks.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 22, 2020

The fact that an arbitrary 50/50 guess happens to work for your program does not make it appropriate for os.Symlink to make an arbitrary 50/50 guess.

@networkimprov
Copy link

@networkimprov networkimprov commented May 22, 2020

Could os.Open() correctly report the cause of the failure? "Access denied" isn't "File/Path not found". Maybe it could Stat() the filename on access-denied.
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/

Go on Windows has a variety of filesystem glitches, including:
Rename/Remove() of open files,
FileInfo.Sys() missing fileId,
RemoveAll() with long names down-tree, and
bad syscall.Errno mappings.

I've resorted to patching the stdlib locally. C'est la Go.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 23, 2020

Even if we changed os.Open to try to better diagnose symlinks of the wrong type, that wouldn't make those symlinks any less mysterious if read by non-Go programs.

@networkimprov
Copy link

@networkimprov networkimprov commented May 23, 2020

If your Go program calls the filesystem, it must test that on every OS it supports. This problem is easy to surface, as (IIUC) any attempt to treat a symlink-to-file as a directory yields access-denied, regardless of whether the target exists.

Were it my call, I'd add os.SymlinkNowhere(old, new string, t SymlinkType). That would make changing the current behavior worth considering. Failing that, I'd say we should simply document this for os.Symlink() and add it to the list of GOOS=windows filesystem glitches.

Sadly, even in Win10, you have to enable "Developer Mode" to create symlinks without admin privs, so they'll probably remain uncommon on Windows.
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinka

cc @jstarks in case John has any thoughts...

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented May 26, 2020

@bcmills

I don't see why we should second guess API intentions and fail os.Symlink.

The API of os.Symlink is narrower than CreateSymbolicLink. When the link target does not exist, CreateSymbolicLink receives an extra bit of explicit information that os.Symlink lacks: the intended type of the link target, passed as a flag to CreateSymbolicLink but not available to os.Symlink.

But that is not what symlinks created with CreateSymbolicLink API do. For example, they could point to a file on another volume / drive, and that drive might not be mounted all the time. It is obviously fine for symlinks point to non-existing file / directory.

Did you read

https://docs.microsoft.com/en-us/windows/win32/fileio/hard-links-and-junctions

?

Alex

@jstarks
Copy link

@jstarks jstarks commented May 26, 2020

I don't have any creative solutions for this problem, unfortunately. We run into a similar issue when trying to determine what symlink type to create when creating symbolic links in WSL. If the target doesn't exist, or if the target type changes after creating the symlink, then we get it wrong. This breaks symlink accesses from Windows (but not from WSL since we interpret the symlinks there, ignoring the type flag).

Ideally we would change Windows to not require this distinction, but so far we don't think that's practical.

@gopherbot
Copy link

@gopherbot gopherbot commented May 26, 2020

Change https://golang.org/cl/235281 mentions this issue: go/packages/packagestest: don't assume that os.Symlink can create links to nonexistent files

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 26, 2020

@alexbrainman

But that is not what symlinks created with CreateSymbolicLink API do. For example, they could point to a file on another volume / drive, and that drive might not be mounted all the time. It is obviously fine for symlinks point to non-existing file / directory.

Yes. But they still need to point to the correct type of non-existing target — a file (0x0), or directory (SYMBOLIC_LINK_FLAG_DIRECTORY) — which os.Symlink does not allow the caller to specify. The problem is that the os.Symlink API does not encode enough information to set that flag correctly for nonexistent targets. (The symlink can be created with the correct flag by calling CreateSymbolicLink directly — but that requires information from the caller that os.Symlink does not receive.)

Did you read
https://docs.microsoft.com/en-us/windows/win32/fileio/hard-links-and-junctions

I don't think that document is relevant to this issue..? This issue is specifically about symbolic links, not junctions or hard-links. (I haven't checked whether an analogous bug exists for os.Link, but I have no reason to suspect that it would.)

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 26, 2020

@networkimprov

This problem is easy to surface, as (IIUC) any attempt to treat a symlink-to-file as a directory yields access-denied, regardless of whether the target exists.

It is empirically easy to detect the symptom of this problem (the Access is denied error) but it is difficult to trace that symptom to the underlying cause — and all too tempting for someone unfamiliar with the nuances of Windows to dismiss the problem as “Windows is flaky” instead of “os.Symlink lied about creating a usable symlink”. The change I propose is to surface the problem at the call that introduces it, instead of deferring detection of the problem to potentially an entirely separate process that tries to consume the link down the line.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 26, 2020

@networkimprov

Were it my call, I'd add os.SymlinkNowhere(old, new string, t SymlinkType). That would make changing the current behavior worth considering. Failing that, I'd say we should simply document this for os.Symlink() and add it to the list of GOOS=windows filesystem glitches.

os.SymlinkNowhere seems misnamed, because it would spuriously succeed even when old exists, or would incur needless extra overhead on all other platforms to ensure that the target does not exist.

But to be honest I don't think we even need to expand the os API for this. In most cases, it is trivial to ensure that the link target exists (and is the correct type) before constructing the symlink, and the os package does not in general try to cover portability problems for unusual cases.

For the few programs that actually do need to create symlinks to nonexistent targets, windows.CreateSymbolicLink seems sufficient, and it's easy enough for users to write their own portable wrappers using +build constraints.

@gopherbot
Copy link

@gopherbot gopherbot commented May 26, 2020

Change https://golang.org/cl/235318 mentions this issue: os: move TestStatSymlinkLoop out of the Windows-only test file

@gopherbot
Copy link

@gopherbot gopherbot commented May 26, 2020

Change https://golang.org/cl/235317 mentions this issue: os: in Symlink, stat the correct target path for drive-relative targets on Windows

@networkimprov
Copy link

@networkimprov networkimprov commented May 26, 2020

it is difficult to trace that symptom to the underlying cause

I did suggest documenting the cause!

Since you're proposing to contract the os API, I don't see why expanding it is off the table :-) Go offers a lot of things from Unix that Windows can't do.

Anyway, you have my views, I'll stop repeating myself.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 27, 2020

Thinking about this some more: even if we decide to retain the current os.Symlink behavior for nonexistent targets, I think it is important that we change it to propagate Stat errors for which os.IsNotExist returns false.

Otherwise, it would be possible to successfully create a directory on a network share, then lose the connection to that share, then “successfully” create a symlink to that directory, and end up with a symlink that is still permanently broken once the share is reconnected.

@networkimprov
Copy link

@networkimprov networkimprov commented May 27, 2020

Symlink currently proceeds even if Stat got an error on the target that's not IsNotExist(err)?

Sounds like a bug; in file utils, I always return an unexpected error, in case the filesystem is hosed!

@gopherbot
Copy link

@gopherbot gopherbot commented May 27, 2020

Change https://golang.org/cl/235420 mentions this issue: os: document Symlink idiosyncracies on Windows

gopherbot pushed a commit that referenced this issue May 28, 2020
…ts on Windows

Previously, when the target (“old”) path passed to os.Symlink was a
“root-relative” Windows path,¹ we would erroneously prepend
destination (“new”) path when determining which path to Stat,
resulting in an invalid path which was then masked by the lack of
error propagation for the Stat call (#39183).

If the link target is a directory (rather than a file), that would
result in the symlink being created without the
SYMBOLIC_LINK_FLAG_DIRECTORY flag, which then fails in os.Open.

¹https://docs.microsoft.com/en-us/windows/win32/fileio/creating-symbolic-links

Updates #39183

Change-Id: I04f179cd2b0c44f984f34ec330acad2408aa3a20
Reviewed-on: https://go-review.googlesource.com/c/go/+/235317
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented May 29, 2020

@bcmills

But that is not what symlinks created with CreateSymbolicLink API do. For example, they could point to a file on another volume / drive, and that drive might not be mounted all the time. It is obviously fine for symlinks point to non-existing file / directory.

Yes. But they still need to point to the correct type of non-existing target — ...

The target type can change after symlink is created. What do you propose to do about that?

The target can get deleted after symlink is created. What do you propose to do about that?

See this test https://play.golang.org/p/APh87cnpUgC - it works fine at this moment on both Linux and Windows. It will fail on Windows after your proposed change.

Symlinks currently work independent of their target - that is how they are designed. You propose to break that design to fix file / directory flag on Windows. I say it is not worth it. Linux has the same problem where symlink stops working when target is removed / does not exist. That is what everyone expects. I consider file / directory flag bug to be in the same boat as symlink pointing to non-existing target.

Did you read
https://docs.microsoft.com/en-us/windows/win32/fileio/hard-links-and-junctions

I don't think that document is relevant to this issue..? This issue is specifically about symbolic links, not junctions or hard-links. (I haven't checked whether an analogous bug exists for os.Link, but I have no reason to suspect that it would.)

I provided this document to point to hard links description. I do not know what you are building, but maybe you should use hard links instead. Hard links won't work without targets. That is what you want. Don't you?

Thinking about this some more: even if we decide to retain the current os.Symlink behavior for nonexistent targets, I think it is important that we change it to propagate Stat errors for which os.IsNotExist returns false.

Otherwise, it would be possible to successfully create a directory on a network share, then lose the connection to that share, then “successfully” create a symlink to that directory, and end up with a symlink that is still permanently broken once the share is reconnected.

This is already happening now. Imagine you create symlink to a file, and then replace file with directory of the same name. We cannot do anything about it. Why should we worry about symlinks to be correct at the moment of their creation?

Alex

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 29, 2020

The target type can change after symlink is created. What do you propose to do about that?

The target can get deleted after symlink is created. What do you propose to do about that?

I propose that functions should report a non-nil error when they do not have enough information to perform the operation that the user requested. If they have enough information to perform the operation, and do so successfully, then what happens afterward is irrelevant.

os.Symlink, as documented today, “creates newname as a symbolic link to oldname.”

On POSIX systems, os.Symlink does not need to report an error for missing targets, because the arguments passed to os.Symlink contain all of the information needed to construct a working symbolic link to oldname, regardless of when oldname is created and with what type.

On Windows, the arguments passed to os.Symlink do not contain all of the information needed to construct a working symbolic link to oldname. os.Symlink needs additional information, which it can usually (but not always) obtain from the filesystem itself via os.Stat.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 29, 2020

See this test https://play.golang.org/p/APh87cnpUgC - it works fine at this moment on both Linux and Windows. It will fail on Windows after your proposed change.

If a real-world program creates a symlink without ever attempting to read or write through that symlink, then there is likely a much deeper problem with that program, or a way that the program could be expressed more clearly without the use of create-only symlinks.

(I am aware of exactly one program that intentionally uses create-only symlinks — the one mentioned by @networkimprov earlier.)

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 29, 2020

I provided this document to point to hard links description. I do not know what you are building, but maybe you should use hard links instead. Hard links won't work without targets. That is what you want. Don't you?

Please bear in mind that among our Gopher values is to “be charitable”. If you don't know what I am building, then ask — or read the issues and CLs to which I have already referred to see for yourself what they are doing.

What I want is to be able to more easily debug failures in programs that unintentionally create symbolic links of the incorrect type, such as golang.org/x/tools/go/packages/packagestest prior to CL 234112.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 29, 2020

This is already happening now. Imagine you create symlink to a file, and then replace file with directory of the same name. We cannot do anything about it. Why should we worry about symlinks to be correct at the moment of their creation?

The current behavior reacts to what could well be a temporary failure (say, a network outage, or an expired credential) by constructing a permanently-broken symlink — one that will not work even once the temporary failure has been resolved.

The os.Symlink implementation for POSIX systems does not have that behavior. If the link target exists but is unavailable when the symlink is created, then the link will be correct and fully usable whenever the link target becomes available again.

@networkimprov
Copy link

@networkimprov networkimprov commented May 29, 2020

FWIW, targetless symlinks serving as placeholders may indeed be accessed, and return file-not-found, indicating that there isn't a usable file at the symlink location.

I rather doubt I'm the only developer using this feature; it's been viable for decades. Perhaps I'm the only one doing it in Go on Windows, since Windows symlinks are inconvenient to create.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented May 31, 2020

@bcmills

I din't think we are making progress in our discussion, because we are repeating ourselves. I will just sum up my position here and let others decide.

I agree there is a problem here, that Go can create symlink of wrong type. You propose to solve this problem by verifying target file type during symlink creation.

I don't see how your solution is an improvement.

Your change will break perfectly valid Go programs, like https://play.golang.org/p/APh87cnpUgC

You change will make some programs - https://play.golang.org/p/APh87cnpUgC - behave differently on different OS.

Your change will still allow for symlinks of wrong type to exists. The program cannot control what happens to the target file. So, if someone removes abc directory, and creates abc file after symlink to abc was created, the symlink will still be broken.

I do not know what you are building, but maybe you should use hard links instead. Hard links won't work without targets. That is what you want. Don't you?

Please bear in mind that among our Gopher values is to “be charitable”. If you don't know what I am building, then ask — or read the issues and CLs to which I have already referred to see for yourself what they are doing.

I did ask here. I am trying to understand why you created an issue in the first place. I don't see how I offended you or anyone else here. If I did offend, I apologize.

You said

Symlink assumes that it is a file rather than reporting the error to the caller
go/src/os/file_windows.go

Lines 337 to 338 in 567556d

fi, err := Stat(destpath)
isdir := err == nil && fi.IsDir()
That assumption seems like a mistake. If a Windows user needs to create a symlink to a not-yet-existing file or directory on Windows, they should be made aware that the call is missing essential information (the destination type), and can then make an explicit choice to use

The That assumption seems like a mistake is an opinion, and not fact.

I also looked at CL 234737 and issue 38772. These weren't helpful either.

What I want is to be able to more easily debug failures in programs that unintentionally create symbolic links of the incorrect type, such as golang.org/x/tools/go/packages/packagestest prior to CL 234112.

So you want to make your change to be able to debug easier. Fair enough. But see my points above about downsides of your change.

Alex

@rsc
Copy link
Contributor

@rsc rsc commented Jun 2, 2020

To try to recap the discussion so far:

  • On Unix, symlink(2) creates the link regardless of whether the target exists as a file, exists as a directory, or does not exist. Later accesses work or not depending on whether the target now exists. Whether it is a file or directory doesn't matter.
  • On Windows, CreateSymbolicLink must be told whether the target is or will be a file versus a directory. When the link is later opened, the Windows kernel API checks that the target matches the file/directory bit in the symlink. The Linux emulation (WSL) goes out of its way to ignore that bit and "correct" the answer from open.
  • In Go on Windows, os.Symlink must decide what kind of bit to pass to CreateSymbolicLink - target is file or target is directory. To do that, it stats the target. If the stat succeeds and the target is a directory, os.Symlink makes a directory link. Otherwise (stat fails for any reason, or stat succeeds and target is a file), os.Symlink makes a file link.

There are five possible relevant behavior cases to compare Unix and Windows behavior:

  1. Target already exists and can be stat'ed. Both systems behave the same - os.Symlink succeeds, later accesses succeed.
  2. Target does not exist, later created as file. Both systems behave the same - os.Symlink succeeds, later accesses succeed.
  3. Target does not exist, later created as directory. Both systems have os.Symlink succeed, but later access works on Unix, fails on Windows.
  4. Target cannot be accessed for some temporary reason (maybe permission denied, expired credentials). os.Symlink succeeds on both systems. Later accesses fail on both systems until reason is fixed. Once reason is fixed, later access succeeds on Unix, fails on Windows if target is/was directory (we assumed file).
  5. Target is removed and replaced with opposite kind of thing (file->directory, directory->file) after symlink created. Works on Unix, fails on Windows.

Then the question is whether os.Symlink should fail when the target does not exist or otherwise cannot be accessed. Looking again the cases:

  1. Unix and Windows continue to match no matter what we decide.
  2. Unix and Windows match today but not if we change to reporting the non-exist error.
  3. Unix and Windows disagree today, would keep disagreeing (earlier) if we change.
  4. Unix and Windows agree on "target was really a file" or "target was fundamentally broken" and disagree on "target was really a directory". If we change, they start disagreeing on "target was really a file" too.
  5. Unix and Windows will not match no matter what we do (except maybe fiddle with os.Open, which we won't*).

If we change to failing on any os.Stat error, we introduce new incompatibility between Unix and Windows in case 2 and one sub-case of case 4, and we don't create any new compatibilities.

If we change to failing on any non-os.IsNotExist os.Stat error, we introduce a new incompatibility between Unix and Windows in just the "target was really a file" subcase of case 4.

It seems like documenting the behavior might be the cleanest, most compatible path forward. And by compatible I mean both "compatible with older Go" and "minimizing incompatibilities between the Unix and Windows ports".

Thoughts?

* The philosophy of Go is to avoid deviating from os behavior when possible. So while we could change os.Open to act like WSL and paper over the "incorrect link type" failure, we should not do that. That would make Go programs different from regular non-Go Windows programs, and we try to avoid that. (WSL does not - the whole point of WSL is to be something different.)

It's also worth noting that os.Readlink works any time os.Symlink succeeds - the link is definitely created. It just may not be usable with the current kind of target.

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 2, 2020

Agreed, documentation is the best solution.

Russ, do you believe that someone who needs to create a non-existent directory-target symlink on Windows should resort to x/sys/windows? If I needed that, I'd probably patch my local stdlib, because I'm already patching it for the glitches listed in #39183 (comment)

We could consider an os API AlignSymlink(target, symlink string) error which would fix a broken symlink, and accept constants in target for SymlinkTypeFile and SymlinkTypeDir.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 2, 2020

Thanks for writing down all the cases. I agree that we should just document.

For people using Windows who need to create a symlink to a directory that does not yet exist, I think the approach is either 1) create the directory first; 2) use x/sys/windows.

I don't think we need an AlignSymlink function. It should be reliable to remove the symlink and recreate it. Accessing the symlink won't work while it is removed, but it won't work before it is removed wither.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Jun 2, 2020

  1. Target cannot be accessed for some temporary reason (maybe permission denied, expired credentials). os.Symlink succeeds on both systems. Later accesses fail on both systems until reason is fixed. Once reason is fixed, later access succeeds on Unix, fails on Windows if target is/was directory (we assumed file).
  1. Unix and Windows agree on "target was really a file" or "target was fundamentally broken" and disagree on "target was really a directory". If we change, they start disagreeing on "target was really a file" too.

I think this analysis disregards the possibility of repeatable (or repeating) processes, which are the de-facto standard way to deal with temporary problems such as network failures or expired credentials.

Consider a related case:

4(a). Target cannot be access for some temporary reason, in an idempotent program that (manually or automatically) retries failed operations until either the operation reports success or the link exists. Today, os.Symlink “succeeds” on both systems, but the resulting link is unusable on Windows if the target is a directory.

In contrast, if we propagate non-IsNotExist errors, then the process will succeed immediately on Unix and (provided that the OS returns a correct error code) will retry until success on Windows.

Case 4(a) demonstrates that the existing “local compatibility” leads to an incompatibility at the level of the larger program, and the “new incompatibility” of reporting an error instead introduces a new compatibility (in that the two overall programs would then produce equivalent final results).

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 2, 2020

Bryan, how would the program you describe acquire Admin privs on Windows? Or check its privs and exit if not Admin? Check .UID from os/user.Current()?

Currently, users run my app from a .cmd script that escalates its privs via PowerShell. Eventually I'll create an installer to grant the app Admin privs.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Jun 3, 2020

Bryan, how would the program you describe acquire Admin privs on Windows? Or check its privs and exit if not Admin? Check .UID from os/user.Current()?

Windows 10 allows non-Admin users to have the Create symbolic links right. (See https://docs.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/create-symbolic-links.)

That said, it's not too difficult to probe for the symlink capability by checking whether os.Symlink can create a symlink to a known-local target — see src/internal/testenv/testenv_windows.go. (As far as I can tell, an os.Symlink to a file created by the same process within a temp directory should only fail if the disk is full, and AFAIK a full disk should always be indicated by ERROR_DISK_FULL.)

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 3, 2020

From earlier in thread:
Sadly, even in Win10, you have to enable "Developer Mode" to create symlinks without admin privs, so they'll probably remain uncommon on Windows.
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinka

PS: enabling Developer Mode requires Admin privs :-)

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Aug 12, 2020

This issue is currently labeled as early-in-cycle for Go 1.16.
That time is now, so this is a friendly ping so the issue is looked at again.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 18, 2020

We didn't quite resolve this above, but it looks like Ian and I both believe this is a documentation issue and that we shouldn't change anything now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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