os: Stat does not follow relative symlinks on windows

[tamird]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8 windows/amd64

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

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

What did you do?

I ran a modified version of os.TestSymLink which creates relative symlinks outside of the cwd:

package main

import (
	"fmt"
	"io/ioutil"
	"os"
	"path/filepath"
	"testing"
)

func TestLink(t *testing.T) {
	for _, isTmpDir := range []bool{true, false} {
		t.Run(fmt.Sprintf("tempDir=%t", isTmpDir), func(t *testing.T) {
			from, to := "smlinktestfrom", "symlinktestto"
			if isTmpDir {
				tmpDir, err := ioutil.TempDir("", "logtest")
				if err != nil {
					t.Fatalf("could not create temporary directory: %s", err)
				}
				defer func() {
					if err := os.RemoveAll(tmpDir); err != nil {
						t.Fatalf("failed to clean up temp directory: %s", err)
					}
				}()

				from, to = filepath.Join(tmpDir, from), filepath.Join(tmpDir, to)
			}

			os.Remove(from) // Just in case.
			file, err := os.Create(to)
			if err != nil {
				t.Fatalf("open %q failed: %v", to, err)
			}
			defer os.Remove(to)
			if err = file.Close(); err != nil {
				t.Errorf("close %q failed: %v", to, err)
			}
			err = os.Symlink(filepath.Base(to), from)
			if err != nil {
				t.Fatalf("symlink %q, %q failed: %v", to, from, err)
			}
			defer os.Remove(from)
			tostat, err := os.Lstat(to)
			if err != nil {
				t.Fatalf("stat %q failed: %v", to, err)
			}
			if tostat.Mode()&os.ModeSymlink != 0 {
				t.Fatalf("stat %q claims to have found a symlink", to)
			}
			fromstat, err := os.Stat(from)
			if err != nil {
				t.Fatalf("stat %q failed: %v", from, err)
			}
			if !os.SameFile(tostat, fromstat) {
				t.Errorf("symlink %q, %q did not create symlink", to, from)
			}
			fromstat, err = os.Lstat(from)
			if err != nil {
				t.Fatalf("lstat %q failed: %v", from, err)
			}
			if fromstat.Mode()&os.ModeSymlink == 0 {
				t.Fatalf("symlink %q, %q did not create symlink", to, from)
			}
			fromstat, err = os.Stat(from)
			if err != nil {
				t.Fatalf("stat %q failed: %v", from, err)
			}
			if fromstat.Mode()&os.ModeSymlink != 0 {
				t.Fatalf("stat %q did not follow symlink", from)
			}
			s, err := os.Readlink(from)
			if err != nil {
				t.Fatalf("readlink %q failed: %v", from, err)
			}
			if s != filepath.Base(to) {
				t.Fatalf("after symlink %q != %q", s, filepath.Base(to))
			}
			file, err = os.Open(from)
			if err != nil {
				t.Fatalf("open %q failed: %v", from, err)
			}
			file.Close()
		})
	}
}

What did you expect to see?

The same result I see on darwin:

ok  	github.com/cockroachdb/cockroach/foo	0.007s

What did you see instead?

=== RUN   TestLink
=== RUN   TestLink/tempDir=true
=== RUN   TestLink/tempDir=false
--- FAIL: TestLink (0.00s)
    --- FAIL: TestLink/tempDir=true (0.00s)
        stat_test.go:52: stat "C:\\Users\\COCKRO~1\\AppData\\Local\\Temp\\2\\logtest096314683\\smlinktestfrom" failed: GetFileAttributesEx symlinktestto: The system cannot find the file specified.
    --- PASS: TestLink/tempDir=false (0.00s)
FAIL
exit status 1
FAIL    _/C_/Users/cockroach/AppData/Local/Temp/2/logtest207355951      0.036s

[hirochachacha]

Oh my! I suspect almost all os.Readlink usage in the stdlib do the same mistake.
We should use something like following one instead.

func readlink(link string) (string, error) {
	to, err := os.Readlink(link)
	if err != nil {
		return "", err
	}
	if filepath.IsAbs(to) {
		return to, nil
	}
	return filepath.Join(filepath.Dir(link), to), nil
}

IIUC, relative symlinks are not relative to $CWD, but relative to link path.

EDITED:
My suspicion was wrong. filepath.EvalSymlinks do the right thing.

[tamird]

IIUC, relative symlinks are not relative to $CWD, but relative to link path.

Correct.

[alexbrainman]

IIUC, relative symlinks are not relative to $CWD, but relative to link path.
Correct.

Double correct. :-)

@hirochachacha would you like to send a change? With new test ...
Thank you.

Alex

PS: simplified version of the test above https://play.golang.org/p/vrY_AAb47I

[hirochachacha]

Double correct. :-)

Yeah! it must be correct.

@hirochachacha would you like to send a change? With new test ...
Thank you.

Sure! I'll do few hours later.

[gopherbot]

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

[hirochachacha]

@alexbrainman I think path/filepath.EvalSymlinks covers relative symlinks well.

https://github.com/golang/go/blob/master/src/path/filepath/path_test.go#L752-L834

IMO, there are no need to add extra tests. Sorry about misdirection.

[alexbrainman]

I think path/filepath.EvalSymlinks covers relative symlinks well.
...
IMO, there are no need to add extra tests

Sounds good. Thank you for looking.

Alex