Skip to content

Commit

Permalink
interp: keep PATH list separators OS-specific
Browse files Browse the repository at this point in the history
A recent commit, 1eb295c, introduced a regression on Windows.
Programs spawned by interp no longer found other programs in PATH.

The reason turns out to be because those programs saw a Unix-like PATH,
with the list separator being ":" rather than ";",
which breaks Windows programs as they see one single mangled directory.

The actual source of the bug is much older, from eb75bb8f7d:

	For better compatibility with non-unix systems, convert $PATH to a
	unix path list when starting the interpreter. This should work with
	Windows, for example.

The recent commit surfaced the bug by tweaking the environment logic.
Before, converting PATH's list separators was an internal detail,
but now we also export this change and it affects spawned programs.

The added test case fails without the fix:

	--- FAIL: TestRunnerRun/mvdan#883 (0.19s)
		interp_test.go:3201: wrong output in "GOSH_CMD=lookpath $GOSH_PROG":
			want: "cmd found\n"
			got:  "exec: \"cmd\": executable file not found in %PATH%\nexit status 1"

The fix is relatively simple: stop converting PATH's list separators.
I believe the reason I originally added that code was for compatibility,
so that scripts using Unix-like path list separator, such as

	export PATH="${PATH}:${PWD}"

would work as expected on Windows.
However, I struggle to agree with my past self on that approach.
We had no tests for this behavior,
and any script using Unix-like paths in any non-trivial way
would likely need to be adjusted to be compatible with Windows anyway.

Fixes mvdan#768.
  • Loading branch information
mvdan committed Dec 2, 2021
1 parent b0d5d5c commit e482fb8
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 45 deletions.
9 changes: 0 additions & 9 deletions interp/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ import (
"math/rand"
"os"
"path/filepath"
"runtime"
"strconv"
"strings"
"sync"
"time"

Expand Down Expand Up @@ -456,13 +454,6 @@ func (r *Runner) Reset() {
r.setVarString("IFS", " \t\n")
r.setVarString("OPTIND", "1")

if runtime.GOOS == "windows" {
// convert $PATH to a unix path list
path := r.writeEnv.Get("PATH").String()
path = strings.Join(filepath.SplitList(path), ":")
r.setVarString("PATH", path)
}

r.dirStack = append(r.dirStack, r.Dir)
r.didReset = true
}
Expand Down
40 changes: 4 additions & 36 deletions interp/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,41 +183,6 @@ func findFile(dir, file string, _ []string) (string, error) {
return checkStat(dir, file, false)
}

func driveLetter(c byte) bool {
return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')
}

// splitList is like filepath.SplitList, but always using the unix path
// list separator ':'. On Windows, it also makes sure not to split
// [A-Z]:[/\].
func splitList(path string) []string {
if path == "" {
return []string{""}
}
list := strings.Split(path, ":")
if runtime.GOOS != "windows" {
return list
}
// join "C", "/foo" into "C:/foo"
var fixed []string
for i := 0; i < len(list); i++ {
s := list[i]
switch {
case len(s) != 1, !driveLetter(s[0]):
case i+1 >= len(list):
// last element
case strings.IndexAny(list[i+1], `/\`) != 0:
// next element doesn't start with / or \
default:
fixed = append(fixed, s+":"+list[i+1])
i++
continue
}
fixed = append(fixed, s)
}
return fixed
}

// LookPath is deprecated. See LookPathDir.
func LookPath(env expand.Environ, file string) (string, error) {
return LookPathDir(env.Get("PWD").String(), env, file)
Expand All @@ -240,7 +205,10 @@ func lookPathDir(cwd string, env expand.Environ, file string, find findAny) (str
panic("no find function found")
}

pathList := splitList(env.Get("PATH").String())
pathList := filepath.SplitList(env.Get("PATH").String())
if len(pathList) == 0 {
pathList = []string{""}
}
chars := `/`
if runtime.GOOS == "windows" {
chars = `:\/`
Expand Down
16 changes: 16 additions & 0 deletions interp/interp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ func TestMain(m *testing.M) {
case "foo_null_bar":
fmt.Println("foo\x00bar")
os.Exit(1)
case "lookpath":
_, err := exec.LookPath(pathProg)
if err != nil {
fmt.Println(err)
os.Exit(1)
}
fmt.Printf("%s found\n", pathProg)
os.Exit(0)
}
r := strings.NewReader(os.Args[1])
file, err := syntax.NewParser().Parse(r, "")
Expand Down Expand Up @@ -2891,6 +2899,10 @@ var runTestsUnix = []runTest{
"mkdir c; echo '#!/bin/sh\necho b' >c/a; chmod 0755 c/a; c/a",
"b\n",
},
{
"GOSH_CMD=lookpath $GOSH_PROG",
"sh found\n",
},

// error strings which are too different on Windows
{
Expand Down Expand Up @@ -3107,6 +3119,10 @@ var runTestsWindows = []runTest{
{"[[ -n $PPID || $PPID -gt 0 ]]", ""}, // os.Getppid can be 0 on windows
{"cmd() { :; }; cmd /c 'echo foo'", ""},
{"cmd() { :; }; command cmd /c 'echo foo'", "foo\r\n"},
{
"GOSH_CMD=lookpath $GOSH_PROG",
"cmd found\n",
},
}

// These tests are specific to 64-bit architectures, and that's fine. We don't
Expand Down

0 comments on commit e482fb8

Please sign in to comment.