From 2e07ff35436bc9adc67885f17cdd9042118d0ffd Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Wed, 20 Nov 2024 13:39:14 -0500 Subject: [PATCH] cmd/go: remove base.ShortPathConservative This CL first removes the base.ShortPathConservative function. It had two classes of uses. The first was in opening files where the paths end up in error messages. In all those cases, the non-shortened paths are used to open the files, and ShortPath is only used for the error messages. The second is in base.RelPaths. RelPaths will now call ShortPath for each of the paths passed in instead of calling RelConservative and then doing the same check as ShortPath to see if the path is shorter. To avoid the possibility of incorrect relative paths ending up in error messages (that might have command lines suggested for users to run), and to avoid the possibility of incorrect relative paths appearing in the output of base.RelPaths, base.ShortPaths always does an os.SameFile check to make sure that the relative path its providing is actually correct. Since this makes ShortPath slower than just manipulating paths (because we need to stat the files), we need to be continue to enforce that ShortPath is only called for error messages (with the exception of base.RelPaths and its callers). This is a simpler way of solving the problem that base.ShortPaths intended to solve. For #68383 Change-Id: I474f464f51a9acb2250069dea3054b55d95a4ab4 Reviewed-on: https://go-review.googlesource.com/c/go/+/630276 LUCI-TryBot-Result: Go LUCI Reviewed-by: Sam Thanawalla --- src/cmd/go/internal/base/path.go | 53 +++++++++----------------- src/cmd/go/internal/modload/init.go | 2 +- src/cmd/go/internal/modload/load.go | 2 +- src/cmd/go/internal/modload/modfile.go | 10 ++--- src/cmd/go/internal/workcmd/use.go | 15 ++++---- 5 files changed, 30 insertions(+), 52 deletions(-) diff --git a/src/cmd/go/internal/base/path.go b/src/cmd/go/internal/base/path.go index 96cf1a2f053..5bb7bc3bde6 100644 --- a/src/cmd/go/internal/base/path.go +++ b/src/cmd/go/internal/base/path.go @@ -6,13 +6,12 @@ package base import ( "errors" + "io/fs" "os" "path/filepath" "runtime" "strings" "sync" - - "cmd/go/internal/str" ) // UncachedCwd returns the current working directory. @@ -35,44 +34,32 @@ func Cwd() string { } // ShortPath returns an absolute or relative name for path, whatever is shorter. -// 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. +// ShortPath should only be used when formatting paths for error messages. func ShortPath(path string) string { - if rel, err := filepath.Rel(Cwd(), path); err == nil && len(rel) < len(path) { + if rel, err := filepath.Rel(Cwd(), path); err == nil && len(rel) < len(path) && sameFile(rel, path) { return rel } return path } -// 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 +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) } - return path + return os.SameFile(fi1, fi2) } -func relConservative(basepath, targpath string) (string, error) { - relpath, err := filepath.Rel(basepath, targpath) - if err != nil { - return "", err +// 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) } - 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 + return err } // RelPaths returns a copy of paths with absolute paths @@ -80,11 +67,7 @@ func relConservative(basepath, targpath string) (string, error) { func RelPaths(paths []string) []string { out := make([]string, 0, len(paths)) for _, p := range paths { - rel, err := relConservative(Cwd(), p) - if err == nil && len(rel) < len(p) { - p = rel - } - out = append(out, p) + out = append(out, ShortPath(p)) } return out } diff --git a/src/cmd/go/internal/modload/init.go b/src/cmd/go/internal/modload/init.go index c1bca7e7329..8fdcd0da632 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)), err) + base.ShortPath(filepath.Dir(gomod)), base.ShortPathError(err)) } } errs = append(errs, err) diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go index 746cefd256d..e8f8e7fa5cd 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.ShortPathConservative(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 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 636ad03c784..769d35e073d 100644 --- a/src/cmd/go/internal/modload/modfile.go +++ b/src/cmd/go/internal/modload/modfile.go @@ -30,10 +30,6 @@ 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 @@ -49,18 +45,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", gomod, err) + return nil, nil, fmt.Errorf("errors parsing %s:\n%w", base.ShortPath(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: gomod, GoVersion: f.Go.Version, Toolchain: toolchain} + return nil, nil, &gover.TooNewError{What: base.ShortPath(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", 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", base.ShortPath(gomod)) } return data, f, err diff --git a/src/cmd/go/internal/workcmd/use.go b/src/cmd/go/internal/workcmd/use.go index 3e503bfac5b..afbe99d3a48 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 := base.ShortPathConservative(filepath.Join(absDir, "go.mod")) + file := 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", file)) + sw.Error(fmt.Errorf("%v is not a regular file", base.ShortPath(file))) return } @@ -126,18 +126,17 @@ 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(useDirShort) + info, err := fsys.Stat(absArg) 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", useDirShort) + err = fmt.Errorf("directory %v does not exist", base.ShortPath(absArg)) } sw.Error(err) continue } else if !info.IsDir() { - sw.Error(fmt.Errorf("%s is not a directory", useDirShort)) + sw.Error(fmt.Errorf("%s is not a directory", base.ShortPath(absArg))) continue } @@ -158,7 +157,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.ShortPathConservative(path)) + fmt.Fprintf(os.Stderr, "warning: ignoring symlink %s\n", base.ShortPath(path)) } } return nil @@ -210,7 +209,7 @@ func workUse(ctx context.Context, gowork string, wf *modfile.WorkFile, args []st } else { abs = filepath.Join(workDir, use.Path) } - _, mf, err := modload.ReadModFile(base.ShortPathConservative(filepath.Join(abs, "go.mod")), nil) + _, mf, err := modload.ReadModFile(filepath.Join(abs, "go.mod"), nil) if err != nil { sw.Error(err) continue