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

Jump to bottom

os: Readlink returns garbage when symlink target is like \\?\Volume{XXXXX}\ #30463

Closed
alexbrainman opened this issue Feb 28, 2019 · 4 comments
Closed

os: Readlink returns garbage when symlink target is like \\?\Volume{XXXXX}\ #30463

alexbrainman opened this issue Feb 28, 2019 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone
Go1.13

Comments

@alexbrainman
Copy link
Member

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

go version devel +8ee9bca272 Wed Feb 27 08:22:03 2019 +0000 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
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Alex\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=c:\users\alex\dev
set GOPROXY=
set GORACE=
set GOROOT=c:\users\alex\dev\go
set GOTMPDIR=
set GOTOOLDIR=c:\users\alex\dev\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
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 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\Alex\AppData\Local\Temp\go-build403512930=/tmp/go-build -gno-record-gcc-switches

What did you do?

I run go test command against this main_test.go file

package main_test

import (
	"io/ioutil"
	"os"
	"os/exec"
	"path/filepath"
	"strings"
	"testing"
)

func TestReadlink(t *testing.T) {
	tmpdir, err := ioutil.TempDir("", "TestReadlink")
	if err != nil {
		t.Fatal(err)
	}
	defer os.RemoveAll(tmpdir)

	vol := filepath.VolumeName(tmpdir)
	output, err := exec.Command("cmd", "/c", "mountvol", vol, "/L").CombinedOutput()
	if err != nil {
		t.Fatalf("failed to run mountvol %v /L: %v %q", vol, err, output)
	}
	target := strings.Trim(string(output), " \n\r")

	dirlink := filepath.Join(tmpdir, "dirlink")
	output, err = exec.Command("cmd", "/c", "mklink", "/J", dirlink, target).CombinedOutput()
	if err != nil {
		t.Fatalf("failed to run mklink %v %v: %v %q", dirlink, target, err, output)
	}

	got, err := os.Readlink(dirlink)
	if err != nil {
		t.Fatal(err)
	}
	if want := vol + `\`; got != want {
		t.Errorf(`os.Readlink(%q): got %q, want %q`, dirlink, got, want)
	}
}

What did you expect to see?

I expected test to pass.

What did you see instead?

Test fails with this error:

--- FAIL: TestReadlink (0.17s)
    main_test.go:37: os.Readlink("C:\\Users\\Alex\\AppData\\Local\\Temp\\TestReadlink551162827\\dirlink"): got "Volume{ea961e77-0000-0000-0000-501f00000000}\\", want "C:\\"
FAIL

The problem here is that os.Readlink does not handle links where target looks like this \\?\Volume{XXXXX}\. Volume{ea961e77-0000-0000-0000-501f00000000}\\ that os.Readlink returns is not useful for anything.

We have to handle symlinks, like that, because, for example, windows-arm TMP directory is such link (see #29746 (comment) for details). So it will be quite common at least on windows-arm.

Alex
@gopherbot
Copy link

Change https://golang.org/cl/164201 mentions this issue: os: make Readlink work with symlinks with target like \??\Volume{ABCD}\

@alexbrainman
Copy link
Member Author

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

Alex

@bcmills bcmills added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 28, 2019
@bcmills bcmills added this to the Go1.13 milestone Feb 28, 2019
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 28, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 28, 2019
@lowenna
Copy link

lowenna commented Feb 28, 2019 •
edited

@alexbrainman Thanks for pinging me on this. @jiria FYI

@kolyshkin kolyshkin mentioned this issue Feb 27, 2020
Merged
kolyshkin added a commit to kolyshkin/continuity that referenced this issue Feb 27, 2020
@kolyshkin
Remove Windows' Readlink fork
5f26c82 
This reverts the part of commit 8100e75 (PR containerd#113)
that added the fork of Readlink() for Windows.

At that time the fork was done to work around the bug
in golang's implementation of os.Readlink() for Windows.

The above bug was never reported upstream, but fortunately
it was independently found, reported [1], and fixed [2].
The fix made its way into go-1.13 (there is no mention of
that in release notes, so I checked it in git history).

[1] golang/go#30463
[2] https://go-review.googlesource.com/c/go/+/164201

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/continuity that referenced this issue Feb 27, 2020
@kolyshkin
Remove Windows' Readlink fork
643e66e 
This reverts the part of commit 8100e75 (PR containerd#113)
that added the fork of Readlink() for Windows.

At that time the fork was done to work around the bug
in golang's implementation of os.Readlink() for Windows.

The above bug was never reported upstream, but fortunately
it was independently found, reported [1], and fixed [2].
The fix made its way into go-1.13 (there is no mention of
that in release notes, so I checked it in git history).

[1] golang/go#30463
[2] https://go-review.googlesource.com/c/go/+/164201

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin mentioned this issue Feb 27, 2020
Merged
@golang golang locked and limited conversation to collaborators Feb 29, 2020
@bcmills bcmills mentioned this issue Oct 24, 2023
Closed
@golang golang unlocked this conversation Oct 24, 2023
@golang golang locked and limited conversation to collaborators Oct 24, 2023
@golang golang unlocked this conversation Oct 24, 2023
@bcmills
Copy link
Contributor

bcmills commented Oct 24, 2023

containerd/continuity@8100e75 referred to this issue, but I believe its use of Readlink was (and still is) incorrect; see containerd/continuity#232.

@golang golang locked as resolved and limited conversation to collaborators Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
4 participants
@bcmills @gopherbot @alexbrainman @lowenna