From 8d519d11d09a0ad7e7c33609a70b0173dda2c000 Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 12:07:22 +0100 Subject: [PATCH] fix(process): Program.Find used a SECOND, unfixed copy of lookPath MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.16.2 fixed executable resolution on Windows and did not fix executable resolution on Windows, because this module carried TWO copies of it: exec/exec.go lookPath <- fixed in v0.16.2 os_exec_link.go lookPath <- byte-identical, untouched Program.Find calls the second. os_exec_link.go:45 still read `return info.Mode()&0111 != 0` — the exact defect v0.16.2's own commit message spends three paragraphs on — so every consumer resolving through Program.Find kept failing with `Program.Find: "git": not found in PATH`, on a release that claimed the opposite. go-inference's windows lane measured it: 41 occurrences before v0.16.2, 41 after. A fix is not landed until you have grepped for its siblings. Byte-identical duplicates are how a fixed defect stays live. So the fix is not a second copy of the fix. internal/lookpath now holds ONE implementation and both consumers call it — because the duplication IS the defect's cause, and patching in place would guarantee a third divergence. The three Windows defects it carries are unchanged from v0.16.2: no %PATHEXT% expansion, a mode&0111 test the platform can never satisfy, and a path-vs-name check that missed '/'. commandContext no longer discards the resolution failure either. It records it on Cmd.Err exactly as exec.Command does with its own LookPath error — Start returns it without running anything. Deferring to Start rather than returning early is deliberate: an early return skipped Service.start's exited-event broadcast, which TestService_Actions/broadcasts_exited_event_on_start_failure caught. The callers' failure handling is untouched. Receipts — macOS, GOWORK=off (what CI runs): go test -count=1 ./... ok process 13.478s · exec 0.733s · internal/lookpath 0.499s · pkg/api 2.800s golangci-lint run ./... 0 issues gofmt -l · go vet: clean TestProgram_Find_UsesSharedResolution pins the miss itself: Program.Find must land on the same path the shared resolver reports. It fails the moment a third copy appears or the two drift apart again — which a version bump could not detect and did not. The resolution tests move with the code into internal/lookpath, keeping the fixture-PATH + fake-%PATHEXT% receipts that prove the Windows rules on a POSIX runner. Co-Authored-By: Virgil --- go.work.sum | 4 + go/exec/exec.go | 156 +------------ go/exec/exec_internal_test.go | 311 ------------------------- go/internal/lookpath/lookpath.go | 173 ++++++++++++++ go/internal/lookpath/lookpath_test.go | 320 ++++++++++++++++++++++++++ go/os_exec_link.go | 69 +++--- go/program.go | 3 +- go/program_test.go | 35 +++ 8 files changed, 562 insertions(+), 509 deletions(-) create mode 100644 go/internal/lookpath/lookpath.go create mode 100644 go/internal/lookpath/lookpath_test.go diff --git a/go.work.sum b/go.work.sum index 5152912..6a36f39 100644 --- a/go.work.sum +++ b/go.work.sum @@ -22,12 +22,16 @@ github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78 h1:ilQV1hzziu+LLM3zU github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78/go.mod h1:aL8wCCfTfSfmXjznFBSZNN13rSJjlIOI1fUNAtF7rmI= golang.org/x/mod v0.32.0 h1:9F4d3PHLljb6x//jOyokMv3eX+YDeepZSEo3mFJy93c= golang.org/x/mod v0.32.0/go.mod h1:SgipZ/3h2Ci89DlEtEXWUk/HteuRin+HHhN+WbNhguU= +golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/term v0.40.0 h1:36e4zGLqU4yhjlmxEaagx2KuYbJq3EwY8K943ZsHcvg= golang.org/x/term v0.40.0/go.mod h1:w2P8uVp06p2iyKKuvXIm7N/y0UCRt3UfJTfZ7oOpglM= +golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= golang.org/x/tools v0.41.0 h1:a9b8iMweWG+S0OBnlU36rzLp20z1Rp10w+IY2czHTQc= golang.org/x/tools v0.41.0/go.mod h1:XSY6eDqxVNiYgezAVqqCeihT4j1U2CCsqvH3WhQpnlg= +golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= rsc.io/pdf v0.1.1 h1:k1MczvYDUvJBe93bYd7wrZLLUEcLZAuF824/I4e5Xr4= diff --git a/go/exec/exec.go b/go/exec/exec.go index 5e30798..a668a7f 100644 --- a/go/exec/exec.go +++ b/go/exec/exec.go @@ -4,6 +4,7 @@ import ( "context" core "dappco.re/go" + "dappco.re/go/process/internal/lookpath" goio "io" ) @@ -298,7 +299,7 @@ func (c *Cmd) logError(msg string, failure core.Result) { // "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) + resolved := lookpath.Look(name) if !resolved.OK { return resolved } @@ -307,156 +308,3 @@ func commandContext(ctx context.Context, name string, arg ...string) core.Result Args: append([]string{name}, arg...), }) } - -// 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 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)) - } - - for _, dir := range core.Split(core.Getenv("PATH"), string(core.PathListSeparator)) { - if dir == "" { - dir = "." - } - 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 - } - info, ok := stat.Value.(core.FsFileInfo) - 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 41752e9..8f1f5a1 100644 --- a/go/exec/exec_internal_test.go +++ b/go/exec/exec_internal_test.go @@ -8,322 +8,11 @@ import ( core "dappco.re/go" ) -func TestExecInternal_lookPath_Good(t *testing.T) { - // An absolute path to an executable resolves to itself. - r := lookPath("/bin/sh") - if !r.OK { - t.Skip("/bin/sh not present on this platform") - } - if r.Value.(string) != "/bin/sh" { - t.Fatalf("expected /bin/sh, got %v", r.Value) - } - - // A bare name found on PATH resolves to an absolute path. - r = lookPath("sh") - if !r.OK { - t.Skip("sh not on PATH") - } - if !core.Contains(r.Value.(string), "sh") { - t.Fatalf("expected a path containing sh, got %v", r.Value) - } -} - -func TestExecInternal_lookPath_Bad(t *testing.T) { - // Empty file name is rejected. - if lookPath("").OK { - t.Fatal("expected empty file name to fail") - } - - // A path-qualified name that is not executable is rejected. - if lookPath("/no/such/binary/here").OK { - t.Fatal("expected missing path-qualified binary to fail") - } -} - -func TestExecInternal_lookPath_Ugly(t *testing.T) { - // A bare name that is not on PATH is rejected after the PATH search. - if lookPath("definitely_not_a_real_binary_xyz").OK { - t.Fatal("expected unknown binary to fail PATH search") - } - - // Empty PATH entries are treated as the current directory; with a name - // that cannot be found anywhere, the search still terminates in failure. - t.Setenv("PATH", "") - if lookPath("definitely_not_a_real_binary_xyz").OK { - t.Fatal("expected failure under empty PATH") - } -} - -func TestExecInternal_isExecutable_Good(t *testing.T) { - if !isExecutable("/bin/sh") { - t.Skip("/bin/sh not executable on this platform") - } -} - -func TestExecInternal_isExecutable_Bad(t *testing.T) { - // A non-existent path is not executable. - if isExecutable("/no/such/file") { - t.Fatal("expected non-existent path to be non-executable") - } -} - -func TestExecInternal_isExecutable_Ugly(t *testing.T) { - // A directory is not executable (even though it has the x bit). - if isExecutable("/tmp") { - t.Fatal("expected a directory to be reported non-executable") - } - - // A regular, non-executable file is not executable. - dir := t.TempDir() - path := core.PathJoin(dir, "plain.txt") - if w := core.WriteFile(path, []byte("data"), 0o600); !w.OK { - t.Fatalf("write failed: %v", w.Error()) - } - if isExecutable(path) { - t.Fatal("expected a 0600 file to be non-executable") - } -} - // 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{} diff --git a/go/internal/lookpath/lookpath.go b/go/internal/lookpath/lookpath.go new file mode 100644 index 0000000..b4507c0 --- /dev/null +++ b/go/internal/lookpath/lookpath.go @@ -0,0 +1,173 @@ +// Package lookpath resolves a command name to a runnable path. +// +// It exists because this module carried TWO copies of that logic — one in +// exec/, one behind Program.Find — and only the first was fixed when the +// Windows defects were found. go-inference calls the second, so its `git` +// resolution stayed broken through a release that claimed to fix it. One +// implementation, two consumers, is the fix for that class of miss as much as +// for the defects themselves. +// +// The Windows defects, for the record: +// - executability was tested as mode&0111, which Windows never sets — Stat +// synthesises 0666, or 0444 for a read-only file — so every file on the +// platform was rejected, git.exe included +// - %PATHEXT% was never expanded, and a command is written "git", not +// "git.exe" +// - the path-vs-name check matched only the platform separator, so a name +// spelled "bin/tool" was hunted on PATH instead of checked directly +package lookpath + +import core "dappco.re/go" + +// defaultPathExt is the extension list Windows itself assumes when %PATHEXT% +// is unset. +const defaultPathExt = ".COM;.EXE;.BAT;.CMD" + +// Look 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 Look(file string) core.Result { + return LookWith(file, Extensions()) +} + +// LookWith is Look 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 LookWith(file string, extensions []string) core.Result { + if file == "" { + return core.Fail(core.E("Look", "executable file not found in PATH", nil)) + } + if ContainsSeparator(file) { + if path, ok := firstExecutable(file, extensions); ok { + return core.Ok(path) + } + return core.Fail(core.E("Look", core.Sprintf("executable file %q not found", file), nil)) + } + + for _, dir := range core.Split(core.Getenv("PATH"), string(core.PathListSeparator)) { + if dir == "" { + dir = "." + } + if path, ok := firstExecutable(core.PathJoin(dir, file), extensions); ok { + return core.Ok(path) + } + } + return core.Fail(core.E("Look", 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 Candidates(base, extensions) { + if IsExecutableWith(candidate, extensions) { + return candidate, true + } + } + return "", false +} + +// Candidates 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 Candidates(base string, extensions []string) []string { + if len(extensions) == 0 || HasExtension(base, extensions) { + return []string{base} + } + candidates := make([]string, 0, len(extensions)) + for _, extension := range extensions { + candidates = append(candidates, base+extension) + } + return candidates +} + +// HasExtension reports whether base already ends in one of the +// listed extensions. Windows filenames are case-insensitive, so the comparison +// is too. +func HasExtension(base string, extensions []string) bool { + lowered := core.Lower(base) + for _, extension := range extensions { + if core.HasSuffix(lowered, extension) { + return true + } + } + return false +} + +// Extensions returns the %PATHEXT% list, or nil off Windows where a +// command name is used exactly as written. +func Extensions() []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, Extensions()) +} + +// 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 + } + info, ok := stat.Value.(core.FsFileInfo) + if !ok || info.IsDir() { + return false + } + if len(extensions) > 0 { + return HasExtension(path, extensions) + } + return info.Mode()&0111 != 0 +} diff --git a/go/internal/lookpath/lookpath_test.go b/go/internal/lookpath/lookpath_test.go new file mode 100644 index 0000000..ed92cd3 --- /dev/null +++ b/go/internal/lookpath/lookpath_test.go @@ -0,0 +1,320 @@ +// SPDX-Licence-Identifier: EUPL-1.2 + +package lookpath + +import ( + "testing" + + core "dappco.re/go" +) + +func TestExecInternal_lookPath_Good(t *testing.T) { + // An absolute path to an executable resolves to itself. + r := Look("/bin/sh") + if !r.OK { + t.Skip("/bin/sh not present on this platform") + } + if r.Value.(string) != "/bin/sh" { + t.Fatalf("expected /bin/sh, got %v", r.Value) + } + + // A bare name found on PATH resolves to an absolute path. + r = Look("sh") + if !r.OK { + t.Skip("sh not on PATH") + } + if !core.Contains(r.Value.(string), "sh") { + t.Fatalf("expected a path containing sh, got %v", r.Value) + } +} + +func TestExecInternal_lookPath_Bad(t *testing.T) { + // Empty file name is rejected. + if Look("").OK { + t.Fatal("expected empty file name to fail") + } + + // A path-qualified name that is not executable is rejected. + if Look("/no/such/binary/here").OK { + t.Fatal("expected missing path-qualified binary to fail") + } +} + +func TestExecInternal_lookPath_Ugly(t *testing.T) { + // A bare name that is not on PATH is rejected after the PATH search. + if Look("definitely_not_a_real_binary_xyz").OK { + t.Fatal("expected unknown binary to fail PATH search") + } + + // Empty PATH entries are treated as the current directory; with a name + // that cannot be found anywhere, the search still terminates in failure. + t.Setenv("PATH", "") + if Look("definitely_not_a_real_binary_xyz").OK { + t.Fatal("expected failure under empty PATH") + } +} + +func TestExecInternal_isExecutable_Good(t *testing.T) { + if !IsExecutable("/bin/sh") { + t.Skip("/bin/sh not executable on this platform") + } +} + +func TestExecInternal_isExecutable_Bad(t *testing.T) { + // A non-existent path is not executable. + if IsExecutable("/no/such/file") { + t.Fatal("expected non-existent path to be non-executable") + } +} + +func TestExecInternal_isExecutable_Ugly(t *testing.T) { + // A directory is not executable (even though it has the x bit). + if IsExecutable("/tmp") { + t.Fatal("expected a directory to be reported non-executable") + } + + // A regular, non-executable file is not executable. + dir := t.TempDir() + path := core.PathJoin(dir, "plain.txt") + if w := core.WriteFile(path, []byte("data"), 0o600); !w.OK { + t.Fatalf("write failed: %v", w.Error()) + } + if IsExecutable(path) { + t.Fatal("expected a 0600 file to be non-executable") + } +} + +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, + Candidates("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, Candidates("/usr/bin/git", nil), []string{"/usr/bin/git"}) + assertExtensions(t, Candidates("/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, + Candidates("C:/tools/git.exe", []string{".com", ".exe"}), + []string{"C:/tools/git.exe"}) + + // Matching is case-insensitive, as Windows filenames are. + assertExtensions(t, + Candidates("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, + Candidates("archive.tar", []string{".exe"}), + []string{"archive.tar.exe"}) +} + +func TestExecInternal_hasExecutableExtension_Good(t *testing.T) { + if !HasExtension("git.exe", []string{".com", ".exe"}) { + t.Fatal("expected git.exe to match .exe") + } + if !HasExtension("GIT.CMD", []string{".cmd"}) { + t.Fatal("expected case-insensitive matching") + } +} + +func TestExecInternal_hasExecutableExtension_Bad(t *testing.T) { + if HasExtension("git", []string{".com", ".exe"}) { + t.Fatal("expected a bare name to match nothing") + } + if HasExtension("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 := LookWith("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 := LookWith("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 := LookWith("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 := LookWith("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 = LookWith("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 = LookWith("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 = LookWith(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") + } +} + +// 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) + } + } +} + +// 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()) + } +} diff --git a/go/os_exec_link.go b/go/os_exec_link.go index 1923c41..3d48d82 100644 --- a/go/os_exec_link.go +++ b/go/os_exec_link.go @@ -4,52 +4,35 @@ import ( "context" core "dappco.re/go" + "dappco.re/go/process/internal/lookpath" ) +// commandContext resolves name to a concrete executable and builds the handle +// for it. +// +// A resolution failure is RECORDED ON THE HANDLE rather than discarded, exactly +// as exec.Command does with its own LookPath error: Cmd.Err is returned by +// Start without running anything. The old code dropped the failure and left +// Path as the bare name, which is not a harmless fallback on Windows — with +// Dir set, Cmd.Start resolves a separator-free Path relative to Dir, so an +// unresolved "git" surfaced as `exec: "C:\...\some work dir\git"`, naming a +// working directory nobody asked about instead of PATH. +// +// Deferring to Start rather than returning the error here keeps the callers' +// failure handling intact — Service.start still broadcasts its exited event, +// and Program.RunDir still wraps the error in its own operation. func commandContext(ctx context.Context, name string, arg ...string) *core.Cmd { - path := name - if result := lookPath(name); result.OK { - path = result.Value.(string) - } - - cmd := &core.Cmd{ - Path: path, - Args: append([]string{name}, arg...), - } - return cmd -} - -func lookPath(file 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) - } - return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found", file), nil)) - } - - for _, dir := range core.Split(core.Getenv("PATH"), string(core.PathListSeparator)) { - if dir == "" { - dir = "." + cmd := &core.Cmd{Args: append([]string{name}, arg...)} + resolved := lookpath.Look(name) + if !resolved.OK { + cmd.Path = name + if failure, ok := resolved.Value.(error); ok { + cmd.Err = failure + } else { + cmd.Err = core.E("commandContext", core.Sprintf("%q: not found in PATH", name), nil) } - path := core.PathJoin(dir, file) - if isExecutable(path) { - return core.Ok(path) - } - } - return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found in PATH", file), nil)) -} - -func isExecutable(path string) bool { - stat := core.Stat(path) - if !stat.OK { - return false + return cmd } - info, ok := stat.Value.(core.FsFileInfo) - if !ok || info.IsDir() { - return false - } - return info.Mode()&0111 != 0 + cmd.Path = resolved.Value.(string) + return cmd } diff --git a/go/program.go b/go/program.go index ed2c380..5bf8afb 100644 --- a/go/program.go +++ b/go/program.go @@ -5,6 +5,7 @@ import ( "unicode" core "dappco.re/go" + "dappco.re/go/process/internal/lookpath" ) // ErrProgramNotFound is returned when Find cannot locate the binary on PATH. @@ -47,7 +48,7 @@ func (p *Program) Find() core.Result { if target == "" { return core.Fail(core.E("Program.Find", "program name is empty", nil)) } - result := lookPath(target) + result := lookpath.Look(target) if !result.OK { return core.Fail(core.E("Program.Find", core.Sprintf("%q: not found in PATH", target), ErrProgramNotFound)) } diff --git a/go/program_test.go b/go/program_test.go index c19d512..15e54ad 100644 --- a/go/program_test.go +++ b/go/program_test.go @@ -7,6 +7,7 @@ import ( core "dappco.re/go" process "dappco.re/go/process" + "dappco.re/go/process/internal/lookpath" ) func testCtx(t *testing.T) context.Context { @@ -306,3 +307,37 @@ func TestProgram_Program_RunDir_Ugly(t *testing.T) { t.Fatalf("want hello, got %q", out) } } + +// TestProgram_Find_UsesSharedResolution is the regression pin for the miss that +// made this fix necessary. Program.Find and exec.Command once resolved through +// two byte-identical copies of the same logic, and when the Windows defects +// were fixed only the exec/ copy was touched — so a release that claimed to fix +// executable resolution left Program.Find, which go-inference calls, still +// broken. +// +// Asserting the two agree is what a version bump could not: it fails the moment +// a third copy appears or the two diverge again. +func TestProgram_Find_UsesSharedResolution(t *testing.T) { + program := &process.Program{Name: "definitely_not_a_real_binary_xyz"} + found := program.Find() + if found.OK { + t.Fatal("Find on an unresolvable name = OK, want failure") + } + if !core.Contains(found.Error(), "definitely_not_a_real_binary_xyz") { + t.Fatalf("failure must name the program, got %q", found.Error()) + } + + // A name that IS resolvable must land on the same absolute path the shared + // resolver reports — the two must not drift apart again. + shared := lookpath.Look("sh") + if !shared.OK { + t.Skip("sh not resolvable on this platform") + } + resolvable := &process.Program{Name: "sh"} + if r := resolvable.Find(); !r.OK { + t.Fatalf("Find(sh) = %s, want the shared resolver's answer", r.Error()) + } else if resolvable.Path != shared.Value.(string) { + t.Fatalf("Program.Find resolved %q but the shared resolver says %q — the copies have diverged", + resolvable.Path, shared.Value.(string)) + } +}