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 returns Cannot create a file when that file already exists #53582

Open
zdszxp opened this issue Jun 28, 2022 · 16 comments
Open

os: Symlink returns Cannot create a file when that file already exists #53582

zdszxp opened this issue Jun 28, 2022 · 16 comments
Labels
NeedsInvestigation OS-Windows

Comments

@zdszxp
Copy link

@zdszxp zdszxp commented Jun 28, 2022

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

$ go version
go version go1.16.10 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
$ go env

set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOOS=windows
set GOPROXY=https://proxy.golang.org,direct
set GOSUMDB=sum.golang.google.cn
set GOTMPDIR=
set GOVCS=
set GOVERSION=go1.16.10
set GCCGO=gccgo
set AR=ar
set CC=gcc
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?

I ran this test (go\src\os\os_test.go's TestSymlink):

func TestSymlink(t *testing.T) {
	testenv.MustHaveSymlink(t)

       /-----------------add code begin---------------
        now := time.Now()
	now.Zone()
       /-----------------end----------------------------

	defer chtmpdir(t)()
	from, to := "symlinktestfrom", "symlinktestto"
	file, err := Create(to)
	if err != nil {
		t.Fatalf("Create(%q) failed: %v", to, err)
	}
	if err = file.Close(); err != nil {
		t.Errorf("Close(%q) failed: %v", to, err)
	}
	err = Symlink(to, from)
	if err != nil {
		t.Fatalf("Symlink(%q, %q) failed: %v", to, from, err)
	}
	tostat, err := Lstat(to)
	if err != nil {
		t.Fatalf("Lstat(%q) failed: %v", to, err)
	}
	if tostat.Mode()&ModeSymlink != 0 {
		t.Fatalf("Lstat(%q).Mode()&ModeSymlink = %v, want 0", to, tostat.Mode()&ModeSymlink)
	}
	fromstat, err := Stat(from)
	if err != nil {
		t.Fatalf("Stat(%q) failed: %v", from, err)
	}
	if !SameFile(tostat, fromstat) {
		t.Errorf("Symlink(%q, %q) did not create symlink", to, from)
	}
	fromstat, err = Lstat(from)
	if err != nil {
		t.Fatalf("Lstat(%q) failed: %v", from, err)
	}
	if fromstat.Mode()&ModeSymlink == 0 {
		t.Fatalf("Lstat(%q).Mode()&ModeSymlink = 0, want %v", from, ModeSymlink)
	}
	fromstat, err = Stat(from)
	if err != nil {
		t.Fatalf("Stat(%q) failed: %v", from, err)
	}
	if fromstat.Name() != from {
		t.Errorf("Stat(%q).Name() = %q, want %q", from, fromstat.Name(), from)
	}
	if fromstat.Mode()&ModeSymlink != 0 {
		t.Fatalf("Stat(%q).Mode()&ModeSymlink = %v, want 0", from, fromstat.Mode()&ModeSymlink)
	}
	s, err := Readlink(from)
	if err != nil {
		t.Fatalf("Readlink(%q) failed: %v", from, err)
	}
	if s != to {
		t.Fatalf("Readlink(%q) = %q, want %q", from, s, to)
	}
	file, err = Open(from)
	if err != nil {
		t.Fatalf("Open(%q) failed: %v", from, err)
	}
	file.Close()
}

What did you expect to see?

I expected the test to pass.

What did you see instead?

=== RUN   TestSymlink
    os_test.go:903: Symlink("symlinktestto", "symlinktestfrom") failed: symlink symlinktestto symlinktestfrom: Cannot create a file when that file already exists.
--- FAIL: TestSymlink (0.04s)

If I don't add the relevant code about the time, the test passes, but if I add it, the link can't be created

@zdszxp zdszxp changed the title affected/package: os os/Symlink: Symlink return Cannot create a file when that file already exists. Jun 28, 2022
@bcmills
Copy link
Member

@bcmills bcmills commented Jun 28, 2022

Per the Go release policy, go1.16.10 is no longer supported. Does this issue still reproduce on a supported release or at HEAD?

For that matter, how are you running an os test with local changes using a released Go version? 🤔

What was the exact command line that you ran to run the test?

Some versions of Windows require that the program be run with Administrator privileges to create symlinks; Windows 10 and after also allows other users to create symlinks if the device is in Developer Mode. If symlinks are not enabled, that should cause testenv.MustHaveSymlink to skip the test. What version of Windows are you running, and do you have Developer Mode enabled?

@bcmills bcmills added OS-Windows WaitingForInfo NeedsInvestigation labels Jun 28, 2022
@ianlancetaylor ianlancetaylor changed the title os/Symlink: Symlink return Cannot create a file when that file already exists. os: Symlink returns Cannot create a file when that file already exists Jun 28, 2022
@mattn
Copy link
Member

@mattn mattn commented Jun 28, 2022

The test is passed on my Windows 11 and Go tip.
image

@zdszxp
Copy link
Author

@zdszxp zdszxp commented Jun 29, 2022

Per the Go release policy, go1.16.10 is no longer supported. Does this issue still reproduce on a supported release or at HEAD?

For that matter, how are you running an os test with local changes using a released Go version? 🤔

What was the exact command line that you ran to run the test?

Some versions of Windows require that the program be run with Administrator privileges to create symlinks; Windows 10 and after also allows other users to create symlinks if the device is in Developer Mode. If symlinks are not enabled, that should cause testenv.MustHaveSymlink to skip the test. What version of Windows are you running, and do you have Developer Mode enabled?

yes, this issue can reproduce with the latest release go1.18.3。I have run the test with Administrator privileges to create symlinks,and it can create symlinks but it returns errror。I just show the existence of this problem through this test。You can call symlink after calling zone and the problem will arise

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 29, 2022

Please tell us exactly what you are doing, and exactly what happens. If you are modifying the code that comes with Go, please show us the complete file that you are running. Thanks.

@zdszxp
Copy link
Author

@zdszxp zdszxp commented Jun 29, 2022

Please tell us exactly what you are doing, and exactly what happens. If you are modifying the code that comes with Go, please show us the complete file that you are running. Thanks.

I don't modify the code that comes with Go.You can run this test:

func TestSymlink(t *testing.T) {
	now := time.Now()
	now.Zone()
	from, to := "symlinktestfrom", "symlinktestto"
	file, err := os.Create(to)
	if err != nil {
		t.Fatalf("Create(%q) failed: %v", to, err)
	}
	if err = file.Close(); err != nil {
		t.Errorf("Close(%q) failed: %v", to, err)
	}
	err = os.Symlink(to, from)
	if err != nil {
		t.Fatalf("Symlink(%q, %q) failed: %v", to, from, err)
	}
}
=== RUN   TestSymlink
    util_test.go:71: Symlink("symlinktestto", "symlinktestfrom") failed: symlink symlinktestto symlinktestfrom: Cannot create a file when that file already exists.
--- FAIL: TestSymlink (0.02s)

FAIL

@mattn
Copy link
Member

@mattn mattn commented Jun 29, 2022

Hmm, I don't reproduce this.

image

Could you please test with adding small sleep?

func TestSymlink(t *testing.T) {
        now := time.Now()
        now.Zone()
        from, to := "symlinktestfrom", "symlinktestto"
        file, err := os.Create(to)
        if err != nil {
                t.Fatalf("Create(%q) failed: %v", to, err)
        }
        if err = file.Close(); err != nil {
                t.Errorf("Close(%q) failed: %v", to, err)
        }

        time.Sleep(1 * time.Second)

        err = os.Symlink(to, from)
        if err != nil {
                t.Fatalf("Symlink(%q, %q) failed: %v", to, from, err)
        }
}

@zdszxp
Copy link
Author

@zdszxp zdszxp commented Jun 29, 2022

Hmm, I don't reproduce this.

image

Could you please test with adding small sleep?

func TestSymlink(t *testing.T) {
        now := time.Now()
        now.Zone()
        from, to := "symlinktestfrom", "symlinktestto"
        file, err := os.Create(to)
        if err != nil {
                t.Fatalf("Create(%q) failed: %v", to, err)
        }
        if err = file.Close(); err != nil {
                t.Errorf("Close(%q) failed: %v", to, err)
        }

        time.Sleep(1 * time.Second)

        err = os.Symlink(to, from)
        if err != nil {
                t.Fatalf("Symlink(%q, %q) failed: %v", to, from, err)
        }
}

I run this code:
image

@mattn
Copy link
Member

@mattn mattn commented Jun 29, 2022

Do you always check that symlinktestfrom does not exist before testing?

@zdszxp
Copy link
Author

@zdszxp zdszxp commented Jun 29, 2022

Do you always check that symlinktestfrom does not exist before testing?

yes

@zdszxp
Copy link
Author

@zdszxp zdszxp commented Jun 29, 2022

Do you always check that symlinktestfrom does not exist before testing?

I debug the code. The code src\os\file_windows.go:369 returns ERROR_ACCESS_DENIED (5), but the symbolic link has been created, then CreateSymbolicLink in src\os\file_windows.go:374 creates it again

go 1.18.3 code from src\os\file_windows.go:364

var flags uint32 = windows.SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
if isdir {
	flags |= syscall.SYMBOLIC_LINK_FLAG_DIRECTORY
}
err = syscall.CreateSymbolicLink(n, o, flags) // returns ERROR_ACCESS_DENIED (5) but the symbolic link has been created
if err != nil {
	// the unprivileged create flag is unsupported
	// below Windows 10 (1703, v10.0.14972). retry without it.
	flags &^= windows.SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
	err = syscall.CreateSymbolicLink(n, o, flags) // then it creates again
	if err != nil {
		return &LinkError{"symlink", oldname, newname, err}
	}
}

return nil

@mattn
Copy link
Member

@mattn mattn commented Jun 29, 2022

I don't understand what happen on this case, but at least os.Symlink for Window should not do second try if err is not ERROR_PRIVILEGE_NOT_HELD, I think.

diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index db5c27dd30..c0cb501450 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -368,6 +368,9 @@ func Symlink(oldname, newname string) error {
 	}
 	err = syscall.CreateSymbolicLink(n, o, flags)
 	if err != nil {
+		if err.(syscall.Errno) != syscall.ERROR_PRIVILEGE_NOT_HELD {
+			return &LinkError{"symlink", oldname, newname, err}
+		}
 		// the unprivileged create flag is unsupported
 		// below Windows 10 (1703, v10.0.14972). retry without it.
 		flags &^= windows.SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 29, 2022

Change https://go.dev/cl/415094 mentions this issue: os: do not call CreateSymbolicLink again if err is not ERROR_PRIVILEGE_NOT_HELD

@seankhliao seankhliao removed the WaitingForInfo label Jun 29, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 29, 2022

@zdszxp It sounds like you are saying that even though CreateSymbolicLink returns ERROR_ACCESS_DENIED, it creates the symlink anyhow. Why would it do that?

@zdszxp
Copy link
Author

@zdszxp zdszxp commented Jun 30, 2022

@zdszxp It sounds like you are saying that even though CreateSymbolicLink returns ERROR_ACCESS_DENIED, it creates the symlink anyhow. Why would it do that?

yes, i don't know.If I remove the code about time, os.Symlink executes correctly

@zdszxp
Copy link
Author

@zdszxp zdszxp commented Jun 30, 2022

I don't understand what happen on this case, but at least os.Symlink for Window should not do second try if err is not ERROR_PRIVILEGE_NOT_HELD, I think.

diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index db5c27dd30..c0cb501450 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -368,6 +368,9 @@ func Symlink(oldname, newname string) error {
 	}
 	err = syscall.CreateSymbolicLink(n, o, flags)
 	if err != nil {
+		if err.(syscall.Errno) != syscall.ERROR_PRIVILEGE_NOT_HELD {
+			return &LinkError{"symlink", oldname, newname, err}
+		}
 		// the unprivileged create flag is unsupported
 		// below Windows 10 (1703, v10.0.14972). retry without it.
 		flags &^= windows.SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE

This is a question, but another question is, why it will return ERROR_ACCESS_DENIED) in windows10, when I call now.Zone before calling os.Symlink, but the link is still created successfully, if I remove the code about time, os.Symlink executes correctly

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jul 2, 2022

I run this code:

@zdszxp why do you use E: drive to run your code on? What is E: drive mapped to?

I cannot reproduce your issue here.

I run this test https://go.dev/play/p/TMpFxBKkafD as simple user on my local C: drive:

c:\Users\user\dev\src\issues\go\53582>a.exe -test.v
=== RUN   TestSymlink
    a_test.go:22: Symlink("symlinktestto", "symlinktestfrom") failed: symlink symlinktestto symlinktestfrom: A required privilege is not held by the client.
--- FAIL: TestSymlink (0.00s)
FAIL

c:\Users\user\dev\src\issues\go\53582>

As simple user on X: drive that is mapped to remote samba server:

X:\rw>copy /y c:\Users\user\dev\src\issues\go\53582\a.exe . && a.exe -test.v
        1 file(s) copied.
=== RUN   TestSymlink
    a_test.go:22: Symlink("symlinktestto", "symlinktestfrom") failed: symlink symlinktestto symlinktestfrom: A required privilege is not held by the client.
--- FAIL: TestSymlink (0.03s)
FAIL

X:\rw>

And locally as an Administrator:

C:\WINDOWS\system32>cd c:\Users\user\dev\src\issues\go\53582

c:\Users\user\dev\src\issues\go\53582>a.exe -test.v
=== RUN   TestSymlink
--- PASS: TestSymlink (0.00s)
PASS

c:\Users\user\dev\src\issues\go\53582>

Perhaps you can try and put some println in Go library code and figure out what is happening on your PC. Please note that your system environment will affect your result. See #53582 (comment) for example.

I also noticed that CreateSymbolicLink API uses BOOLEAN return type, unlike all other Windows API that use BOOL. You can see it listed in https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinkw . But Go code is using BOOLEAN. And BOOLEAN appears to be correct value according to current Microsoft docs and https://microsoft.public.win32.programmer.kernel.narkive.com/Xd6JhpEr/createsymboliclink-problem-in-windows-7-rtm (I googled for it).

Not sure what to do here.

Alex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation OS-Windows
Projects
None yet
Development

No branches or pull requests

7 participants