diff --git a/go/exec/exec.go b/go/exec/exec.go index c807135..5e30798 100644 --- a/go/exec/exec.go +++ b/go/exec/exec.go @@ -219,7 +219,11 @@ func (c *Cmd) prepare() core.Result { return core.Fail(core.E("Cmd.prepare", "exec: command context is required", ErrCommandContextRequired)) } - c.cmd = commandContext(c.ctx, c.name, c.args...) + resolved := commandContext(c.ctx, c.name, c.args...) + if !resolved.OK { + return resolved + } + c.cmd = resolved.Value.(*core.Cmd) c.cmd.Dir = c.opts.Dir if len(c.opts.Env) > 0 { @@ -285,26 +289,51 @@ func (c *Cmd) logError(msg string, failure core.Result) { c.getLogger().Error(msg, "cmd", c.name, "args", core.Join(" ", c.args...), "err", failure.Error()) } -func commandContext(ctx context.Context, name string, arg ...string) *core.Cmd { - path := name - if result := lookPath(name); result.OK { - path = result.Value.(string) +// commandContext resolves name to a concrete executable and builds the handle +// for it. +// +// Resolution failure is returned, never swallowed. Handing the bare name to +// os/exec instead is not a harmless fallback on Windows: when Dir is set, +// Cmd.Start resolves a separator-free Path relative to Dir, so an unresolved +// "git" is reported as `exec: "C:\...\some work dir\git"` — a confusing error +// naming a path nobody asked for, in place of the honest "not found on PATH". +func commandContext(ctx context.Context, name string, arg ...string) core.Result { + resolved := lookPath(name) + if !resolved.OK { + return resolved } - - cmd := &core.Cmd{ - Path: path, + return core.Ok(&core.Cmd{ + Path: resolved.Value.(string), Args: append([]string{name}, arg...), - } - return cmd + }) } +// defaultPathExt is the extension list Windows itself assumes when %PATHEXT% +// is unset. +const defaultPathExt = ".COM;.EXE;.BAT;.CMD" + +// lookPath resolves file to a runnable path, searching PATH when file carries +// no directory component. +// +// On Windows a command is named without its extension — "git", not "git.exe" — +// so every candidate is also tried with each %PATHEXT% suffix. Without that, +// no Windows executable is ever found by its bare name. func lookPath(file string) core.Result { + return lookPathWith(file, executableExtensions()) +} + +// lookPathWith is lookPath with the extension list supplied rather than read +// from the environment. Taking it as an argument is what lets the Windows +// resolution rules be pinned on a POSIX runner — the tests drive it with a +// fixture PATH and a fake %PATHEXT%, so no Windows box is needed to prove the +// logic and the CI lane is left to prove only the wiring. +func lookPathWith(file string, extensions []string) core.Result { if file == "" { return core.Fail(core.E("lookPath", "executable file not found in PATH", nil)) } - if core.Contains(file, string(core.PathSeparator)) { - if isExecutable(file) { - return core.Ok(file) + if containsSeparator(file) { + if path, ok := firstExecutable(file, extensions); ok { + return core.Ok(path) } return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found", file), nil)) } @@ -313,15 +342,111 @@ func lookPath(file string) core.Result { if dir == "" { dir = "." } - path := core.PathJoin(dir, file) - if isExecutable(path) { + if path, ok := firstExecutable(core.PathJoin(dir, file), extensions); ok { return core.Ok(path) } } return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found in PATH", file), nil)) } +// firstExecutable returns the first of base's candidate spellings that names a +// runnable file. +func firstExecutable(base string, extensions []string) (string, bool) { + for _, candidate := range executableCandidates(base, extensions) { + if isExecutableWith(candidate, extensions) { + return candidate, true + } + } + return "", false +} + +// executableCandidates returns the spellings of base to try, in order. With no +// extensions in play — every POSIX case — base stands alone. On Windows a base +// that already ends in a listed extension also stands alone; anything else is +// tried once per extension, so "git" becomes "git.com", "git.exe" and so on. +func executableCandidates(base string, extensions []string) []string { + if len(extensions) == 0 || hasExecutableExtension(base, extensions) { + return []string{base} + } + candidates := make([]string, 0, len(extensions)) + for _, extension := range extensions { + candidates = append(candidates, base+extension) + } + return candidates +} + +// hasExecutableExtension reports whether base already ends in one of the +// listed extensions. Windows filenames are case-insensitive, so the comparison +// is too. +func hasExecutableExtension(base string, extensions []string) bool { + lowered := core.Lower(base) + for _, extension := range extensions { + if core.HasSuffix(lowered, extension) { + return true + } + } + return false +} + +// executableExtensions returns the %PATHEXT% list, or nil off Windows where a +// command name is used exactly as written. +func executableExtensions() []string { + if string(core.PathSeparator) != `\` { + return nil + } + return parsePathExt(core.Getenv("PATHEXT")) +} + +// parsePathExt normalises a %PATHEXT% value into lower-cased, dot-prefixed +// extensions, dropping blanks and duplicates. An unset or unusable value falls +// back to the set Windows assumes, so a stripped environment still resolves +// the common executables. +func parsePathExt(value string) []string { + extensions := make([]string, 0, 8) + seen := make(map[string]bool, 8) + for _, field := range core.Split(value, ";") { + extension := core.Lower(core.Trim(field)) + if extension == "" || extension == "." { + continue + } + if !core.HasPrefix(extension, ".") { + extension = "." + extension + } + if seen[extension] { + continue + } + seen[extension] = true + extensions = append(extensions, extension) + } + if len(extensions) == 0 && value != defaultPathExt { + return parsePathExt(defaultPathExt) + } + return extensions +} + +// containsSeparator reports whether file carries a directory component under +// either convention. Windows accepts '/' as well as '\', so a name spelled +// "bin/tool" there is a path to check directly, not a name to hunt on PATH. +func containsSeparator(file string) bool { + if core.Contains(file, "/") { + return true + } + separator := string(core.PathSeparator) + return separator != "/" && core.Contains(file, separator) +} + func isExecutable(path string) bool { + return isExecutableWith(path, executableExtensions()) +} + +// isExecutableWith applies the platform's own rule for "this can be run". +// +// POSIX asks the mode bits. Windows has no execute bit — os.Stat synthesises +// 0666, or 0444 for a read-only file — so mode&0111 is never set there and a +// mode test rejects every file, git.exe included. Under a non-empty extension +// list the question becomes whether the suffix is one %PATHEXT% names, which +// is what Windows itself keys on. +func isExecutableWith(path string, extensions []string) bool { stat := core.Stat(path) if !stat.OK { return false @@ -330,5 +455,8 @@ func isExecutable(path string) bool { if !ok || info.IsDir() { return false } + if len(extensions) > 0 { + return hasExecutableExtension(path, extensions) + } return info.Mode()&0111 != 0 } diff --git a/go/exec/exec_internal_test.go b/go/exec/exec_internal_test.go index c676ba3..41752e9 100644 --- a/go/exec/exec_internal_test.go +++ b/go/exec/exec_internal_test.go @@ -84,6 +84,246 @@ func TestExecInternal_isExecutable_Ugly(t *testing.T) { } } +// The extension helpers below carry the Windows resolution rules. They take +// the extension list as an argument rather than reading %PATHEXT%, so the +// semantics are pinned hermetically on every runner — no Windows box needed +// for the logic, and the CI lane proves the wiring. + +func TestExecInternal_parsePathExt_Good(t *testing.T) { + got := parsePathExt(".COM;.EXE;.BAT;.CMD") + want := []string{".com", ".exe", ".bat", ".cmd"} + assertExtensions(t, got, want) + + // Order is significant — it is the order Windows tries them in. + if got[0] != ".com" || got[1] != ".exe" { + t.Fatalf("parsePathExt lost %%PATHEXT%% ordering: %v", got) + } +} + +func TestExecInternal_parsePathExt_Bad(t *testing.T) { + // An unset or content-free value falls back to the set Windows assumes, + // so a stripped environment still resolves the common executables. + for _, value := range []string{"", " ", ";;;", ".", "; . ;"} { + assertExtensions(t, parsePathExt(value), []string{".com", ".exe", ".bat", ".cmd"}) + } +} + +func TestExecInternal_parsePathExt_Ugly(t *testing.T) { + // Dot-less entries are accepted (some installers write PATHEXT that way), + // case is normalised, blanks and repeats are dropped, and a repeat does + // not displace the first occurrence's position. + assertExtensions(t, + parsePathExt("EXE; .Bat ;;exe;.BAT;.ps1"), + []string{".exe", ".bat", ".ps1"}) +} + +func TestExecInternal_executableCandidates_Good(t *testing.T) { + // A bare Windows name is tried once per extension, in order. + assertExtensions(t, + executableCandidates("C:/tools/git", []string{".com", ".exe"}), + []string{"C:/tools/git.com", "C:/tools/git.exe"}) +} + +func TestExecInternal_executableCandidates_Bad(t *testing.T) { + // With no extensions in play — every POSIX case — the name stands alone + // and is never suffixed. + assertExtensions(t, executableCandidates("/usr/bin/git", nil), []string{"/usr/bin/git"}) + assertExtensions(t, executableCandidates("/usr/bin/git", []string{}), []string{"/usr/bin/git"}) +} + +func TestExecInternal_executableCandidates_Ugly(t *testing.T) { + // A name that already carries a listed extension stands alone — it must + // not become "git.exe.exe". + assertExtensions(t, + executableCandidates("C:/tools/git.exe", []string{".com", ".exe"}), + []string{"C:/tools/git.exe"}) + + // Matching is case-insensitive, as Windows filenames are. + assertExtensions(t, + executableCandidates("C:/tools/GIT.EXE", []string{".com", ".exe"}), + []string{"C:/tools/GIT.EXE"}) + + // An extension that is NOT listed is not an extension for this purpose: + // "archive.tar" is a name to suffix, not an executable already. + assertExtensions(t, + executableCandidates("archive.tar", []string{".exe"}), + []string{"archive.tar.exe"}) +} + +func TestExecInternal_hasExecutableExtension_Good(t *testing.T) { + if !hasExecutableExtension("git.exe", []string{".com", ".exe"}) { + t.Fatal("expected git.exe to match .exe") + } + if !hasExecutableExtension("GIT.CMD", []string{".cmd"}) { + t.Fatal("expected case-insensitive matching") + } +} + +func TestExecInternal_hasExecutableExtension_Bad(t *testing.T) { + if hasExecutableExtension("git", []string{".com", ".exe"}) { + t.Fatal("expected a bare name to match nothing") + } + if hasExecutableExtension("git.exe", nil) { + t.Fatal("expected an empty extension list to match nothing") + } +} + +func TestExecInternal_containsSeparator_Good(t *testing.T) { + // '/' is a directory component on every platform, so a name spelled with + // it is a path to check directly rather than a name to hunt on PATH. + if !containsSeparator("bin/tool") { + t.Fatal("expected bin/tool to read as a path") + } + if !containsSeparator("/usr/bin/git") { + t.Fatal("expected an absolute POSIX path to read as a path") + } +} + +func TestExecInternal_containsSeparator_Bad(t *testing.T) { + if containsSeparator("git") { + t.Fatal("expected a bare name to carry no directory component") + } + if containsSeparator("") { + t.Fatal("expected an empty name to carry no directory component") + } +} + +// TestExecInternal_lookPathWith_Good is the receipt for the bug this fix +// exists for: on Windows a command is named without its extension, so a bare +// "tool" has to resolve to "tool.exe". Driving the extension list directly +// lets that be proven on a POSIX runner against a fixture PATH. +func TestExecInternal_lookPathWith_Good(t *testing.T) { + dir := t.TempDir() + // 0644, deliberately: under the Windows rule the mode bits are not the + // question, and this file would be rejected by a mode&0111 test — which + // is precisely why no Windows executable was ever found before. + writeFixture(t, core.PathJoin(dir, "tool.exe"), 0o644) + t.Setenv("PATH", dir) + + result := lookPathWith("tool", []string{".com", ".exe"}) + if !result.OK { + t.Fatalf("expected bare \"tool\" to resolve to tool.exe, got %v", result.Error()) + } + if want := core.PathJoin(dir, "tool.exe"); result.Value.(string) != want { + t.Fatalf("resolved to %q, want %q", result.Value, want) + } +} + +// TestExecInternal_lookPathWith_Bad pins the POSIX contract as the same code +// path: with no extensions the name is used exactly as written, never +// suffixed, and the mode bits decide. +func TestExecInternal_lookPathWith_Bad(t *testing.T) { + dir := t.TempDir() + writeFixture(t, core.PathJoin(dir, "tool.exe"), 0o755) + t.Setenv("PATH", dir) + + if result := lookPathWith("tool", nil); result.OK { + t.Fatalf("expected no suffixing without extensions, resolved to %v", result.Value) + } + + // The non-executable file is skipped even though its name matches. + writeFixture(t, core.PathJoin(dir, "plain"), 0o644) + if result := lookPathWith("plain", nil); result.OK { + t.Fatalf("expected a 0644 file to be skipped, resolved to %v", result.Value) + } +} + +// TestExecInternal_lookPathWith_Ugly covers the order-sensitive and +// already-suffixed cases: %PATHEXT% order decides which of two matches wins, +// and a name given with its extension is not suffixed a second time. +func TestExecInternal_lookPathWith_Ugly(t *testing.T) { + dir := t.TempDir() + writeFixture(t, core.PathJoin(dir, "tool.com"), 0o644) + writeFixture(t, core.PathJoin(dir, "tool.exe"), 0o644) + t.Setenv("PATH", dir) + + // .com precedes .exe in the list, so .com wins. + result := lookPathWith("tool", []string{".com", ".exe"}) + if !result.OK || result.Value.(string) != core.PathJoin(dir, "tool.com") { + t.Fatalf("expected .com to win on list order, got %v (ok=%v)", result.Value, result.OK) + } + + // Reversing the list reverses the winner — order is the whole rule. + result = lookPathWith("tool", []string{".exe", ".com"}) + if !result.OK || result.Value.(string) != core.PathJoin(dir, "tool.exe") { + t.Fatalf("expected .exe to win on reversed order, got %v (ok=%v)", result.Value, result.OK) + } + + // A name already carrying a listed extension resolves as-is, not as + // "tool.exe.exe". + result = lookPathWith("tool.exe", []string{".com", ".exe"}) + if !result.OK || result.Value.(string) != core.PathJoin(dir, "tool.exe") { + t.Fatalf("expected tool.exe to resolve as-is, got %v (ok=%v)", result.Value, result.OK) + } + + // A path-qualified name takes the same extension treatment, and '/' reads + // as a separator even under the Windows rules. + result = lookPathWith(core.PathJoin(dir, "tool"), []string{".exe"}) + if !result.OK || result.Value.(string) != core.PathJoin(dir, "tool.exe") { + t.Fatalf("expected a path-qualified bare name to gain .exe, got %v (ok=%v)", result.Value, result.OK) + } +} + +// TestExecInternal_isExecutableWith_Ugly pins the rule swap directly: the same +// 0644 file is not executable by the POSIX rule and is by the Windows one, +// while a directory is neither however it is spelled. +func TestExecInternal_isExecutableWith_Ugly(t *testing.T) { + dir := t.TempDir() + path := core.PathJoin(dir, "tool.exe") + writeFixture(t, path, 0o644) + + if isExecutableWith(path, nil) { + t.Fatal("expected a 0644 file to fail the POSIX mode test") + } + if !isExecutableWith(path, []string{".exe"}) { + t.Fatal("expected a 0644 .exe to pass the Windows extension test") + } + + // An extension not on the list is not executable under the Windows rule + // even at 0755 — Windows does not consult the mode bits at all. + other := core.PathJoin(dir, "notes.txt") + writeFixture(t, other, 0o755) + if isExecutableWith(other, []string{".exe"}) { + t.Fatal("expected an unlisted extension to fail the Windows test") + } + + // A directory named like an executable is still not one. + subdir := core.PathJoin(dir, "bundle.exe") + if r := core.MkdirAll(subdir, 0o755); !r.OK { + t.Fatalf("mkdir failed: %v", r.Error()) + } + if isExecutableWith(subdir, []string{".exe"}) { + t.Fatal("expected a directory to be reported non-executable") + } +} + +// writeFixture creates a file with the given mode, failing the test if it +// cannot. WriteFile does not apply the mode to an existing file, so each +// fixture name is written once per test. +func writeFixture(t *testing.T, path string, mode core.FileMode) { + t.Helper() + if w := core.WriteFile(path, []byte("fixture"), mode); !w.OK { + t.Fatalf("write %s: %v", path, w.Error()) + } + if c := core.Chmod(path, mode); !c.OK { + t.Fatalf("chmod %s: %v", path, c.Error()) + } +} + +// assertExtensions compares two string slices element-wise, reporting the +// whole of both on mismatch so a reordering is readable. +func assertExtensions(t *testing.T, got, want []string) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("got %v (%d entries), want %v (%d entries)", got, len(got), want, len(want)) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("entry %d: got %q, want %q (full: %v vs %v)", i, got[i], want[i], got, want) + } + } +} + func TestExecInternal_watchContext_Bad(t *testing.T) { // nil context is a no-op, no panic. c := &Cmd{} @@ -118,18 +358,34 @@ func TestExecInternal_watchContext_Good(t *testing.T) { } func TestExecInternal_commandContext_Good(t *testing.T) { - // A resolvable name yields an absolute Path. - cmd := commandContext(context.Background(), "sh", "-c", "true") - if cmd == nil { - t.Fatal("expected a command") + // A resolvable name yields a command whose Path is the resolved binary + // while Args[0] keeps the name the caller wrote. + result := commandContext(context.Background(), "sh", "-c", "true") + if !result.OK { + t.Skipf("sh not resolvable on this platform: %v", result.Error()) + } + cmd, ok := result.Value.(*core.Cmd) + if !ok || cmd == nil { + t.Fatalf("expected a *core.Cmd, got %T", result.Value) } if cmd.Args[0] != "sh" { t.Fatalf("expected first arg sh, got %v", cmd.Args[0]) } + if cmd.Path == "sh" { + t.Fatal("expected Path to be resolved, not the bare name") + } +} - // An unresolvable name falls back to the raw name as Path. - cmd = commandContext(context.Background(), "definitely_not_a_real_binary_xyz") - if cmd.Path != "definitely_not_a_real_binary_xyz" { - t.Fatalf("expected raw name fallback, got %v", cmd.Path) +func TestExecInternal_commandContext_Bad(t *testing.T) { + // An unresolvable name FAILS rather than falling back to the raw name. + // The old fallback handed os/exec a separator-free Path, which Windows + // then resolves relative to Cmd.Dir — reporting the command as missing + // from a working directory the caller never nominated as its home. + result := commandContext(context.Background(), "definitely_not_a_real_binary_xyz") + if result.OK { + t.Fatalf("expected an unresolvable name to fail, got %v", result.Value) + } + if !core.Contains(result.Error(), "definitely_not_a_real_binary_xyz") { + t.Fatalf("expected the failure to name the command, got %q", result.Error()) } }