diff --git a/cmd/bb_runner/main.go b/cmd/bb_runner/main.go index 79654bf2..6387f427 100644 --- a/cmd/bb_runner/main.go +++ b/cmd/bb_runner/main.go @@ -31,7 +31,7 @@ func main() { } var configuration bb_runner.ApplicationConfiguration if err := util.UnmarshalConfigurationFromFile(os.Args[1], &configuration); err != nil { - return util.StatusWrapf(err, "Failed to read configuration from %s", os.Args[1]) + return util.StatusWrapf(err, "Failed to read configuration from %#v", os.Args[1]) } lifecycleState, grpcClientFactory, err := global.ApplyConfiguration(configuration.Global) if err != nil { @@ -40,7 +40,7 @@ func main() { buildDirectoryPath, scopeWalker := path.EmptyBuilder.Join(path.NewAbsoluteScopeWalker(path.VoidComponentWalker)) if err := path.Resolve(path.NewLocalParser(configuration.BuildDirectoryPath), scopeWalker); err != nil { - return util.StatusWrap(err, "Failed to resolve build directory") + return util.StatusWrapf(err, "Failed to resolve build directory %#v", configuration.BuildDirectoryPath) } buildDirectory := re_filesystem.NewLazyDirectory( func() (filesystem.DirectoryCloser, error) { diff --git a/cmd/bb_worker/main.go b/cmd/bb_worker/main.go index d21a8915..5feccffe 100644 --- a/cmd/bb_worker/main.go +++ b/cmd/bb_worker/main.go @@ -260,10 +260,10 @@ func main() { // used. This increases cache hit rate. cacheDirectory, err := filesystem.NewLocalDirectory(path.NewLocalParser(nativeConfiguration.CacheDirectoryPath)) if err != nil { - return util.StatusWrap(err, "Failed to open cache directory") + return util.StatusWrapf(err, "Failed to open cache directory %#v", nativeConfiguration.CacheDirectoryPath) } if err := cacheDirectory.RemoveAllChildren(); err != nil { - return util.StatusWrap(err, "Failed to clear cache directory") + return util.StatusWrapf(err, "Failed to clear cache directory %#v", nativeConfiguration.CacheDirectoryPath) } evictionSet, err := eviction.NewSetFromConfiguration[string](nativeConfiguration.CacheReplacementPolicy) if err != nil { diff --git a/pkg/builder/build_client.go b/pkg/builder/build_client.go index 08dbe409..6d50a20f 100644 --- a/pkg/builder/build_client.go +++ b/pkg/builder/build_client.go @@ -176,7 +176,7 @@ func (bc *BuildClient) touchSchedulerMayThinkExecuting() { bc.schedulerMayThinkExecutingUntil = &until } -// Run a iteration of the Remote Worker client, by performing a single +// Run an iteration of the Remote Worker client, by performing a single // synchronization against the scheduler. func (bc *BuildClient) Run(ctx context.Context) (bool, error) { // Allow the worker to terminate if the scheduler doesn't think diff --git a/pkg/builder/clean_build_directory_creator.go b/pkg/builder/clean_build_directory_creator.go index 06c5e97e..15684bb5 100644 --- a/pkg/builder/clean_build_directory_creator.go +++ b/pkg/builder/clean_build_directory_creator.go @@ -15,7 +15,7 @@ type cleanBuildDirectoryCreator struct { } // NewCleanBuildDirectoryCreator is an adapter for BuildDirectoryCreator -// that upon acquistion and release calls into a Cleaner. This Cleaner +// that upon acquisition and release calls into a Cleaner. This Cleaner // may, for example, be set up to empty out the build directory. This // guarantees that build actions aren't able to see data left behind by // ones that ran previously. diff --git a/pkg/builder/local_build_executor.go b/pkg/builder/local_build_executor.go index 847dc21c..96785312 100644 --- a/pkg/builder/local_build_executor.go +++ b/pkg/builder/local_build_executor.go @@ -160,7 +160,14 @@ func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesyste util.StatusWrap(err, "Failed to acquire build environment")) return response } - defer buildDirectory.Close() + defer func() { + err := buildDirectory.Close() + if err != nil { + attachErrorToExecuteResponse( + response, + util.StatusWrap(err, "Failed to close build directory")) + } + }() // Install hooks on build directory to capture file creation and // I/O error events. diff --git a/pkg/runner/local_runner.go b/pkg/runner/local_runner.go index 07098ddb..13236347 100644 --- a/pkg/runner/local_runner.go +++ b/pkg/runner/local_runner.go @@ -3,7 +3,12 @@ package runner import ( "context" "errors" + "math" + "os" "os/exec" + "path/filepath" + "strings" + "syscall" "github.com/buildbarn/bb-remote-execution/pkg/proto/runner" "github.com/buildbarn/bb-storage/pkg/filesystem" @@ -88,6 +93,37 @@ func (r *localRunner) openLog(logPath string) (filesystem.FileAppender, error) { // on whether the action needs to be run in a chroot() or not. type CommandCreator func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) +// NewPlainCommandCreator returns a CommandCreator for cases where we don't +// need to chroot into the input root directory. +func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator { + return func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) { + workingDirectory, scopeWalker := inputRootDirectory.Join(path.VoidScopeWalker) + if err := path.Resolve(workingDirectoryParser, scopeWalker); err != nil { + return nil, util.StatusWrap(err, "Failed to resolve working directory") + } + workingDirectoryStr, err := path.GetLocalString(workingDirectory) + if err != nil { + return nil, util.StatusWrap(err, "Failed to create local representation of working directory") + } + executablePath, err := lookupExecutable(workingDirectory, pathVariable, arguments[0]) + if err != nil { + return nil, err + } + + // 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 = workingDirectoryStr + cmd.Path = executablePath + cmd.SysProcAttr = sysProcAttr + return cmd, nil + } +} + // NewLocalRunner returns a Runner capable of running commands on the // local system directly. func NewLocalRunner(buildDirectory filesystem.Directory, buildDirectoryPath *path.Builder, commandCreator CommandCreator, setTmpdirEnvironmentVariable bool) runner.RunnerServer { @@ -201,3 +237,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. +func lookupExecutable(workingDirectory *path.Builder, pathVariable, argv0 string) (string, error) { + if strings.ContainsFunc(argv0, func(r rune) bool { + return r <= math.MaxUint8 && os.IsPathSeparator(uint8(r)) + }) { + // 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..ab392e1f 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,100 +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 { - return func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) { - workingDirectory, scopeWalker := inputRootDirectory.Join(path.VoidScopeWalker) - if err := path.Resolve(workingDirectoryParser, scopeWalker); err != nil { - return nil, util.StatusWrap(err, "Failed to resolve working directory") - } - workingDirectoryStr, err := path.GetLocalString(workingDirectory) - if err != nil { - return nil, util.StatusWrap(err, "Failed to create local representation of working directory") - } - executablePath, err := lookupExecutable(workingDirectory, pathVariable, arguments[0]) - if err != nil { - return nil, err - } - - // 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 = workingDirectoryStr - cmd.Path = executablePath - cmd.SysProcAttr = sysProcAttr - return cmd, nil - } -} - // NewChrootedCommandCreator returns a CommandCreator for cases where we // need to chroot into the input root directory. func NewChrootedCommandCreator(sysProcAttr *syscall.SysProcAttr) (CommandCreator, error) { diff --git a/pkg/runner/local_runner_windows.go b/pkg/runner/local_runner_windows.go index 237d9452..29291357 100644 --- a/pkg/runner/local_runner_windows.go +++ b/pkg/runner/local_runner_windows.go @@ -4,14 +4,11 @@ package runner import ( - "context" "os" "os/exec" "syscall" "github.com/buildbarn/bb-remote-execution/pkg/proto/resourceusage" - "github.com/buildbarn/bb-storage/pkg/filesystem/path" - "github.com/buildbarn/bb-storage/pkg/util" "golang.org/x/sys/windows" "google.golang.org/grpc/codes" @@ -19,34 +16,10 @@ import ( "google.golang.org/protobuf/types/known/durationpb" ) -// NewPlainCommandCreator returns a CommandCreator for cases where we don't -// need to chroot into the input root directory. -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. - cmd := exec.CommandContext(ctx, arguments[0], arguments[1:]...) - cmd.SysProcAttr = sysProcAttr - - // Set the working relative to be relative to the input - // root directory. - workingDirectory, scopeWalker := inputRootDirectory.Join(path.VoidScopeWalker) - if err := path.Resolve(workingDirectoryParser, scopeWalker); err != nil { - return nil, util.StatusWrap(err, "Failed to resolve working directory") - } - workingDirectoryStr, err := path.GetLocalString(workingDirectory) - if err != nil { - return nil, util.StatusWrap(err, "Failed to create local representation of working directory") - } - cmd.Dir = workingDirectoryStr - return cmd, nil - } -} - // NewChrootedCommandCreator gives an error on Windows, as chroot is not // supported on the platform. func NewChrootedCommandCreator(sysProcAttr *syscall.SysProcAttr) (CommandCreator, error) { - return nil, status.Error(codes.InvalidArgument, "Chroot not supported on Windows") + return nil, status.Error(codes.InvalidArgument, "Chroot is not supported on Windows") } var temporaryDirectoryEnvironmentVariablePrefixes = [...]string{"TMP=", "TEMP="} @@ -54,6 +27,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()),