Refine 1524fef

It turns out that the previous change breaks shell scripts that assume
$0 is relative. This is because scripts that use '#!' don't receive
argv[0]. Instead, they will receive the exact path that is passed to
execve().

Change our logic to do a better job at keeping the executable path
intact. In case argv[0] contains slashes, we can use it as the
executable path directly. If we do need to search PATH, then compute the
executable path twice:

- Once absolute, to be passed to exec.LookPath().
- Once potentially relative, to be passed to execve().

This makes our behaviour more consistent with POSIX shells.

pkg/runner/local_runner_test.go

@@ -297,11 +297,13 @@ func TestLocalRunner(t *testing.T) {
 		require.NoError(t, os.Mkdir(testPath, 0o777))
 		require.NoError(t, os.MkdirAll(filepath.Join(testPath, "root", "subdirectory"), 0o777))
 		require.NoError(t, os.Mkdir(filepath.Join(testPath, "tmp"), 0o777))
-		require.NoError(t, os.WriteFile(filepath.Join(testPath, "root", "subdirectory", "hello.sh"), []byte("#!/bin/sh\nexit 42\n"), 0o777))
+		require.NoError(t, os.WriteFile(filepath.Join(testPath, "root", "subdirectory", "hello.sh"), []byte("#!/bin/sh\necho $0\nexit 42\n"), 0o777))
 
 		// If the PATH environment variable contains a relative
 		// path, it should be treated as being relative to the
-		// working directory.
+		// working directory. Because the search path is
+		// relative, execve() should be called with a relative
+		// path as well.
 		runner := runner.NewLocalRunner(buildDirectory, buildDirectoryPathBuilder, runner.NewPlainCommandCreator(&syscall.SysProcAttr{}), false)
 		response, err := runner.Run(context.Background(), &runner_pb.RunRequest{
 			Arguments:            []string{"hello.sh"},
@@ -316,7 +318,7 @@ func TestLocalRunner(t *testing.T) {
 
 		stdout, err := os.ReadFile(filepath.Join(testPath, "stdout"))
 		require.NoError(t, err)
-		require.Empty(t, stdout)
+		require.Equal(t, "subdirectory/hello.sh\n", string(stdout))
 
 		stderr, err := os.ReadFile(filepath.Join(testPath, "stderr"))
 		require.NoError(t, err)

pkg/runner/local_runner_unix.go

@@ -21,18 +21,26 @@ import (
 	"google.golang.org/protobuf/types/known/durationpb"
 )
 
+// getExecutablePath returns the path of an executable within a given
+// search path that is part of the PATH environment variable.
+func getExecutablePath(baseDirectory *path.Builder, searchPathStr, argv0 string) (string, error) {
+	searchPath, scopeWalker := baseDirectory.Join(path.VoidScopeWalker)
+	if err := path.Resolve(searchPathStr, scopeWalker); err != nil {
+		return "", err
+	}
+	executablePath, scopeWalker := searchPath.Join(path.VoidScopeWalker)
+	if err := path.Resolve(argv0, scopeWalker); err != nil {
+		return "", err
+	}
+	return executablePath.String(), nil
+}
+
 // lookupExecutable returns the path of an executable, taking the PATH
 // environment variable into account.
-func lookupExecutable(argv0 string, workingDirectory *path.Builder, pathVariable string) (*path.Builder, error) {
+func lookupExecutable(workingDirectory *path.Builder, pathVariable, argv0 string) (string, error) {
 	if strings.ContainsRune(argv0, os.PathSeparator) {
-		// The path contains one or more slashes. Resolve it to
-		// a location relative to the action's working
-		// directory.
-		executablePath, scopeWalker := workingDirectory.Join(path.VoidScopeWalker)
-		if err := path.Resolve(argv0, scopeWalker); err != nil {
-			return nil, util.StatusWrap(err, "Failed to resolve executable path")
-		}
-		return executablePath, nil
+		// No PATH processing needs to be performed.
+		return argv0, nil
 	}
 
 	// Executable path does not contain any slashes. Perform PATH
@@ -45,19 +53,29 @@ func lookupExecutable(argv0 string, workingDirectory *path.Builder, pathVariable
 	// the action. Do call into this function to validate the
 	// existence of the executable.
 	for _, searchPathStr := range filepath.SplitList(pathVariable) {
-		searchPath, scopeWalker := workingDirectory.Join(path.VoidScopeWalker)
-		if err := path.Resolve(searchPathStr, scopeWalker); err != nil {
-			return nil, util.StatusWrapf(err, "Failed to resolve executable search path %#v", searchPathStr)
-		}
-		executablePath, scopeWalker := searchPath.Join(path.VoidScopeWalker)
-		if err := path.Resolve(argv0, scopeWalker); err != nil {
-			return nil, util.StatusWrap(err, "Failed to resolve executable path")
+		executablePathAbs, err := getExecutablePath(workingDirectory, searchPathStr, argv0)
+		if err != nil {
+			return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
 		}
-		if _, err := exec.LookPath(executablePath.String()); err == nil {
-			return executablePath, nil
+		if _, err := exec.LookPath(executablePathAbs); err == nil {
+			// Regular compiled executables will receive the
+			// argv[0] that we provide, but scripts starting
+			// with '#!' will receive the literal executable
+			// path.
+			//
+			// Most shells seem to guarantee that if argv[0]
+			// is relative, the executable path is relative
+			// as well. Prevent these scripts from breaking
+			// by recomputing the executable path once more,
+			// but relative.
+			executablePathRel, err := getExecutablePath(&path.EmptyBuilder, searchPathStr, argv0)
+			if err != nil {
+				return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
+			}
+			return executablePathRel, nil
 		}
 	}
-	return nil, status.Errorf(codes.InvalidArgument, "Cannot find executable %#v in search paths %#v", argv0, pathVariable)
+	return "", status.Errorf(codes.InvalidArgument, "Cannot find executable %#v in search paths %#v", argv0, pathVariable)
 }
 
 // NewPlainCommandCreator returns a CommandCreator for cases where we don't
@@ -68,20 +86,21 @@ func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator {
 		if err := path.Resolve(workingDirectoryStr, scopeWalker); err != nil {
 			return nil, util.StatusWrap(err, "Failed to resolve working directory")
 		}
-
-		// Call exec.CommandContext() with an absolute path, so
-		// that it does not try to do PATH lookups for us.
-		// Override the arguments afterwards, so that argv[0]
-		// remains equal to the originally provided value.
-		executablePath, err := lookupExecutable(arguments[0], workingDirectory, pathVariable)
+		executablePath, err := lookupExecutable(workingDirectory, pathVariable, arguments[0])
 		if err != nil {
 			return nil, err
 		}
-		cmd := exec.CommandContext(ctx, executablePath.String())
-		cmd.Args = arguments
-		cmd.SysProcAttr = sysProcAttr
 
+		// exec.CommandContext() has some smartness to call
+		// exec.LookPath() under the hood, which we don't want.
+		// Call it with a placeholder path, followed by setting
+		// cmd.Path and cmd.Args manually. This ensures that our
+		// own values remain respected.
+		cmd := exec.CommandContext(ctx, "/nonexistent")
+		cmd.Args = arguments
 		cmd.Dir = workingDirectory.String()
+		cmd.Path = executablePath
+		cmd.SysProcAttr = sysProcAttr
 		return cmd, nil
 	}
 }