From f51931d2f3b74fad694f5a74ccfda9b5dc987f1c Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Thu, 25 Apr 2024 20:28:42 +0800 Subject: [PATCH 01/13] os/exec: not add a suffix to Cmd.Path For avoid data race for cmd.Path , in CL 527820 fixed data race , but addition of suffixe as shown in #66586 was also introduced. The result of call lookExtensions is actually the name passed to os.StartProcess, But solutions at the time chose to reuse cmd.Path to represent the name passed to os.StartProcess,since this results in #66586, So use new field save call lookExtensions result. Fixes #66586 Change-Id: Ib1baa6d7803f9471af6e38bcb55f0298422e6743 --- src/os/exec/exec.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 35e4e7e792fdbc..070495ccce0120 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -332,6 +332,10 @@ type Cmd struct { // See https://go.dev/blog/path-security // and https://go.dev/issue/43724 for more context. lookPathErr error + + // cacheLookExtensions cache the result of calling lookExtensions, + // use it only on windows. + cacheLookExtensions string } // A ctxResult reports the result of watching the Context associated with a @@ -437,9 +441,7 @@ func Command(name string, arg ...string) *Cmd { // cmd.Dir may be set after we return from this function and that may cause // the command to resolve to a different extension. lp, err := lookExtensions(name, "") - if lp != "" { - cmd.Path = lp - } + cmd.cacheLookExtensions = lp if err != nil { cmd.Err = err } @@ -640,8 +642,8 @@ func (c *Cmd) Start() error { } return c.Err } - lp := c.Path - if runtime.GOOS == "windows" && !filepath.IsAbs(c.Path) { + lp := c.cacheLookExtensions + if lp == "" && runtime.GOOS == "windows" { // If c.Path is relative, we had to wait until now // to resolve it in case c.Dir was changed. // (If it is absolute, we already resolved its extension in Command From 078683c4cb65408e8cf593af4b07cd4e123ee12f Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Thu, 25 Apr 2024 20:52:14 +0800 Subject: [PATCH 02/13] n Change-Id: Iaf48768865f63b9d828a2bf46478e72da65891c7 --- src/os/exec/exec.go | 46 ++++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 070495ccce0120..3087709e5435e0 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -642,27 +642,31 @@ func (c *Cmd) Start() error { } return c.Err } - lp := c.cacheLookExtensions - if lp == "" && runtime.GOOS == "windows" { - // If c.Path is relative, we had to wait until now - // to resolve it in case c.Dir was changed. - // (If it is absolute, we already resolved its extension in Command - // and shouldn't need to do so again.) - // - // Unfortunately, we cannot write the result back to c.Path because programs - // may assume that they can call Start concurrently with reading the path. - // (It is safe and non-racy to do so on Unix platforms, and users might not - // test with the race detector on all platforms; - // see https://go.dev/issue/62596.) - // - // So we will pass the fully resolved path to os.StartProcess, but leave - // c.Path as is: missing a bit of logging information seems less harmful - // than triggering a surprising data race, and if the user really cares - // about that bit of logging they can always use LookPath to resolve it. - var err error - lp, err = lookExtensions(c.Path, c.Dir) - if err != nil { - return err + lp := c.Path + if runtime.GOOS == "windows" { + if lp == "" { + // If c.Path is relative, we had to wait until now + // to resolve it in case c.Dir was changed. + // (If it is absolute, we already resolved its extension in Command + // and shouldn't need to do so again.) + // + // Unfortunately, we cannot write the result back to c.Path because programs + // may assume that they can call Start concurrently with reading the path. + // (It is safe and non-racy to do so on Unix platforms, and users might not + // test with the race detector on all platforms; + // see https://go.dev/issue/62596.) + // + // So we will pass the fully resolved path to os.StartProcess, but leave + // c.Path as is: missing a bit of logging information seems less harmful + // than triggering a surprising data race, and if the user really cares + // about that bit of logging they can always use LookPath to resolve it. + var err error + lp, err = lookExtensions(c.Path, c.Dir) + if err != nil { + return err + } + } else { + lp = c.cacheLookExtensions } } if c.Cancel != nil && c.ctx == nil { From ff7b0a6641546c291d68db2f5428210decce2570 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Thu, 25 Apr 2024 21:19:28 +0800 Subject: [PATCH 03/13] n Change-Id: I23878885974cddceb110566020f44a776e9af2f4 --- src/os/exec/exec.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 3087709e5435e0..88eb9a82c22a3c 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -665,7 +665,7 @@ func (c *Cmd) Start() error { if err != nil { return err } - } else { + } else if c.cacheLookExtensions != "" { lp = c.cacheLookExtensions } } From 73f07ac6d3144949f9b7646f8291b1cf67ef60db Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Thu, 25 Apr 2024 21:25:35 +0800 Subject: [PATCH 04/13] n Change-Id: Ia532697ea429907567ab156f6f50059a15e90fbb --- src/os/exec/exec.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 88eb9a82c22a3c..467bbfabc4ba0b 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -644,7 +644,7 @@ func (c *Cmd) Start() error { } lp := c.Path if runtime.GOOS == "windows" { - if lp == "" { + if c.cacheLookExtensions == "" { // If c.Path is relative, we had to wait until now // to resolve it in case c.Dir was changed. // (If it is absolute, we already resolved its extension in Command @@ -665,7 +665,7 @@ func (c *Cmd) Start() error { if err != nil { return err } - } else if c.cacheLookExtensions != "" { + } else { lp = c.cacheLookExtensions } } From 7ba34d55963ec2cf82612fadd6d578fda0f79768 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Fri, 26 Apr 2024 16:17:07 +0800 Subject: [PATCH 05/13] n Change-Id: Ia5035a9e583fffee355265ee18cb10609d9c14c1 --- src/os/exec/exec_windows_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/os/exec/exec_windows_test.go b/src/os/exec/exec_windows_test.go index efd37107edf542..c0deadc720da47 100644 --- a/src/os/exec/exec_windows_test.go +++ b/src/os/exec/exec_windows_test.go @@ -12,6 +12,7 @@ import ( "io" "os" "os/exec" + "path/filepath" "strconv" "strings" "syscall" @@ -107,3 +108,16 @@ func TestChildCriticalEnv(t *testing.T) { t.Error("no SYSTEMROOT found") } } + +func TestIssue66586(t *testing.T) { + testenv.MustHaveGoBuild(t) + path := filepath.Join(testenv.GOROOT(t), "bin", "go") + cmd := exec.Command(path, "version") + err := cmd.Run() + if err != nil { + t.Fatal(err) + } + if path != cmd.Path { + t.Fatalf("unexpected path: %s", cmd.Path) + } +} From b41c326363cd8996ccc40f6c528cef3d44f706f1 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Sun, 28 Apr 2024 09:43:24 +0800 Subject: [PATCH 06/13] n Change-Id: I22c8737d1c6201f5d1b63ff70b85b04df3863e0f --- src/os/exec/exec.go | 55 ++++++++++++++++---------------- src/os/exec/exec_windows_test.go | 2 +- 2 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 467bbfabc4ba0b..60531dca72de5d 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -333,9 +333,9 @@ type Cmd struct { // and https://go.dev/issue/43724 for more context. lookPathErr error - // cacheLookExtensions cache the result of calling lookExtensions, - // use it only on windows. - cacheLookExtensions string + // callLookExtensions indicate whether + // lookExtensions has been called in Cmd.Command. + callLookExtensions bool } // A ctxResult reports the result of watching the Context associated with a @@ -440,8 +440,11 @@ func Command(name string, arg ...string) *Cmd { // Note that we cannot add an extension here for relative paths, because // cmd.Dir may be set after we return from this function and that may cause // the command to resolve to a different extension. + cmd.callLookExtensions = true lp, err := lookExtensions(name, "") - cmd.cacheLookExtensions = lp + if lp != "" { + cmd.Path = lp + } if err != nil { cmd.Err = err } @@ -643,30 +646,26 @@ func (c *Cmd) Start() error { return c.Err } lp := c.Path - if runtime.GOOS == "windows" { - if c.cacheLookExtensions == "" { - // If c.Path is relative, we had to wait until now - // to resolve it in case c.Dir was changed. - // (If it is absolute, we already resolved its extension in Command - // and shouldn't need to do so again.) - // - // Unfortunately, we cannot write the result back to c.Path because programs - // may assume that they can call Start concurrently with reading the path. - // (It is safe and non-racy to do so on Unix platforms, and users might not - // test with the race detector on all platforms; - // see https://go.dev/issue/62596.) - // - // So we will pass the fully resolved path to os.StartProcess, but leave - // c.Path as is: missing a bit of logging information seems less harmful - // than triggering a surprising data race, and if the user really cares - // about that bit of logging they can always use LookPath to resolve it. - var err error - lp, err = lookExtensions(c.Path, c.Dir) - if err != nil { - return err - } - } else { - lp = c.cacheLookExtensions + if runtime.GOOS == "windows" && !c.callLookExtensions { + // If c.Path is relative, we had to wait until now + // to resolve it in case c.Dir was changed. + // (If it is absolute, we already resolved its extension in Command + // and shouldn't need to do so again.) + // + // Unfortunately, we cannot write the result back to c.Path because programs + // may assume that they can call Start concurrently with reading the path. + // (It is safe and non-racy to do so on Unix platforms, and users might not + // test with the race detector on all platforms; + // see https://go.dev/issue/62596.) + // + // So we will pass the fully resolved path to os.StartProcess, but leave + // c.Path as is: missing a bit of logging information seems less harmful + // than triggering a surprising data race, and if the user really cares + // about that bit of logging they can always use LookPath to resolve it. + var err error + lp, err = lookExtensions(c.Path, c.Dir) + if err != nil { + return err } } if c.Cancel != nil && c.ctx == nil { diff --git a/src/os/exec/exec_windows_test.go b/src/os/exec/exec_windows_test.go index c0deadc720da47..5f16df93bd5e99 100644 --- a/src/os/exec/exec_windows_test.go +++ b/src/os/exec/exec_windows_test.go @@ -112,7 +112,7 @@ func TestChildCriticalEnv(t *testing.T) { func TestIssue66586(t *testing.T) { testenv.MustHaveGoBuild(t) path := filepath.Join(testenv.GOROOT(t), "bin", "go") - cmd := exec.Command(path, "version") + cmd := exec.Cmd{Path: path, Args: []string{path, "version"}} err := cmd.Run() if err != nil { t.Fatal(err) From d82fe3043379ca9794d5cb9196f08246be2314e8 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Mon, 29 Apr 2024 06:37:12 +0800 Subject: [PATCH 07/13] n Change-Id: I3d1f59eb50c754715d333526b84f7cebee65476a --- src/os/exec/exec.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 60531dca72de5d..dacfe2003c8dbf 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -333,9 +333,9 @@ type Cmd struct { // and https://go.dev/issue/43724 for more context. lookPathErr error - // callLookExtensions indicate whether + // calledLookExtensions indicates whether // lookExtensions has been called in Cmd.Command. - callLookExtensions bool + calledLookExtensions bool } // A ctxResult reports the result of watching the Context associated with a @@ -440,7 +440,7 @@ func Command(name string, arg ...string) *Cmd { // Note that we cannot add an extension here for relative paths, because // cmd.Dir may be set after we return from this function and that may cause // the command to resolve to a different extension. - cmd.callLookExtensions = true + cmd.calledLookExtensions = true lp, err := lookExtensions(name, "") if lp != "" { cmd.Path = lp @@ -646,7 +646,7 @@ func (c *Cmd) Start() error { return c.Err } lp := c.Path - if runtime.GOOS == "windows" && !c.callLookExtensions { + if runtime.GOOS == "windows" && !c.calledLookExtensions { // If c.Path is relative, we had to wait until now // to resolve it in case c.Dir was changed. // (If it is absolute, we already resolved its extension in Command From 44f0084f54cc7697f3591569076d00ffdcbebe82 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Thu, 30 May 2024 21:23:11 +0800 Subject: [PATCH 08/13] n Change-Id: Ie084691584b08e17944079f182cc353a98191780 --- src/os/exec/exec.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index dacfe2003c8dbf..d646f234368e32 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -333,9 +333,9 @@ type Cmd struct { // and https://go.dev/issue/43724 for more context. lookPathErr error - // calledLookExtensions indicates whether - // lookExtensions has been called in Cmd.Command. - calledLookExtensions bool + // cacheLookExtensions cache the result of calling lookExtensions, + // use it only on windows. + cacheLookExtensions string } // A ctxResult reports the result of watching the Context associated with a @@ -440,11 +440,8 @@ func Command(name string, arg ...string) *Cmd { // Note that we cannot add an extension here for relative paths, because // cmd.Dir may be set after we return from this function and that may cause // the command to resolve to a different extension. - cmd.calledLookExtensions = true lp, err := lookExtensions(name, "") - if lp != "" { - cmd.Path = lp - } + cmd.cacheLookExtensions = lp if err != nil { cmd.Err = err } @@ -646,7 +643,10 @@ func (c *Cmd) Start() error { return c.Err } lp := c.Path - if runtime.GOOS == "windows" && !c.calledLookExtensions { + if c.cacheLookExtensions == "" { + lp = c.cacheLookExtensions + } + if runtime.GOOS == "windows" && c.cacheLookExtensions == "" { // If c.Path is relative, we had to wait until now // to resolve it in case c.Dir was changed. // (If it is absolute, we already resolved its extension in Command From f9ace65631ad25925d697a8c73a5cb765ce16e65 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Thu, 30 May 2024 21:38:32 +0800 Subject: [PATCH 09/13] n Change-Id: If135e6103cc0fac40a3724e2ba43a72f48c5fc9f --- src/os/exec/exec.go | 2 +- src/os/exec/exec_windows_test.go | 14 -------------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index d646f234368e32..2bdd78129d74ec 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -643,7 +643,7 @@ func (c *Cmd) Start() error { return c.Err } lp := c.Path - if c.cacheLookExtensions == "" { + if c.cacheLookExtensions != "" { lp = c.cacheLookExtensions } if runtime.GOOS == "windows" && c.cacheLookExtensions == "" { diff --git a/src/os/exec/exec_windows_test.go b/src/os/exec/exec_windows_test.go index 5f16df93bd5e99..efd37107edf542 100644 --- a/src/os/exec/exec_windows_test.go +++ b/src/os/exec/exec_windows_test.go @@ -12,7 +12,6 @@ import ( "io" "os" "os/exec" - "path/filepath" "strconv" "strings" "syscall" @@ -108,16 +107,3 @@ func TestChildCriticalEnv(t *testing.T) { t.Error("no SYSTEMROOT found") } } - -func TestIssue66586(t *testing.T) { - testenv.MustHaveGoBuild(t) - path := filepath.Join(testenv.GOROOT(t), "bin", "go") - cmd := exec.Cmd{Path: path, Args: []string{path, "version"}} - err := cmd.Run() - if err != nil { - t.Fatal(err) - } - if path != cmd.Path { - t.Fatalf("unexpected path: %s", cmd.Path) - } -} From 7ee45f859accc48d923f342e8c94bc0e3d7b6281 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Thu, 30 May 2024 21:41:38 +0800 Subject: [PATCH 10/13] n Change-Id: Ib1a532c540c890ce988e4092699fd879baf9799f --- src/os/exec/exec.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 2bdd78129d74ec..b3bf5ec83cb2a9 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -334,7 +334,7 @@ type Cmd struct { lookPathErr error // cacheLookExtensions cache the result of calling lookExtensions, - // use it only on windows. + // set it only on windows. cacheLookExtensions string } From 197d6e445c67fb74bb429bda11b6fc17a5c0d037 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Fri, 31 May 2024 16:07:48 +0800 Subject: [PATCH 11/13] n Change-Id: I04351ac442c5d5cd25ddca84ad9e76a88bb21abb --- src/os/exec/exec_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go index c749de99db69e1..dbe59fea119e70 100644 --- a/src/os/exec/exec_test.go +++ b/src/os/exec/exec_test.go @@ -1835,3 +1835,32 @@ func TestPathRace(t *testing.T) { t.Logf("running in background: %v", cmd) <-done } + +func TestAbsPathExec(t *testing.T) { + testenv.MustHaveExec(t) + testenv.MustHaveGoBuild(t) // must have GOROOT/bin/gofmt, but close enough + + // A simple exec of a full path should work. + // Go 1.22 broke this on Windows, requiring ".exe"; see #66586. + exe := filepath.Join(testenv.GOROOT(t), "bin/gofmt") + cmd := exec.Command(exe) + if cmd.Path != exe { + t.Errorf("exec.Command(%#q) set Path=%#q", exe, cmd.Path) + } + err := cmd.Run() + if err != nil { + t.Errorf("using exec.Command(%#q): %v", exe, err) + } + + cmd = &exec.Cmd{Path: exe} + err = cmd.Run() + if err != nil { + t.Errorf("using exec.Cmd{Path: %#q}: %v", cmd.Path, err) + } + + cmd = &exec.Cmd{Path: "gofmt", Dir: "/"} + err = cmd.Run() + if err == nil { + t.Errorf("using exec.Cmd{Path: %#q}: unexpected success", cmd.Path) + } +} From b7431d6eeba20e073c50df98b8824261a6ccd270 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Sat, 1 Jun 2024 08:49:04 +0800 Subject: [PATCH 12/13] n Change-Id: Ibeb7ff0def24354435a3784dceb22245463a21f1 --- src/os/exec/exec.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index b3bf5ec83cb2a9..19d4bedfc8e01a 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -333,9 +333,9 @@ type Cmd struct { // and https://go.dev/issue/43724 for more context. lookPathErr error - // cacheLookExtensions cache the result of calling lookExtensions, - // set it only on windows. - cacheLookExtensions string + // cachedLookExtensions caches the result of calling lookExtensions. + // This is only used on Windows. + cachedLookExtensions string } // A ctxResult reports the result of watching the Context associated with a @@ -434,8 +434,7 @@ func Command(name string, arg ...string) *Cmd { // We may need to add a filename extension from PATHEXT // or verify an extension that is already present. // Since the path is absolute, its extension should be unambiguous - // and independent of cmd.Dir, and we can go ahead and update cmd.Path to - // reflect it. + // and independent of cmd.Dir, and we can go ahead and cache the lookup now. // // Note that we cannot add an extension here for relative paths, because // cmd.Dir may be set after we return from this function and that may cause From dc3169f2350f61acac5ef7842b7514013abacbe1 Mon Sep 17 00:00:00 2001 From: qiulaidongfeng <2645477756@qq.com> Date: Sat, 1 Jun 2024 08:57:56 +0800 Subject: [PATCH 13/13] n Change-Id: Iaaa821da8451bab91e81ea457b3ea6f32ff91823 --- src/os/exec/exec.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 19d4bedfc8e01a..50ed3a8d165886 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -440,7 +440,7 @@ func Command(name string, arg ...string) *Cmd { // cmd.Dir may be set after we return from this function and that may cause // the command to resolve to a different extension. lp, err := lookExtensions(name, "") - cmd.cacheLookExtensions = lp + cmd.cachedLookExtensions = lp if err != nil { cmd.Err = err } @@ -642,10 +642,10 @@ func (c *Cmd) Start() error { return c.Err } lp := c.Path - if c.cacheLookExtensions != "" { - lp = c.cacheLookExtensions + if c.cachedLookExtensions != "" { + lp = c.cachedLookExtensions } - if runtime.GOOS == "windows" && c.cacheLookExtensions == "" { + if runtime.GOOS == "windows" && c.cachedLookExtensions == "" { // If c.Path is relative, we had to wait until now // to resolve it in case c.Dir was changed. // (If it is absolute, we already resolved its extension in Command