From: Russ Cox Date: Wed, 20 Nov 2024 22:09:43 +0000 (+0000) Subject: cmd/go: revert "remove base.ShortPathConservative" X-Git-Tag: go1.24rc1~168 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=60299c251364c6f4a830721e4953230260d118a0;p=gostls13.git cmd/go: revert "remove base.ShortPathConservative" This reverts commit 2e07ff35436b (CL 630276). Reason for revert: broke longtest builders. Change-Id: I86b313cc6c1be061b408502e96a09916a6ac2c4e Reviewed-on: https://go-review.googlesource.com/c/go/+/630317 Auto-Submit: Russ Cox Reviewed-by: Ian Lance Taylor Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI --- diff --git a/src/cmd/go/internal/base/path.go b/src/cmd/go/internal/base/path.go index 5bb7bc3bde..96cf1a2f05 100644 --- a/src/cmd/go/internal/base/path.go +++ b/src/cmd/go/internal/base/path.go @@ -6,12 +6,13 @@ package base import ( "errors" - "io/fs" "os" "path/filepath" "runtime" "strings" "sync" + + "cmd/go/internal/str" ) // UncachedCwd returns the current working directory. @@ -34,32 +35,44 @@ func Cwd() string { } // ShortPath returns an absolute or relative name for path, whatever is shorter. -// ShortPath should only be used when formatting paths for error messages. +// There are rare cases where the path produced by ShortPath could be incorrect +// so it should only be used when formatting paths for error messages, not to read +// a file. func ShortPath(path string) string { - if rel, err := filepath.Rel(Cwd(), path); err == nil && len(rel) < len(path) && sameFile(rel, path) { + if rel, err := filepath.Rel(Cwd(), path); err == nil && len(rel) < len(path) { return rel } return path } -func sameFile(path1, path2 string) bool { - fi1, err1 := os.Stat(path1) - fi2, err2 := os.Stat(path2) - if err1 != nil || err2 != nil { - // If there were errors statting the files return false, - // unless both of the files don't exist. - return os.IsNotExist(err1) && os.IsNotExist(err2) +// ShortPathConservative is similar to ShortPath, but returns the input if the result of ShortPath +// would meet conditions that could make it invalid. If the short path would reach into a +// parent directory and the base path contains a symlink, a ".." component can +// cross a symlink boundary. That could be a problem because the symlinks could be evaluated, +// changing the relative location of the boundary, before the ".." terms are applied to +// go to parents. The check here is a little more conservative: it checks +// whether the path starts with a ../ or ..\ component, and if any of the parent directories +// of the working directory are symlinks. +// See #68383 for a case where this could happen. +func ShortPathConservative(path string) string { + if rel, err := relConservative(Cwd(), path); err == nil && len(rel) < len(path) { + return rel } - return os.SameFile(fi1, fi2) + return path } -// ShortPathError rewrites the path in err using base.ShortPath, if err is a wrapped PathError. -func ShortPathError(err error) error { - var pe *fs.PathError - if errors.As(err, &pe) { - pe.Path = ShortPath(pe.Path) +func relConservative(basepath, targpath string) (string, error) { + relpath, err := filepath.Rel(basepath, targpath) + if err != nil { + return "", err } - return err + if strings.HasPrefix(relpath, str.WithFilePathSeparator("..")) { + expanded, err := filepath.EvalSymlinks(basepath) + if err != nil || expanded != basepath { // The basepath contains a symlink. Be conservative and reject it. + return "", errors.New("conservatively rejecting relative path that may be invalid") + } + } + return relpath, nil } // RelPaths returns a copy of paths with absolute paths @@ -67,7 +80,11 @@ func ShortPathError(err error) error { func RelPaths(paths []string) []string { out := make([]string, 0, len(paths)) for _, p := range paths { - out = append(out, ShortPath(p)) + rel, err := relConservative(Cwd(), p) + if err == nil && len(rel) < len(p) { + p = rel + } + out = append(out, p) } return out } diff --git a/src/cmd/go/internal/modload/init.go b/src/cmd/go/internal/modload/init.go index 8fdcd0da63..c1bca7e732 100644 --- a/src/cmd/go/internal/modload/init.go +++ b/src/cmd/go/internal/modload/init.go @@ -944,7 +944,7 @@ func loadModFile(ctx context.Context, opts *PackageOpts) (*Requirements, error) err = errWorkTooOld(gomod, workFile, tooNew.GoVersion) } else { err = fmt.Errorf("cannot load module %s listed in go.work file: %w", - base.ShortPath(filepath.Dir(gomod)), base.ShortPathError(err)) + base.ShortPath(filepath.Dir(gomod)), err) } } errs = append(errs, err) diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go index e8f8e7fa5c..746cefd256 100644 --- a/src/cmd/go/internal/modload/load.go +++ b/src/cmd/go/internal/modload/load.go @@ -670,7 +670,7 @@ func resolveLocalPackage(ctx context.Context, dir string, rs *Requirements) (str } if inWorkspaceMode() { if mr := findModuleRoot(absDir); mr != "" { - return "", fmt.Errorf("%s is contained in a module that is not one of the workspace modules listed in go.work. You can add the module to the workspace using:\n\tgo work use %s", dirstr, base.ShortPath(mr)) + return "", fmt.Errorf("%s is contained in a module that is not one of the workspace modules listed in go.work. You can add the module to the workspace using:\n\tgo work use %s", dirstr, base.ShortPathConservative(mr)) } return "", fmt.Errorf("%s outside modules listed in go.work or their selected dependencies", dirstr) } diff --git a/src/cmd/go/internal/modload/modfile.go b/src/cmd/go/internal/modload/modfile.go index 769d35e073..636ad03c78 100644 --- a/src/cmd/go/internal/modload/modfile.go +++ b/src/cmd/go/internal/modload/modfile.go @@ -30,6 +30,10 @@ import ( // ReadModFile reads and parses the mod file at gomod. ReadModFile properly applies the // overlay, locks the file while reading, and applies fix, if applicable. func ReadModFile(gomod string, fix modfile.VersionFixer) (data []byte, f *modfile.File, err error) { + // The path used to open the file shows up in errors. Use ShortPathConservative + // so a more convenient path is displayed in the errors. ShortPath isn't used + // because it's meant only to be used in errors, not to open files. + gomod = base.ShortPathConservative(gomod) if fsys.Replaced(gomod) { // Don't lock go.mod if it's part of the overlay. // On Plan 9, locking requires chmod, and we don't want to modify any file @@ -45,18 +49,18 @@ func ReadModFile(gomod string, fix modfile.VersionFixer) (data []byte, f *modfil f, err = modfile.Parse(gomod, data, fix) if err != nil { // Errors returned by modfile.Parse begin with file:line. - return nil, nil, fmt.Errorf("errors parsing %s:\n%w", base.ShortPath(gomod), err) + return nil, nil, fmt.Errorf("errors parsing %s:\n%w", gomod, err) } if f.Go != nil && gover.Compare(f.Go.Version, gover.Local()) > 0 { toolchain := "" if f.Toolchain != nil { toolchain = f.Toolchain.Name } - return nil, nil, &gover.TooNewError{What: base.ShortPath(gomod), GoVersion: f.Go.Version, Toolchain: toolchain} + return nil, nil, &gover.TooNewError{What: gomod, GoVersion: f.Go.Version, Toolchain: toolchain} } if f.Module == nil { // No module declaration. Must add module path. - return nil, nil, fmt.Errorf("error reading %s: missing module declaration. To specify the module path:\n\tgo mod edit -module=example.com/mod", base.ShortPath(gomod)) + return nil, nil, fmt.Errorf("error reading %s: missing module declaration. To specify the module path:\n\tgo mod edit -module=example.com/mod", gomod) } return data, f, err diff --git a/src/cmd/go/internal/workcmd/use.go b/src/cmd/go/internal/workcmd/use.go index afbe99d3a4..3e503bfac5 100644 --- a/src/cmd/go/internal/workcmd/use.go +++ b/src/cmd/go/internal/workcmd/use.go @@ -102,7 +102,7 @@ func workUse(ctx context.Context, gowork string, wf *modfile.WorkFile, args []st lookDir := func(dir string) { absDir, dir := pathRel(workDir, dir) - file := filepath.Join(absDir, "go.mod") + file := base.ShortPathConservative(filepath.Join(absDir, "go.mod")) fi, err := fsys.Stat(file) if err != nil { if os.IsNotExist(err) { @@ -114,7 +114,7 @@ func workUse(ctx context.Context, gowork string, wf *modfile.WorkFile, args []st } if !fi.Mode().IsRegular() { - sw.Error(fmt.Errorf("%v is not a regular file", base.ShortPath(file))) + sw.Error(fmt.Errorf("%v is not a regular file", file)) return } @@ -126,17 +126,18 @@ func workUse(ctx context.Context, gowork string, wf *modfile.WorkFile, args []st for _, useDir := range args { absArg, _ := pathRel(workDir, useDir) + useDirShort := base.ShortPathConservative(absArg) // relative to the working directory rather than the workspace - info, err := fsys.Stat(absArg) + info, err := fsys.Stat(useDirShort) if err != nil { // Errors raised from os.Stat are formatted to be more user-friendly. if os.IsNotExist(err) { - err = fmt.Errorf("directory %v does not exist", base.ShortPath(absArg)) + err = fmt.Errorf("directory %v does not exist", useDirShort) } sw.Error(err) continue } else if !info.IsDir() { - sw.Error(fmt.Errorf("%s is not a directory", base.ShortPath(absArg))) + sw.Error(fmt.Errorf("%s is not a directory", useDirShort)) continue } @@ -157,7 +158,7 @@ func workUse(ctx context.Context, gowork string, wf *modfile.WorkFile, args []st if !d.IsDir() { if d.Type()&fs.ModeSymlink != 0 { if target, err := fsys.Stat(path); err == nil && target.IsDir() { - fmt.Fprintf(os.Stderr, "warning: ignoring symlink %s\n", base.ShortPath(path)) + fmt.Fprintf(os.Stderr, "warning: ignoring symlink %s\n", base.ShortPathConservative(path)) } } return nil @@ -209,7 +210,7 @@ func workUse(ctx context.Context, gowork string, wf *modfile.WorkFile, args []st } else { abs = filepath.Join(workDir, use.Path) } - _, mf, err := modload.ReadModFile(filepath.Join(abs, "go.mod"), nil) + _, mf, err := modload.ReadModFile(base.ShortPathConservative(filepath.Join(abs, "go.mod")), nil) if err != nil { sw.Error(err) continue