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

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

Closed
mandelsoft opened this issue Jan 15, 2018 · 16 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mandelsoft
Copy link

mandelsoft commented Jan 15, 2018

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
Copy link
Member

mvdan commented Jan 15, 2018

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.

@mvdan mvdan changed the title package path/filepath does not work at all for paths containing symbolic links on directories path/filepath: does not work at all for paths containing symbolic links on directories Jan 15, 2018
@mvdan mvdan added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 15, 2018
@cznic
Copy link
Contributor

cznic commented Jan 15, 2018

My bet is on working as intended.

@mandelsoft
Copy link
Author

mandelsoft commented Jan 15, 2018

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
Copy link
Author

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
Copy link
Contributor

cznic commented Jan 15, 2018

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

@hirochachacha
Copy link
Contributor

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
Copy link
Contributor

cznic commented Jan 15, 2018

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
Copy link
Contributor

cznic commented Jan 15, 2018

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.

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Jan 15, 2018
@hirochachacha
Copy link
Contributor

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

@fraenkel
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@TheLinuxGuy
Copy link

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

@alexbrainman
Copy link
Member

which version go fixed this bug?

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

Alex

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

9 participants