[go] os/exec: return error when PATH lookup would use current directory
826 views
Skip to first unread message
Olivier Mengué (Gerrit)
Feb 2, 2022, 8:28:00 AM2/2/22
to Russ Cox, goph...@pubsubhelper.golang.org, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Russ Cox.
Patch set 1:Code-Review -1
2 comments:
Patchset:
This is not a correct implementation of my proposal 43947.
File src/os/exec/lp_windows.go:
Patch Set #1, Line 98:
if _, found := syscall.Getenv("NoDefaultCurrentDirectoryInExePath"); !found {This is not a correct implementation of my proposal 43947 which is about calling the NeedCurrentDirectoryForExePathW function.
To view, visit change 381374. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Apr 19, 2022, 2:54:19 PM4/19/22
to Russ Cox, goph...@pubsubhelper.golang.org, Bryan Mills, Gopher Robot, Olivier Mengué, golang-co...@googlegroups.com
Attention is currently required from: Olivier Mengué, Russ Cox.
8 comments:
File src/cmd/dist/test.go:
Patch Set #7, Line 30:
t.goCmd = "go"I recall from CL 398061 that `cmd/dist` expects `GOROOT` to always be set in the process environment.
Given that, would it make sense to hard-code the absolute path to the 'go' command to be used for testing as `filepath.Join(goroot, "bin", "go")` rather than just `"go"`?
(That seems to be the intent behind setting `PATH` at line 114, too.)
Patch Set #7, Line 35:
if info, err := os.Stat("go" + exeSuffix); err == nil && !info.IsDir() && info.Mode()&0111 != 0 {This check seems fairly involved — is this materially different from checking whether `exec.LookPath("./go")` resolves?
File src/cmd/go/testdata/script/cgo_path.txt:
Patch Set #7, Line 17:
executable found relative to current directoryThe text of this error is a bit confusing to me — outside the specific context of the proposal, it doesn't come across as “an error”. Is there a way we could rephrase it for clarity?
File src/os/exec/dot_test.go:
Patch Set #7, Line 37:
defer os.Chdir(cwd)(nit) In theory this `Chdir` could fail (for example, if the user's filesystem access credential expires while the test is running).
Maybe check the error and panic if it fails?
Patch Set #7, Line 49:
if _, err := LookPath("execabs-test"); err == nil {Is there a test that functions as the “control case” for this, verifying that `LookPath("./execabs-test")` and `LookPath("../testdir/execabs-test")` do still succeed?
(Or should those not succeed either?)
Patch Set #7, Line 64:
// and it should fail because it's not a valid binary.The initial write is with permission 0777 — does this rely on the temp directory's umask to prevent a co-tenant from overwriting the dummy executable with a hostile one?
File src/os/exec/lp_unix.go:
Patch Set #7, Line 34:
// On success, the result is an absolute path.Does this hold even if the file is tried directly (per the previous sentence)?
File src/os/exec/lp_windows.go:
Patch Set #1, Line 98:
if _, found := syscall.Getenv("NoDefaultCurrentDirectoryInExePath"); !found {
As I understand it, the system setting is reflected in the environment variable, and the environment […]
The remarks in the documentation say:
> However, you should call this function rather than checking the environment variable directly, as the registry location of this environment variable can change.That seems to imply that the variable is expected to come from the registry rather than per-process environment variables.
Does `syscall.Getenv` resolve environment variables from the registry? (It isn't obvious to me either way, even after reading https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getenvironmentvariable.)
To view, visit change 381374. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Apr 27, 2022, 4:17:35 PM4/27/22
to Russ Cox, goph...@pubsubhelper.golang.org, Gopher Robot, Bryan Mills, Olivier Mengué, golang-co...@googlegroups.com
Attention is currently required from: Olivier Mengué, Russ Cox.
1 comment:
File src/os/exec/lp_windows.go:
Patch Set #1, Line 98:
if _, found := syscall.Getenv("NoDefaultCurrentDirectoryInExePath"); !found {
I don't understand what it means to check the registry when
(1) if you want to force it off, you create the environment variable
(2) if you want to force it on, you delete the environment variable.I can't see when an implementation would ever check the registry.
I'm happy to return to this if someone can provide a real example of
how this test is incorrect.I'm not sure that I know what it means to “delete the environment variable” on Windows.
The third paragraph of the Remarks section¹ for NeedCurrentDirectoryForExePath worries me, in a vague Chesterton's Fence sort of way. It explicitly says: “you should call this function rather than checking the environment variable directly”. If it really is just a matter of checking the environment variable, what is that paragraph getting at?
To view, visit change 381374. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Apr 29, 2022, 2:55:34 PM4/29/22
to Russ Cox, goph...@pubsubhelper.golang.org, Gopher Robot, Bryan Mills, Olivier Mengué, golang-co...@googlegroups.com
Attention is currently required from: Bryan Mills, Olivier Mengué, Russ Cox.
Bryan Mills removed a vote from this change.
Removed Run-TryBot+1 by Russ Cox <r...@golang.org>
To view, visit change 381374. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Apr 29, 2022, 2:55:37 PM4/29/22
to Russ Cox, goph...@pubsubhelper.golang.org, Gopher Robot, Bryan Mills, Olivier Mengué, golang-co...@googlegroups.com
Attention is currently required from: Bryan Mills, Olivier Mengué, Russ Cox.
Bryan Mills removed a vote from this change.
Removed TryBot-Result+1 by Gopher Robot <go...@golang.org>
Bryan Mills (Gerrit)
Apr 29, 2022, 2:55:47 PM4/29/22
to Russ Cox, goph...@pubsubhelper.golang.org, Bryan Mills, Gopher Robot, Olivier Mengué, golang-co...@googlegroups.com
Attention is currently required from: Olivier Mengué, Russ Cox.
Patch set 10:Run-TryBot +1Code-Review +2
2 comments:
Patchset:
TRY=longtest,windows-amd64-longtest
File src/os/exec/lp_windows.go:
Patch Set #1, Line 98:
if _, found := syscall.Getenv("NoDefaultCurrentDirectoryInExePath"); !found {
I'd like to move forward with this CL as is and then circle back to this if we come to understand better why the more expensive call is necessary. Is that OK with you?
Yep. Thanks for investigating!
To view, visit change 381374. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Apr 29, 2022, 5:38:45 PM4/29/22
to Russ Cox, goph...@pubsubhelper.golang.org, Bryan Mills, Gopher Robot, Olivier Mengué, golang-co...@googlegroups.com
Bryan Mills has created a revert of this change.
To view, visit change 381374. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Apr 29, 2022, 10:30:18 PM4/29/22
to Russ Cox, goph...@pubsubhelper.golang.org, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Russ Cox.
Patch set 2:Code-Review +2
4 comments:
File src/cmd/dist/build.go:
Patch Set #2, Line 122:
gorootBinGo = pathf("%s/bin/go", goroot)Does this need a ".exe" on Windows?
File src/cmd/dist/test.go:
Extraneous whitespace change?
File src/os/exec/dot_test.go:
Patch Set #2, Line 1:
// Copyright 2020 The Go Authors. All rights reserved.s/2020/2022/
Patch Set #2, Line 31:
if err := ioutil.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0777); err != nil {os.WriteFile
To view, visit change 403274. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
May 2, 2022, 10:54:14 AM5/2/22
to Russ Cox, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, golang-co...@googlegroups.com
Gopher Robot submitted this change.
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/os/exec/dot_test.go
Insertions: 2, Deletions: 3.
@@ -1,4 +1,4 @@
-// Copyright 2020 The Go Authors. All rights reserved.
+// Copyright 2022 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
@@ -7,7 +7,6 @@
import (
"errors"
"internal/testenv"
- "io/ioutil"
"os"
. "os/exec"
"path/filepath"
@@ -28,7 +27,7 @@
if runtime.GOOS == "windows" {
executable += ".exe"
}
- if err := ioutil.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0777); err != nil {
+ if err := os.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0777); err != nil {
t.Fatal(err)
}
cwd, err := os.Getwd()
```
Approvals:
Gopher Robot: TryBots succeeded
Ian Lance Taylor: Looks good to me, approved
Russ Cox: Run TryBots; Automatically submit change
os/exec: return error when PATH lookup would use current directory
CL 381374 was reverted because x/sys/execabs broke.
This CL reapplies CL 381374, but adding a lookPathErr error
field back, for execabs to manipulate with reflect.
That field will just be a bit of scar tissue in this package forever,
to keep old code working with new toolchains.
CL 403256 fixes x/sys/execabs's test to be ready for the change.
Older versions of x/sys/execabs will keep working
(that is, will keep rejecting what they should reject),
but they will return a slightly different error from LookPath
without that CL, and the test fails because of the different
error text.
For #43724.
This reverts commit f2b674756b3b684118e4245627d4ed8c07e518e7.
Change-Id: Iee55f8cd9939e1bd31e5cbdada50681cdc505117
Reviewed-on: https://go-review.googlesource.com/c/go/+/403274
Auto-Submit: Russ Cox <r...@golang.org>
Run-TryBot: Russ Cox <r...@golang.org>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Ian Lance Taylor <ia...@google.com>
---
A api/next/43724.txt
M src/cmd/dist/build.go
M src/cmd/dist/test.go
M src/cmd/dist/util.go
M src/cmd/go/testdata/script/cgo_path.txt
M src/internal/execabs/execabs.go
D src/internal/execabs/execabs_test.go
A src/os/exec/dot_test.go
M src/os/exec/exec.go
M src/os/exec/lp_plan9.go
M src/os/exec/lp_unix.go
M src/os/exec/lp_windows.go
M src/os/exec/lp_windows_test.go
13 files changed, 302 insertions(+), 173 deletions(-)
diff --git a/api/next/43724.txt b/api/next/43724.txt
new file mode 100644
index 0000000..1030a80
--- /dev/null
+++ b/api/next/43724.txt
@@ -0,0 +1,2 @@
+pkg os/exec, type Cmd struct, Err error #43724
+pkg os/exec, var ErrDot error #43724
diff --git a/src/cmd/dist/build.go b/src/cmd/dist/build.go
index c19714c..f99f1f4 100644
--- a/src/cmd/dist/build.go
+++ b/src/cmd/dist/build.go
@@ -27,6 +27,7 @@
var (
goarch string
gorootBin string
+ gorootBinGo string
gohostarch string
gohostos string
goos string
@@ -115,6 +116,12 @@
goroot = filepath.Clean(b)
gorootBin = pathf("%s/bin", goroot)
+ // Don't run just 'go' because the build infrastructure
+ // runs cmd/dist inside go/bin often, and on Windows
+ // it will be found in the current directory and refuse to exec.
+ // All exec calls rewrite "go" into gorootBinGo.
+ gorootBinGo = pathf("%s/bin/go", goroot)
+
b = os.Getenv("GOROOT_FINAL")
if b == "" {
b = goroot
diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go
index afe4b3a..be4c552 100644
--- a/src/cmd/dist/test.go
+++ b/src/cmd/dist/test.go
@@ -27,6 +27,7 @@
gogcflags = os.Getenv("GO_GCFLAGS")
var t tester
+
var noRebuild bool
flag.BoolVar(&t.listMode, "list", false, "list available tests")
flag.BoolVar(&t.rebuild, "rebuild", false, "rebuild everything first")
@@ -96,15 +97,9 @@
func (t *tester) run() {
timelog("start", "dist test")
- var exeSuffix string
- if goos == "windows" {
- exeSuffix = ".exe"
- }
- if _, err := os.Stat(filepath.Join(gorootBin, "go"+exeSuffix)); err == nil {
- os.Setenv("PATH", fmt.Sprintf("%s%c%s", gorootBin, os.PathListSeparator, os.Getenv("PATH")))
- }
+ os.Setenv("PATH", fmt.Sprintf("%s%c%s", gorootBin, os.PathListSeparator, os.Getenv("PATH")))
- cmd := exec.Command("go", "env", "CGO_ENABLED")
+ cmd := exec.Command(gorootBinGo, "env", "CGO_ENABLED")
cmd.Stderr = new(bytes.Buffer)
slurp, err := cmd.Output()
if err != nil {
@@ -419,7 +414,7 @@
args = append(args, "-run=^$")
}
args = append(args, stdMatches...)
- cmd := exec.Command("go", args...)
+ cmd := exec.Command(gorootBinGo, args...)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
return cmd.Run()
@@ -456,7 +451,7 @@
args = append(args, "-bench=.*")
}
args = append(args, benchMatches...)
- cmd := exec.Command("go", args...)
+ cmd := exec.Command(gorootBinGo, args...)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
return cmd.Run()
@@ -484,7 +479,7 @@
} else {
// Use a format string to only list packages and commands that have tests.
const format = "{{if (or .TestGoFiles .XTestGoFiles)}}{{.ImportPath}}{{end}}"
- cmd := exec.Command("go", "list", "-f", format)
+ cmd := exec.Command(gorootBinGo, "list", "-f", format)
if t.race {
cmd.Args = append(cmd.Args, "-tags=race")
}
@@ -619,7 +614,7 @@
fmt.Println("skipping terminal test; stdout/stderr not terminals")
return nil
}
- cmd := exec.Command("go", "test")
+ cmd := exec.Command(gorootBinGo, "test")
setDir(cmd, filepath.Join(os.Getenv("GOROOT"), "src/cmd/go/testdata/testterminal18153"))
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
@@ -1003,7 +998,11 @@
}
list = out
- return list[0], list[1:]
+ bin = list[0]
+ if bin == "go" {
+ bin = gorootBinGo
+ }
+ return bin, list[1:]
}
func (t *tester) addCmd(dt *distTest, dir string, cmdline ...interface{}) *exec.Cmd {
@@ -1157,7 +1156,7 @@
}
func (t *tester) runHostTest(dir, pkg string) error {
- out, err := exec.Command("go", "env", "GOEXE", "GOTMPDIR").Output()
+ out, err := exec.Command(gorootBinGo, "env", "GOEXE", "GOTMPDIR").Output()
if err != nil {
return err
}
diff --git a/src/cmd/dist/util.go b/src/cmd/dist/util.go
index 8856f46..ee8ba91 100644
--- a/src/cmd/dist/util.go
+++ b/src/cmd/dist/util.go
@@ -71,7 +71,11 @@
errprintf("run: %s\n", strings.Join(cmd, " "))
}
- xcmd := exec.Command(cmd[0], cmd[1:]...)
+ bin := cmd[0]
+ if bin == "go" {
+ bin = gorootBinGo
+ }
+ xcmd := exec.Command(bin, cmd[1:]...)
setDir(xcmd, dir)
var data []byte
var err error
diff --git a/src/cmd/go/testdata/script/cgo_path.txt b/src/cmd/go/testdata/script/cgo_path.txt
index be9609e..1f84dbc 100644
--- a/src/cmd/go/testdata/script/cgo_path.txt
+++ b/src/cmd/go/testdata/script/cgo_path.txt
@@ -14,7 +14,7 @@
[windows] exists -exec p/gcc.bat p/clang.bat
! exists p/bug.txt
! go build -x
-stderr '^cgo: exec (clang|gcc): (clang|gcc) resolves to executable relative to current directory \(.[/\\](clang|gcc)(.bat)?\)$'
+stderr '^cgo: C compiler "(clang|gcc)" not found: exec: "(clang|gcc)": cannot run executable found relative to current directory'
! exists p/bug.txt
-- go.mod --
diff --git a/src/internal/execabs/execabs.go b/src/internal/execabs/execabs.go
index 9a05d97..5f60fbb 100644
--- a/src/internal/execabs/execabs.go
+++ b/src/internal/execabs/execabs.go
@@ -12,11 +12,7 @@
import (
"context"
- "fmt"
"os/exec"
- "path/filepath"
- "reflect"
- "unsafe"
)
var ErrNotFound = exec.ErrNotFound
@@ -27,44 +23,14 @@
ExitError = exec.ExitError
)
-func relError(file, path string) error {
- return fmt.Errorf("%s resolves to executable relative to current directory (.%c%s)", file, filepath.Separator, path)
-}
-
func LookPath(file string) (string, error) {
- path, err := exec.LookPath(file)
- if err != nil {
- return "", err
- }
- if filepath.Base(file) == file && !filepath.IsAbs(path) {
- return "", relError(file, path)
- }
- return path, nil
-}
-
-func fixCmd(name string, cmd *exec.Cmd) {
- if filepath.Base(name) == name && !filepath.IsAbs(cmd.Path) {
- // exec.Command was called with a bare binary name and
- // exec.LookPath returned a path which is not absolute.
- // Set cmd.lookPathErr and clear cmd.Path so that it
- // cannot be run.
- lookPathErr := (*error)(unsafe.Pointer(reflect.ValueOf(cmd).Elem().FieldByName("lookPathErr").Addr().Pointer()))
- if *lookPathErr == nil {
- *lookPathErr = relError(name, cmd.Path)
- }
- cmd.Path = ""
- }
+ return exec.LookPath(file)
}
func CommandContext(ctx context.Context, name string, arg ...string) *exec.Cmd {
- cmd := exec.CommandContext(ctx, name, arg...)
- fixCmd(name, cmd)
- return cmd
-
+ return exec.CommandContext(ctx, name, arg...)
}
func Command(name string, arg ...string) *exec.Cmd {
- cmd := exec.Command(name, arg...)
- fixCmd(name, cmd)
- return cmd
+ return exec.Command(name, arg...)
}
diff --git a/src/internal/execabs/execabs_test.go b/src/internal/execabs/execabs_test.go
deleted file mode 100644
index 97a3f39..0000000
--- a/src/internal/execabs/execabs_test.go
+++ /dev/null
@@ -1,103 +0,0 @@
-// Copyright 2020 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package execabs
-
-import (
- "context"
- "fmt"
- "internal/testenv"
- "os"
- "os/exec"
- "path/filepath"
- "runtime"
- "testing"
-)
-
-func TestFixCmd(t *testing.T) {
- cmd := &exec.Cmd{Path: "hello"}
- fixCmd("hello", cmd)
- if cmd.Path != "" {
- t.Error("fixCmd didn't clear cmd.Path")
- }
- expectedErr := fmt.Sprintf("hello resolves to executable relative to current directory (.%chello)", filepath.Separator)
- if err := cmd.Run(); err == nil {
- t.Fatal("Command.Run didn't fail")
- } else if err.Error() != expectedErr {
- t.Fatalf("Command.Run returned unexpected error: want %q, got %q", expectedErr, err.Error())
- }
-}
-
-func TestCommand(t *testing.T) {
- testenv.MustHaveExec(t)
-
- for _, cmd := range []func(string) *Cmd{
- func(s string) *Cmd { return Command(s) },
- func(s string) *Cmd { return CommandContext(context.Background(), s) },
- } {
- tmpDir := t.TempDir()
- executable := "execabs-test"
- if runtime.GOOS == "windows" {
- executable += ".exe"
- }
- if err := os.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0111); err != nil {
- t.Fatalf("os.WriteFile failed: %s", err)
- }
- cwd, err := os.Getwd()
- if err != nil {
- t.Fatalf("os.Getwd failed: %s", err)
- }
- defer os.Chdir(cwd)
- if err = os.Chdir(tmpDir); err != nil {
- t.Fatalf("os.Chdir failed: %s", err)
- }
- if runtime.GOOS != "windows" {
- // add "." to PATH so that exec.LookPath looks in the current directory on
- // non-windows platforms as well
- origPath := os.Getenv("PATH")
- defer os.Setenv("PATH", origPath)
- os.Setenv("PATH", fmt.Sprintf(".:%s", origPath))
- }
- expectedErr := fmt.Sprintf("execabs-test resolves to executable relative to current directory (.%c%s)", filepath.Separator, executable)
- if err = cmd("execabs-test").Run(); err == nil {
- t.Fatalf("Command.Run didn't fail when exec.LookPath returned a relative path")
- } else if err.Error() != expectedErr {
- t.Errorf("Command.Run returned unexpected error: want %q, got %q", expectedErr, err.Error())
- }
- }
-}
-
-func TestLookPath(t *testing.T) {
- testenv.MustHaveExec(t)
-
- tmpDir := t.TempDir()
- executable := "execabs-test"
- if runtime.GOOS == "windows" {
- executable += ".exe"
- }
- if err := os.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0111); err != nil {
- t.Fatalf("os.WriteFile failed: %s", err)
- }
- cwd, err := os.Getwd()
- if err != nil {
- t.Fatalf("os.Getwd failed: %s", err)
- }
- defer os.Chdir(cwd)
- if err = os.Chdir(tmpDir); err != nil {
- t.Fatalf("os.Chdir failed: %s", err)
- }
- if runtime.GOOS != "windows" {
- // add "." to PATH so that exec.LookPath looks in the current directory on
- // non-windows platforms as well
- origPath := os.Getenv("PATH")
- defer os.Setenv("PATH", origPath)
- os.Setenv("PATH", fmt.Sprintf(".:%s", origPath))
- }
- expectedErr := fmt.Sprintf("execabs-test resolves to executable relative to current directory (.%c%s)", filepath.Separator, executable)
- if _, err := LookPath("execabs-test"); err == nil {
- t.Fatalf("LookPath didn't fail when finding a non-relative path")
- } else if err.Error() != expectedErr {
- t.Errorf("LookPath returned unexpected error: want %q, got %q", expectedErr, err.Error())
- }
-}
diff --git a/src/os/exec/dot_test.go b/src/os/exec/dot_test.go
new file mode 100644
index 0000000..932d907
--- /dev/null
+++ b/src/os/exec/dot_test.go
@@ -0,0 +1,87 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package exec_test
+
+import (
+ "errors"
+ "internal/testenv"
+ "os"
+ . "os/exec"
+ "path/filepath"
+ "runtime"
+ "strings"
+ "testing"
+)
+
+func TestLookPath(t *testing.T) {
+ testenv.MustHaveExec(t)
+
+ tmpDir := filepath.Join(t.TempDir(), "testdir")
+ if err := os.Mkdir(tmpDir, 0777); err != nil {
+ t.Fatal(err)
+ }
+
+ executable := "execabs-test"
+ if runtime.GOOS == "windows" {
+ executable += ".exe"
+ }
+ if err := os.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0777); err != nil {
+ t.Fatal(err)
+ }
+ cwd, err := os.Getwd()
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer func() {
+ if err := os.Chdir(cwd); err != nil {
+ panic(err)
+ }
+ }()
+ if err = os.Chdir(tmpDir); err != nil {
+ t.Fatal(err)
+ }
+ origPath := os.Getenv("PATH")
+ defer os.Setenv("PATH", origPath)
+
+ // Add "." to PATH so that exec.LookPath looks in the current directory on all systems.
+ // And try to trick it with "../testdir" too.
+ for _, dir := range []string{".", "../testdir"} {
+ os.Setenv("PATH", dir+string(filepath.ListSeparator)+origPath)
+ t.Run("PATH="+dir, func(t *testing.T) {
+ good := dir + "/execabs-test"
+ if found, err := LookPath(good); err != nil || !strings.HasPrefix(found, good) {
+ t.Fatalf("LookPath(%q) = %q, %v, want \"%s...\", nil", good, found, err, good)
+ }
+ if runtime.GOOS == "windows" {
+ good = dir + `\execabs-test`
+ if found, err := LookPath(good); err != nil || !strings.HasPrefix(found, good) {
+ t.Fatalf("LookPath(%q) = %q, %v, want \"%s...\", nil", good, found, err, good)
+ }
+ }
+
+ if _, err := LookPath("execabs-test"); err == nil {
+ t.Fatalf("LookPath didn't fail when finding a non-relative path")
+ } else if !errors.Is(err, ErrDot) {
+ t.Fatalf("LookPath returned unexpected error: want Is ErrDot, got %q", err)
+ }
+
+ cmd := Command("execabs-test")
+ if cmd.Err == nil {
+ t.Fatalf("Command didn't fail when finding a non-relative path")
+ } else if !errors.Is(cmd.Err, ErrDot) {
+ t.Fatalf("Command returned unexpected error: want Is ErrDot, got %q", cmd.Err)
+ }
+ cmd.Err = nil
+
+ // Clearing cmd.Err should let the execution proceed,
+ // and it should fail because it's not a valid binary.
+ if err := cmd.Run(); err == nil {
+ t.Fatalf("Run did not fail: expected exec error")
+ } else if errors.Is(err, ErrDot) {
+ t.Fatalf("Run returned unexpected error ErrDot: want error like ENOEXEC: %q", err)
+ }
+ })
+ }
+}
diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index 91c2e00..dad63f1 100644
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -18,6 +18,71 @@
// Note that the examples in this package assume a Unix system.
// They may not run on Windows, and they do not run in the Go Playground
// used by golang.org and godoc.org.
+//
+// Executables in the current directory
+//
+// The functions Command and LookPath look for a program
+// in the directories listed in the current path, following the
+// conventions of the host operating system.
+// Operating systems have for decades included the current
+// directory in this search, sometimes implicitly and sometimes
+// configured explicitly that way by default.
+// Modern practice is that including the current directory
+// is usually unexpected and often leads to security problems.
+//
+// To avoid those security problems, as of Go 1.19, this package will not resolve a program
+// using an implicit or explicit path entry relative to the current directory.
+// That is, if you run exec.LookPath("go"), it will not successfully return
+// ./go on Unix nor .\go.exe on Windows, no matter how the path is configured.
+// Instead, if the usual path algorithms would result in that answer,
+// these functions return an error err satisfying errors.Is(err, ErrDot).
+//
+// For example, consider these two program snippets:
+//
+// path, err := exec.LookPath("prog")
+// if err != nil {
+// log.Fatal(err)
+// }
+// use(path)
+//
+// and
+//
+// cmd := exec.Command("prog")
+// if err := cmd.Run(); err != nil {
+// log.Fatal(err)
+// }
+//
+// These will not find and run ./prog or .\prog.exe,
+// no matter how the current path is configured.
+//
+// Code that always wants to run a program from the current directory
+// can be rewritten to say "./prog" instead of "prog".
+//
+// Code that insists on including results from relative path entries
+// can instead override the error using an errors.Is check:
+//
+// path, err := exec.LookPath("prog")
+// if errors.Is(err, exec.ErrDot) {
+// err = nil
+// }
+// if err != nil {
+// log.Fatal(err)
+// }
+// use(path)
+//
+// and
+//
+// cmd := exec.Command("prog")
+// if errors.Is(cmd.Err, exec.ErrDot) {
+// cmd.Err = nil
+// }
+// if err := cmd.Run(); err != nil {
+// log.Fatal(err)
+// }
+//
+// Before adding such overrides, make sure you understand the
+// security implications of doing so.
+// See https://go.dev/blog/path-security for more information.
package exec
import (
@@ -134,7 +199,7 @@
ProcessState *os.ProcessState
ctx context.Context // nil means none
- lookPathErr error // LookPath error, if any.
+ Err error // LookPath error, if any.
finished bool // when Wait was called
childFiles []*os.File
closeAfterStart []io.Closer
@@ -142,6 +207,25 @@
goroutine []func() error
errch chan error // one send per goroutine
waitDone chan struct{}
+
+ // For a security release long ago, we created x/sys/execabs,
+ // which manipulated the unexported lookPathErr error field
+ // in this struct. For Go 1.19 we exported the field as Err error,
+ // above, but we have to keep lookPathErr around for use by
+ // old programs building against new toolchains.
+ // The String and Start methods look for an error in lookPathErr
+ // in preference to Err, to preserve the errors that execabs sets.
+ //
+ // In general we don't guarantee misuse of reflect like this,
+ // but the misuse of reflect was by us, the best of various bad
+ // options to fix the security problem, and people depend on
+ // those old copies of execabs continuing to work.
+ // The result is that we have to leave this variable around for the
+ // rest of time, a compatibility scar.
+ //
+ // See https://go.dev/blog/path-security
+ // and https://go.dev/issue/43724 for more context.
+ lookPathErr error
}
// Command returns the Cmd struct to execute the named program with
@@ -173,7 +257,7 @@
}
if filepath.Base(name) == name {
if lp, err := LookPath(name); err != nil {
- cmd.lookPathErr = err
+ cmd.Err = err
} else {
cmd.Path = lp
}
@@ -200,7 +284,7 @@
// In particular, it is not suitable for use as input to a shell.
// The output of String may vary across Go releases.
func (c *Cmd) String() string {
- if c.lookPathErr != nil {
+ if c.Err != nil || c.lookPathErr != nil {
// failed to resolve path; report the original requested path (plus args)
return strings.Join(c.Args, " ")
}
@@ -335,7 +419,7 @@
// lookExtensions does not search PATH, instead it converts `prog` into `.\prog`.
func lookExtensions(path, dir string) (string, error) {
if filepath.Base(path) == path {
- path = filepath.Join(".", path)
+ path = "." + string(filepath.Separator) + path
}
if dir == "" {
return LookPath(path)
@@ -363,10 +447,13 @@
// The Wait method will return the exit code and release associated resources
// once the command exits.
func (c *Cmd) Start() error {
- if c.lookPathErr != nil {
+ if c.Err != nil || c.lookPathErr != nil {
c.closeDescriptors(c.closeAfterStart)
c.closeDescriptors(c.closeAfterWait)
- return c.lookPathErr
+ if c.lookPathErr != nil {
+ return c.lookPathErr
+ }
+ return c.Err
}
if runtime.GOOS == "windows" {
lp, err := lookExtensions(c.Path, c.Dir)
@@ -845,3 +932,12 @@
}
return append(env, "SYSTEMROOT="+os.Getenv("SYSTEMROOT"))
}
+
+// ErrDot indicates that a path lookup resolved to an executable
+// in the current directory due to ‘.’ being in the path, either
+// implicitly or explicitly. See the package documentation for details.
+//
+// Note that functions in this package do not return ErrDot directly.
+// Code should use errors.Is(err, ErrDot), not err == ErrDot,
+// to test whether a returned error err is due to this condition.
+var ErrDot = errors.New("cannot run executable found relative to current directory")
diff --git a/src/os/exec/lp_plan9.go b/src/os/exec/lp_plan9.go
index e8826a5..6822481 100644
--- a/src/os/exec/lp_plan9.go
+++ b/src/os/exec/lp_plan9.go
@@ -30,7 +30,11 @@
// directories named by the path environment variable.
// If file begins with "/", "#", "./", or "../", it is tried
// directly and the path is not consulted.
-// The result may be an absolute path or a path relative to the current directory.
+// On success, the result is an absolute path.
+//
+// In older versions of Go, LookPath could return a path relative to the current directory.
+// As of Go 1.19, LookPath will instead return that path along with an error satisfying
+// errors.Is(err, ErrDot). See the package documentation for more details.
func LookPath(file string) (string, error) {
// skip the path lookup for these prefixes
skip := []string{"/", "#", "./", "../"}
@@ -49,6 +53,9 @@
for _, dir := range filepath.SplitList(path) {
path := filepath.Join(dir, file)
if err := findExecutable(path); err == nil {
+ if !filepath.IsAbs(path) {
+ return path, &Error{file, ErrDot}
+ }
return path, nil
}
}
diff --git a/src/os/exec/lp_unix.go b/src/os/exec/lp_unix.go
index 5db6c5e..9833205 100644
--- a/src/os/exec/lp_unix.go
+++ b/src/os/exec/lp_unix.go
@@ -31,7 +31,11 @@
// LookPath searches for an executable named file in the
// directories named by the PATH environment variable.
// If file contains a slash, it is tried directly and the PATH is not consulted.
-// The result may be an absolute path or a path relative to the current directory.
+// Otherwise, on success, the result is an absolute path.
+//
+// In older versions of Go, LookPath could return a path relative to the current directory.
+// As of Go 1.19, LookPath will instead return that path along with an error satisfying
+// errors.Is(err, ErrDot). See the package documentation for more details.
func LookPath(file string) (string, error) {
// NOTE(rsc): I wish we could use the Plan 9 behavior here
// (only bypass the path if file begins with / or ./ or ../)
@@ -52,6 +56,9 @@
}
path := filepath.Join(dir, file)
if err := findExecutable(path); err == nil {
+ if !filepath.IsAbs(path) {
+ return path, &Error{file, ErrDot}
+ }
return path, nil
}
}
diff --git a/src/os/exec/lp_windows.go b/src/os/exec/lp_windows.go
index e7a2cdf..dab5770 100644
--- a/src/os/exec/lp_windows.go
+++ b/src/os/exec/lp_windows.go
@@ -10,6 +10,7 @@
"os"
"path/filepath"
"strings"
+ "syscall"
)
// ErrNotFound is the error resulting if a path search failed to find an executable file.
@@ -53,10 +54,14 @@
// LookPath searches for an executable named file in the
// directories named by the PATH environment variable.
-// If file contains a slash, it is tried directly and the PATH is not consulted.
// LookPath also uses PATHEXT environment variable to match
// a suitable candidate.
-// The result may be an absolute path or a path relative to the current directory.
+// If file contains a slash, it is tried directly and the PATH is not consulted.
+// Otherwise, on success, the result is an absolute path.
+//
+// In older versions of Go, LookPath could return a path relative to the current directory.
+// As of Go 1.19, LookPath will instead return that path along with an error satisfying
+// errors.Is(err, ErrDot). See the package documentation for more details.
func LookPath(file string) (string, error) {
var exts []string
x := os.Getenv(`PATHEXT`)
@@ -75,18 +80,34 @@
}
if strings.ContainsAny(file, `:\/`) {
- if f, err := findExecutable(file, exts); err == nil {
+ f, err := findExecutable(file, exts)
+ if err == nil {
return f, nil
- } else {
- return "", &Error{file, err}
+ }
+ return "", &Error{file, err}
+ }
+
+ // On Windows, creating the NoDefaultCurrentDirectoryInExePath
+ // environment variable (with any value or no value!) signals that
+ // path lookups should skip the current directory.
+ // In theory we are supposed to call NeedCurrentDirectoryForExePathW
+ // "as the registry location of this environment variable can change"
+ // but that seems exceedingly unlikely: it would break all users who
+ // have configured their environment this way!
+ // https://docs.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-needcurrentdirectoryforexepathw
+ // See also go.dev/issue/43947.
+ if _, found := syscall.Getenv("NoDefaultCurrentDirectoryInExePath"); !found {
+ if f, err := findExecutable(filepath.Join(".", file), exts); err == nil {
+ return f, &Error{file, ErrDot}
}
}
- if f, err := findExecutable(filepath.Join(".", file), exts); err == nil {
- return f, nil
- }
+
path := os.Getenv("path")
for _, dir := range filepath.SplitList(path) {
if f, err := findExecutable(filepath.Join(dir, file), exts); err == nil {
+ if !filepath.IsAbs(f) {
+ return f, &Error{file, ErrDot}
+ }
return f, nil
}
}
diff --git a/src/os/exec/lp_windows_test.go b/src/os/exec/lp_windows_test.go
index 34abe09..1f609ff 100644
--- a/src/os/exec/lp_windows_test.go
+++ b/src/os/exec/lp_windows_test.go
@@ -8,6 +8,7 @@
package exec_test
import (
+ "errors"
"fmt"
"internal/testenv"
"io"
@@ -36,6 +37,9 @@
func cmdExec(args ...string) {
cmd := exec.Command(args[1])
cmd.Dir = args[0]
+ if errors.Is(cmd.Err, exec.ErrDot) {
+ cmd.Err = nil
+ }
output, err := cmd.CombinedOutput()
if err != nil {
fmt.Fprintf(os.Stderr, "Child: %s %s", err, string(output))
@@ -329,7 +333,7 @@
},
}
-func TestLookPath(t *testing.T) {
+func TestLookPathWindows(t *testing.T) {
tmp := t.TempDir()
printpathExe := buildPrintPathExe(t, tmp)
To view, visit change 403274. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
May 3, 2022, 10:09:06 AM5/3/22
to Gopher Robot, Russ Cox, goph...@pubsubhelper.golang.org, Bryan Mills, golang-co...@googlegroups.com
1 comment:
Patchset:
This broke `x/sys/execabs` again. Filed as #52666 and mailed CL 403694 with what I hope is a fix.
To view, visit change 403274. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages