diff --git a/pkg/runner/local_runner.go b/pkg/runner/local_runner.go index 07098ddb..ee7a4aac 100644 --- a/pkg/runner/local_runner.go +++ b/pkg/runner/local_runner.go @@ -3,7 +3,10 @@ package runner import ( "context" "errors" + "os" "os/exec" + "path/filepath" + "strings" "github.com/buildbarn/bb-remote-execution/pkg/proto/runner" "github.com/buildbarn/bb-storage/pkg/filesystem" @@ -201,3 +204,63 @@ func (r *localRunner) CheckReadiness(ctx context.Context, request *runner.CheckR return &emptypb.Empty{}, nil } + +// 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(path.NewLocalParser(searchPathStr), scopeWalker); err != nil { + return "", err + } + + executablePath, scopeWalker := searchPath.Join(path.VoidScopeWalker) + if err := path.Resolve(path.NewLocalParser(argv0), scopeWalker); err != nil { + return "", err + } + return path.GetLocalString(executablePath) +} + +// lookupExecutable returns the path of an executable, taking the PATH +// environment variable into account. +// This operates on platform native paths, or unix-style slash paths. +// The latter are customarily sent by Bazel in multiplatform builds. +func lookupExecutable(workingDirectory *path.Builder, pathVariable, argv0 string) (string, error) { + if strings.ContainsRune(argv0, os.PathSeparator) { + // No PATH processing needs to be performed. + return argv0, nil + } + + // Executable path does not contain any slashes. Perform PATH + // lookups. + // + // We cannot use exec.LookPath() directly, as that function + // disregards the working directory of the action. It also uses + // the PATH environment variable of the current process, as + // opposed to respecting the value that is provided as part of + // the action. Do call into this function to validate the + // existence of the executable. + for _, searchPathStr := range filepath.SplitList(pathVariable) { + 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(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 "", status.Errorf(codes.InvalidArgument, "Cannot find executable %#v in search paths %#v", argv0, pathVariable) +} diff --git a/pkg/runner/local_runner_unix.go b/pkg/runner/local_runner_unix.go index b3241249..a995241d 100644 --- a/pkg/runner/local_runner_unix.go +++ b/pkg/runner/local_runner_unix.go @@ -7,7 +7,6 @@ import ( "context" "os" "os/exec" - "path/filepath" "strings" "syscall" @@ -16,69 +15,9 @@ import ( "github.com/buildbarn/bb-storage/pkg/util" "golang.org/x/sys/unix" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "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(path.NewLocalParser(searchPathStr), scopeWalker); err != nil { - return "", err - } - - executablePath, scopeWalker := searchPath.Join(path.VoidScopeWalker) - if err := path.Resolve(path.NewLocalParser(argv0), scopeWalker); err != nil { - return "", err - } - return path.GetLocalString(executablePath) -} - -// lookupExecutable returns the path of an executable, taking the PATH -// environment variable into account. -func lookupExecutable(workingDirectory *path.Builder, pathVariable, argv0 string) (string, error) { - if strings.ContainsRune(argv0, os.PathSeparator) { - // No PATH processing needs to be performed. - return argv0, nil - } - - // Executable path does not contain any slashes. Perform PATH - // lookups. - // - // We cannot use exec.LookPath() directly, as that function - // disregards the working directory of the action. It also uses - // the PATH environment variable of the current process, as - // opposed to respecting the value that is provided as part of - // the action. Do call into this function to validate the - // existence of the executable. - for _, searchPathStr := range filepath.SplitList(pathVariable) { - 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(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 "", status.Errorf(codes.InvalidArgument, "Cannot find executable %#v in search paths %#v", argv0, pathVariable) -} - // NewPlainCommandCreator returns a CommandCreator for cases where we don't // need to chroot into the input root directory. func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator { diff --git a/pkg/runner/local_runner_windows.go b/pkg/runner/local_runner_windows.go index 237d9452..b830f7cc 100644 --- a/pkg/runner/local_runner_windows.go +++ b/pkg/runner/local_runner_windows.go @@ -7,6 +7,7 @@ import ( "context" "os" "os/exec" + "path/filepath" "syscall" "github.com/buildbarn/bb-remote-execution/pkg/proto/resourceusage" @@ -25,6 +26,14 @@ func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator { return func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) { // TODO: This may not work correctly if the action sets // the PATH environment variable explicitly. + // + // 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. + argv0 := arguments[0] + arguments[0] = "/nonexistent-place-back-later" cmd := exec.CommandContext(ctx, arguments[0], arguments[1:]...) cmd.SysProcAttr = sysProcAttr @@ -39,6 +48,19 @@ func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator { return nil, util.StatusWrap(err, "Failed to create local representation of working directory") } cmd.Dir = workingDirectoryStr + + executablePath, err := lookupExecutable(workingDirectory, pathVariable, argv0) + if err != nil { + return nil, err + } + + // Must use backslashes to execute relative paths + executablePath = filepath.FromSlash(executablePath) + arguments[0] = executablePath + + cmd.Args = arguments + cmd.Path = executablePath + cmd.SysProcAttr = sysProcAttr return cmd, nil } } @@ -54,6 +76,7 @@ var temporaryDirectoryEnvironmentVariablePrefixes = [...]string{"TMP=", "TEMP="} var invalidArgumentErrs = [...]error{exec.ErrNotFound, os.ErrPermission, os.ErrNotExist, windows.ERROR_BAD_EXE_FORMAT} func getPOSIXResourceUsage(cmd *exec.Cmd) *resourceusage.POSIXResourceUsage { + // TODO: These do not work processState := cmd.ProcessState return &resourceusage.POSIXResourceUsage{ UserTime: durationpb.New(processState.SystemTime()),