path/filepath: does not work at all for paths containing symbolic links on directories

[mandelsoft]

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

all /up to 1.10

Does this issue reproduce with the latest release?

yes

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

all

What did you do?

Using filepath.Join or filepath.EvalSymlinks

What did you expect to see?

I expected those functions to transform valid file paths to still valid file paths

What did you see instead?

Completely corrupted file paths if the path contains a symbolic link to a directory

General Description

The first time I observed a strange behaviour was by using terraform. terrform switched to the
use of filepath.EvalSymlinks. Afterwards my terraform project using symbolic links was completely corrupted. Later I wanted to use this function in my own coding, with the same strange results.

An analysis of the filepath package yields something I couldn't believe:

The package defines a function Clean that formally normalizes a file path by eliminating .. and . entries. As described in the documentation this is done WITHOUT observing the actual file system. Although this is no problem for the function itself, because it is designed to do so, it becomes a severe problem for the whole package, because NEARLY ALL functions internally use Clean to clean the path even on intermediate results. As a consequence even functions like Join may deliver corrupted invalid results for valid inputs if the path incorporates symbolic links to directories. Especially EvalSymlinks cannot be used to evaluate paths to existing files, because Clean is internally used to normalize content of symbolic links.

Therefore I had to switch to a modified implementation solving this problem.

[mvdan]

This bug would be much clearer if you provided a small, self-contained program showing the unexpected output, and stating what output you were expecting. Please try to provide one.

[cznic]

My bet is on working as intended.

[mandelsoft]

An example for misbehaving EvalSymlinks could be:

Just use this small program (osyml ):

package main

import (
	"fmt"
	"os"

	"path/filepath"
)

func main() {
	if len(os.Args) < 2 {
		handle(".")
	} else {
		for _, p := range os.Args[1:] {
			handle(p)
		}
	}
}

func handle(p string) {
	a, err := filepath.EvalSymlinks(p)
	if err != nil {
		fmt.Printf("%s: %s\n", p, err)
	} else {
		fmt.Printf("%s -> %s\n", p, a)
	}
}

on the following filesystem structure:

├── src
│   ├── pool
│   │   └── test
│   └── versions
│       ├── current -> ../../version
│       └── v1
│           └── modules
│               └── test -> ../../../pool/test
└── version -> src/versions/v1

by calling osyml src/versions/current/modules/test

Expected result is obviously src/versions/current/modules/test -> src/pool/test,
but the execution fails with: src/versions/current/modules/test: lstat ../pool: no such file or directory

[mandelsoft]

Another example for filepath.Join:

.
├── version -> versions/v1
└── versions
    ├── generic
    │   └── test
    └── v1
        └── test

The file version/../generic/testexists, but Join("version","../generic/test") results in generic/test, which obviously does not exist.

Therefore all those functions cannot be used on paths given by somebody else (i.e. Input Parameter), because the result might be not as expected, if there are .. and symbolic directory links. There is no
library function in go that handles this correctly. If the actual behaviour is seen as works as designed, what are those functions for, if I cannot use them on arbitrary paths and file structures.

[cznic]

Deleted by me, wrong setup used. Will try again.

[hirochachacha]

I guess that the following patch fix the problem.

diff --git a/src/path/filepath/symlink.go b/src/path/filepath/symlink.go
index 824aee4e49..9e0af00c73 100644
--- a/src/path/filepath/symlink.go
+++ b/src/path/filepath/symlink.go
@@ -74,6 +74,16 @@ func walkLinks(path string, linksWalked *int) (string, error) {
                if err != nil {
                        return "", err
                }
+               for {
+                       var islink bool
+                       newdir, islink, err = walkLink(newdir, linksWalked)
+                       if err != nil {
+                               return "", err
+                       }
+                       if !islink {
+                               break
+                       }
+               }
                newpath, islink, err := walkLink(Join(newdir, file), linksWalked)
                if err != nil {
                        return "", err

[cznic]

Therefore all those functions cannot be used on paths given by somebody else (i.e. Input Parameter), because the result might be not as expected, if there are .. and symbolic directory links. There is no
library function in go that handles this correctly. If the actual behaviour is seen as works as designed, what are those functions for, if I cannot use them on arbitrary paths and file structures.

The intended use is to first resolve symlinks and only then use Join or any other function documented to work only lexically.

[cznic]

Expected result is obviously src/versions/current/modules/test -> src/pool/test,
but the execution fails with: src/versions/current/modules/test: lstat ../pool: no such file or directory

After properly reproducing the setup, I can reproduce your error result, that's all I know ATM.

[hirochachacha]

FYI, the problem is coming from wrong evaluation order.
To solve this problem correctly, someone needs to re-design walkLinks carefully.

[fraenkel]

Its not what I would call the wrong evaluation order but more Cleaning at every possible step. The only safe time to Clean is when we are done since Join and Clean does not lstat before eliding dots.
I have cobbled something that appears to work, now I just need to clean it up and add some testcases. Should have something tomorrow.

[fraenkel]

While I like the approach above, it doesn't quite work in all situations. I have added a single test case which highlights the problem. The above code only solves 1 out of 3 scenarios performed in the tests.

[gopherbot]

Change https://golang.org/cl/121305 mentions this issue: path/filepath: evaluate symlinks until it is not one

[gopherbot]

Change https://golang.org/cl/121497 mentions this issue: path/filepath: do not clean when walking symlinks

[gopherbot]

Change https://golang.org/cl/121676 mentions this issue: path/filepath: rewrite walkSymlinks

[TheLinuxGuy]

which version go fixed this bug?
I have go 1.11 and on windows the symlink issue continues to occur.

[alexbrainman]

which version go fixed this bug?

7d27e87 change has not been released yet. It will be part of go1.12.

Alex