path/filepath: EvalSymlinks, incorrect traversal of relative paths

[jbardin]

go version go1.12 darwin/amd64
and
go version devel +820ad17303 Fri Mar 1 18:26:37 2019 +0000 darwin/amd64

Relative paths with ../ and symlinks are not correctly evaluated.

This reproduces the issue on macOS, since the $TMPDIR in/var is a symlink by default, starting from the path $HOME/foo

func main() {
	path := "../../../var/folders/"
	fmt.Println(path)
	fmt.Println(os.Getwd())
	fmt.Println(filepath.EvalSymlinks(path))
	fi, _ := os.Stat(path)
	fmt.Println(fi.Name())
}

Which outputs

$ go run eval_symlinks.go
../../../var/folders/
/Users/jbardin/foo <nil>
 lstat ../var: no such file or directory
folders

The resolution seems to depend on the number of ../ in the path, as it does work from $HOME with ../../var/folder. The errors strings also alternate the number of reported ../ depending on whether there were an odd or even number in the original path.

The behavior is the same on linux on linux with a similar directory structure.

[bcmills]

This reproduces the issue on macOS

Can you provide a reproducer that does not depend on a specific OS and working directory? I've read this issue three times and I still don't understand what exactly is wrong with the current logic.

[bcmills]

CC @robpike @rsc for path/filepath

[ianlancetaylor]

If I'm reading this correctly, the case in question is when someone uses .. to step up above the root directory.

[jbardin]

@ianlancetaylor, That's what I thought too (though it should still work in that case), but here it's using the correct number of ../.

Here's a self-contained repro for linux:

Dockerfile

FROM golang:1.12.0-stretch

RUN mkdir /tmp/target; ln -s /tmp /symlink
WORKDIR /home/user/foo
COPY eval_symlinks.go ./

CMD go run eval_symlinks.go

eval_symlinks.go

package main

import (
	"fmt"
	"os"
	"path/filepath"
)

func main() {
	path := "../../../symlink/target/"
	fmt.Println(path)
	fmt.Println(os.Getwd())
	fmt.Println(filepath.EvalSymlinks(path))
	fi, _ := os.Stat(path)
	fmt.Println(fi.Name())
}

[ianlancetaylor]

I think this is a self-contained example that doesn't require Docker or assumptions about home directory layout.

package main

import (
	"fmt"
	"io/ioutil"
	"log"
	"os"
	"path/filepath"
	"strings"
)

func main() {
	tmpdir, err := ioutil.TempDir("", "EvalSymlinks")
	if err != nil {
		log.Fatal(err)
	}

	if err := os.Chdir(tmpdir); err != nil {
		log.Fatal(err)
	}
	defer os.RemoveAll(".")

	if err := os.Mkdir("a", 0777); err != nil {
		log.Fatal(err)
	}
	if err := os.Symlink("a", "b"); err != nil {
		log.Fatal(err)
	}
	if err := ioutil.WriteFile(filepath.Join("a", "file"), nil, 0666); err != nil {
		log.Fatal(err)
	}

	wd, err := os.Getwd()
	if err != nil {
		log.Fatal(err)
	}
	up := strings.Repeat(".." + string(os.PathSeparator), strings.Count(wd, string(os.PathSeparator)))
	up = filepath.Join("..", up, wd, "b", "file")
	if eval, err := filepath.EvalSymlinks(up); err != nil {
		fmt.Fprintf(os.Stderr, "EvalSymlinks(%q) failed: %v\n", up, err)
	} else {
		fmt.Println(eval)
	}

	up = filepath.Join("..", up)
	if eval, err := filepath.EvalSymlinks(up); err != nil {
		fmt.Fprintf(os.Stderr, "EvalSymlinks(%q) failed: %v\n", up, err)
	} else {
		fmt.Println(eval)
	}
}

[ianlancetaylor]

When I run that program I see

EvalSymlinks("../../../tmp/EvalSymlinks289380246/b/file") failed: lstat ../tmp: no such file or directory
../../tmp/EvalSymlinks289380246/a/file

In other words, evaluating the path fails by itself, and succeeds when adding a ../ prefix.

[gopherbot]

Change https://golang.org/cl/164762 mentions this issue: path/filepath: handle extra .. in EvalSymlinks

[jbardin]

@ianlancetaylor I don't think it's an extra ../ (I admit I haven't had time to walk through the code, so I don't know exactly what's going on). The examples I've managed to create all require a certain number of path segments to trigger.

I modified your example to follow the patten above, which still fails with 164762

func main() {
	tmpdir, err := ioutil.TempDir("", "EvalSymlinks")
	if err != nil {
		log.Fatal(err)
	}

	if err := os.Chdir(tmpdir); err != nil {
		log.Fatal(err)
	}
	defer os.RemoveAll(".")

	// make a/b/file and c->b
	ab := filepath.Join("a", "b")
	if err := os.MkdirAll(ab, 0777); err != nil {
		log.Fatal(err)
	}
	if err := os.Symlink(ab, "c"); err != nil {
		log.Fatal(err)
	}
	if err := ioutil.WriteFile(filepath.Join(ab, "file"), nil, 0666); err != nil {
		log.Fatal(err)
	}

	wd, err := os.Getwd()
	if err != nil {
		log.Fatal(err)
	}

	// move to ./home/user/foo
	foo := filepath.Join("home", "user", "foo")
	if err := os.MkdirAll(foo, 0777); err != nil {
		log.Fatal(err)
	}
	if err := os.Chdir(foo); err != nil {
		log.Fatal(err)
	}
	defer os.Chdir(wd)

	// look for ../../../c/file
	filePath := filepath.Join("..", "..", "..", "c", "file")
	if eval, err := filepath.EvalSymlinks(filePath); err != nil {
		log.Printf("EvalSymlinks(%q) failed: %v\n", filePath, err)
	} else {
		fmt.Println(eval)
	}

	fi, err := os.Stat(filePath)
	if err != nil {
		log.Fatal(err)
	}
	log.Printf("os.Stat for %q success\n", fi.Name())
}

outputs:

2019/03/01 16:04:23 EvalSymlinks("../../../c/file") failed: lstat ../../c: no such file or directory
2019/03/01 16:04:23 os.Stat for "file" success

[ianlancetaylor]

Do you think you could add to the test in https://golang.org/cl/164762 to show the problem?

[jbardin]

Sorry, here's the above example formatted as a test. If someone wants to remind me how to push a single commit to an existing changeset in gerrit, I can give it another try, but it's been a long time since I've used gerrit.

func TestEvalSymlinksParentDirWithSymlinks(t *testing.T) {
	tmpdir, err := ioutil.TempDir("", "EvalSymlinks")
	if err != nil {
		t.Fatal(err)
	}

	if err := os.Chdir(tmpdir); err != nil {
		t.Fatal(err)
	}
	defer os.RemoveAll(".")

	// make a/b/file and c->b
	ab := filepath.Join("a", "b")
	if err := os.MkdirAll(ab, 0777); err != nil {
		t.Fatal(err)
	}
	if err := os.Symlink(ab, "c"); err != nil {
		t.Fatal(err)
	}
	if err := ioutil.WriteFile(filepath.Join(ab, "file"), nil, 0666); err != nil {
		t.Fatal(err)
	}

	wd, err := os.Getwd()
	if err != nil {
		t.Fatal(err)
	}

	// move to ./home/user/foo
	foo := filepath.Join("home", "user", "foo")
	if err := os.MkdirAll(foo, 0777); err != nil {
		t.Fatal(err)
	}
	if err := os.Chdir(foo); err != nil {
		t.Fatal(err)
	}
	defer os.Chdir(wd)

	// look for ../../../c/file, first making sure it exists with os.Stat
	filePath := filepath.Join("..", "..", "..", "c", "file")
	if _, err = os.Stat(filePath); err != nil {
		t.Fatalf("os.Stat(%q) failed: %s", filePath, err)
	}

	if _, err := filepath.EvalSymlinks(filePath); err != nil {
		t.Fatalf("EvalSymlinks(%q) failed: %v\n", filePath, err)
	}
}

[ianlancetaylor]

Thanks. I added a similar test case, and a fix for the problem.

[gopherbot]

Change https://golang.org/cl/165197 mentions this issue: [release-branch.go1.12] path/filepath: don't discard .. in EvalSymlinks

[ianlancetaylor]

@gopherbot please open backport to 1.12.

[gopherbot]

Backport issue(s) opened: #30586 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.