From 0de32146db59c321c250d5b94f155243ed54c36e Mon Sep 17 00:00:00 2001 From: David Negstad Date: Wed, 19 Aug 2026 16:04:14 -0700 Subject: [PATCH 1/2] fix: reset inherited signal dispositions for forked processes on macOS On macOS, processes started by 'dcp fork-process' inherited the Go runtime's signal handler flags (SA_SIGINFO|SA_ONSTACK|SA_RESTART). Other language runtimes do not expect to start with those flags set, and NativeAOT .NET binaries crash with SIGSEGV the first time they take a GC suspension via SIGUSR1. Clearing the dispositions in 'fork-process' before starting the child does not help, because the Go runtime restores its own dispositions in the forked child before it reaches execve. The reset has to happen in the process that calls exec, so 'fork-process' now routes the child through a hidden 'fork-process-exec' command that clears the dispositions and then execs the requested program. Since execve keeps the process ID, the reported PID, session, standard streams, and exit code all stay the same. The reset is a no-op on other platforms, where children are started directly as before. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fcd61592-fa3e-49ab-8efc-d21136bb147d --- internal/dcp/commands/root.go | 6 ++ internal/dcpproc/commands/fork_process.go | 34 +++++++ .../dcpproc/commands/fork_process_exec.go | 86 +++++++++++++++++ .../dcpproc/commands/fork_process_test.go | 64 +++++++++++++ internal/dcpproc/commands/root.go | 6 ++ pkg/process/signal_disposition_darwin.go | 93 +++++++++++++++++++ pkg/process/signal_disposition_darwin_test.go | 62 +++++++++++++ pkg/process/signal_disposition_other.go | 26 ++++++ 8 files changed, 377 insertions(+) create mode 100644 internal/dcpproc/commands/fork_process_exec.go create mode 100644 internal/dcpproc/commands/fork_process_test.go create mode 100644 pkg/process/signal_disposition_darwin.go create mode 100644 pkg/process/signal_disposition_darwin_test.go create mode 100644 pkg/process/signal_disposition_other.go diff --git a/internal/dcp/commands/root.go b/internal/dcp/commands/root.go index 8c4bd106..de9944f3 100644 --- a/internal/dcp/commands/root.go +++ b/internal/dcp/commands/root.go @@ -93,6 +93,12 @@ func NewRootCmd(log *logger.Logger) (*cobra.Command, error) { rootCmd.AddCommand(cmd) } + if cmd, err = dcpproc_cmds.NewForkProcessExecCommand(log.Logger); err != nil { + return nil, fmt.Errorf("could not set up '%s' command: %w", dcpproc_cmds.ForkProcessExecCmdName, err) + } else { + rootCmd.AddCommand(cmd) + } + // Add dcptun sub-commands rootCmd.AddCommand(dcptun_cmds.NewRunServerCommand(log.Logger)) diff --git a/internal/dcpproc/commands/fork_process.go b/internal/dcpproc/commands/fork_process.go index 4d0bb725..d1b08fea 100644 --- a/internal/dcpproc/commands/fork_process.go +++ b/internal/dcpproc/commands/fork_process.go @@ -57,6 +57,10 @@ func forkProcess(log logr.Logger) func(cmd *cobra.Command, args []string) error logger.WithSessionId(childCmd) process.ForkFromParent(childCmd) + if wrapErr := useExecShim(childCmd); wrapErr != nil { + return wrapErr + } + monitorEnabled := cmd.Flags().Changed("monitor") var monitorCtx context.Context var monitorCtxCancel context.CancelFunc @@ -163,3 +167,33 @@ func trimForkProcessArgSeparator(args []string) []string { return args } + +// Redirects the child through the 'fork-process-exec' command on platforms where the child would +// otherwise inherit the Go runtime's signal handler flags. The shim clears those flags and then +// execs the original program, which keeps the process ID, session, standard streams, and exit +// code that the caller of 'fork-process' expects. +// +// The reset cannot be done here: the Go runtime restores its own signal dispositions in the +// forked child before it reaches execve, so it has to happen in the process that calls exec. +func useExecShim(childCmd *exec.Cmd) error { + if !process.SignalDispositionsLeakToChildren() { + return nil + } + + if childCmd.Err != nil { + // The program could not be located. Leave the command untouched so that starting it + // reports that original failure rather than one from the shim. + return nil + } + + dcpPath, dcpPathErr := os.Executable() + if dcpPathErr != nil { + return fmt.Errorf("could not determine the path of the current executable: %w", dcpPathErr) + } + + shimArgs := []string{dcpPath, ForkProcessExecCmdName, "--" + execPathFlagName, childCmd.Path, "--"} + childCmd.Args = append(shimArgs, childCmd.Args...) + childCmd.Path = dcpPath + + return nil +} diff --git a/internal/dcpproc/commands/fork_process_exec.go b/internal/dcpproc/commands/fork_process_exec.go new file mode 100644 index 00000000..63a1905e --- /dev/null +++ b/internal/dcpproc/commands/fork_process_exec.go @@ -0,0 +1,86 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package commands + +import ( + "fmt" + "os" + "syscall" + + "github.com/go-logr/logr" + "github.com/spf13/cobra" + + cmds "github.com/microsoft/dcp/internal/commands" + "github.com/microsoft/dcp/pkg/process" +) + +const ( + // The name of the command, also used when 'fork-process' builds an invocation of it. + ForkProcessExecCmdName = "fork-process-exec" + + // The flag carrying the resolved path of the image to execute. It is passed separately from + // the arguments so that the child keeps the argv[0] the caller asked for. + execPathFlagName = "exec-path" + + // Reported when the image cannot be executed, matching the shell convention for a command + // that could not be run. + execFailedExitCode = 127 +) + +var execPath string + +// NewForkProcessExecCommand creates the 'fork-process-exec' command, which replaces itself with +// the requested image after clearing the signal dispositions inherited from the Go runtime. +// It is an implementation detail of 'fork-process' and is not meant to be invoked directly. +func NewForkProcessExecCommand(log logr.Logger) (*cobra.Command, error) { + forkProcessExecCmd := &cobra.Command{ + Use: ForkProcessExecCmdName + " --" + execPathFlagName + " path -- command [args...]", + Short: "Replaces this process with another program.", + Long: "Clears the signal dispositions this process inherited from the Go runtime and then replaces it with the requested program, keeping the same process ID. Used internally by 'fork-process' so that children do not inherit signal handler flags that confuse other language runtimes.", + RunE: forkProcessExec(log), + Args: validateForkProcessExecArgs, + + Hidden: true, + SilenceUsage: true, + } + + forkProcessExecCmd.Flags().StringVar(&execPath, execPathFlagName, "", "Resolved path of the program to execute") + + return forkProcessExecCmd, nil +} + +func validateForkProcessExecArgs(_ *cobra.Command, args []string) error { + if len(trimForkProcessArgSeparator(args)) == 0 { + return fmt.Errorf("command is required") + } + + return nil +} + +func forkProcessExec(log logr.Logger) func(cmd *cobra.Command, args []string) error { + return func(_ *cobra.Command, args []string) error { + args = trimForkProcessArgSeparator(args) + + if execPath == "" { + return fmt.Errorf("--%s is required", execPathFlagName) + } + + log = log.WithName("ForkProcessExec").WithValues( + "Path", execPath, + "Args", args[1:], + ) + + // From this point on the process must not rely on the Go runtime's signal handling, + // which the reset disables. The only remaining step is the exec. + process.ResetSignalDispositions() + + // Exec only returns when it fails; on success this process becomes the requested program. + execErr := syscall.Exec(execPath, args, os.Environ()) + + log.Error(execErr, "Could not execute the requested program") + return cmds.NewExitCodeError(fmt.Errorf("could not execute %q: %w", execPath, execErr), execFailedExitCode) + } +} diff --git a/internal/dcpproc/commands/fork_process_test.go b/internal/dcpproc/commands/fork_process_test.go new file mode 100644 index 00000000..e041ce10 --- /dev/null +++ b/internal/dcpproc/commands/fork_process_test.go @@ -0,0 +1,64 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package commands + +import ( + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/microsoft/dcp/pkg/process" +) + +// Verifies that the child is redirected through the 'fork-process-exec' command on platforms +// that need it, and left alone everywhere else. The redirection is what clears the Go runtime's +// signal handler flags before the real program starts, so losing it reintroduces crashes in +// child runtimes that inspect those flags. +func TestUseExecShim(t *testing.T) { + t.Parallel() + + childCmd := exec.Command("sh", "-c", "exit 0") + originalPath := childCmd.Path + originalArgs := childCmd.Args + + require.NoError(t, useExecShim(childCmd)) + + if !process.SignalDispositionsLeakToChildren() { + require.Equal(t, originalPath, childCmd.Path, "the command should not be redirected on this platform") + require.Equal(t, originalArgs, childCmd.Args, "the arguments should not be rewritten on this platform") + return + } + + dcpPath, dcpPathErr := os.Executable() + require.NoError(t, dcpPathErr) + + expectedArgs := append( + []string{dcpPath, ForkProcessExecCmdName, "--" + execPathFlagName, originalPath, "--"}, + originalArgs..., + ) + + require.Equal(t, dcpPath, childCmd.Path, "the command should run the current executable") + require.Equal(t, expectedArgs, childCmd.Args, "the original program and arguments should be passed to the shim") +} + +// Verifies that a command that could not be resolved is left untouched, so that starting it +// reports the original lookup failure instead of one produced by the shim. +func TestUseExecShimLeavesUnresolvedCommand(t *testing.T) { + t.Parallel() + + childCmd := exec.Command("dcp-command-that-does-not-exist") + require.Error(t, childCmd.Err, "the test requires a command that cannot be resolved") + + originalPath := childCmd.Path + originalArgs := childCmd.Args + + require.NoError(t, useExecShim(childCmd)) + + require.Equal(t, originalPath, childCmd.Path, "an unresolved command should not be redirected") + require.Equal(t, originalArgs, childCmd.Args, "an unresolved command should not have its arguments rewritten") +} diff --git a/internal/dcpproc/commands/root.go b/internal/dcpproc/commands/root.go index e7254060..e9d6f84c 100644 --- a/internal/dcpproc/commands/root.go +++ b/internal/dcpproc/commands/root.go @@ -68,6 +68,12 @@ func NewRootCmd(log *logger.Logger) (*cobra.Command, error) { rootCmd.AddCommand(cmd) } + if cmd, err = NewForkProcessExecCommand(log.Logger); err != nil { + return nil, fmt.Errorf("could not set up '%s' command: %w", ForkProcessExecCmdName, err) + } else { + rootCmd.AddCommand(cmd) + } + return rootCmd, nil } diff --git a/pkg/process/signal_disposition_darwin.go b/pkg/process/signal_disposition_darwin.go new file mode 100644 index 00000000..a5df293a --- /dev/null +++ b/pkg/process/signal_disposition_darwin.go @@ -0,0 +1,93 @@ +//go:build darwin + +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package process + +import ( + "syscall" + "unsafe" +) + +// Darwin's NSIG. Valid signal numbers are 1 through darwinNumSignals-1. +const darwinNumSignals = 32 + +// SIG_DFL, which the syscall package does not define. +const darwinSigDfl = uintptr(0) + +// darwinSigactionNew mirrors Darwin's `struct __sigaction`, which is the layout the +// sigaction(2) system call expects for the new disposition. It differs from the userspace +// `struct sigaction` by the sa_tramp field that libc fills in with the signal trampoline. +// A nil trampoline is only safe when the handler is SIG_DFL or SIG_IGN, because the kernel +// stores the trampoline but never invokes it in those cases. +type darwinSigactionNew struct { + handler uintptr + tramp uintptr + mask uint32 + flags int32 +} + +// darwinSigactionOld mirrors Darwin's userspace `struct sigaction`, which is the layout the +// sigaction(2) system call uses when reporting the previous disposition. +type darwinSigactionOld struct { + handler uintptr + mask uint32 + flags int32 +} + +// SignalDispositionsLeakToChildren reports whether signal handler flags set by the Go runtime +// survive into an exec'd child on this platform, and therefore whether the child needs +// ResetSignalDispositions to be called on its behalf. +// +// Darwin's execve(2) resets signal handlers to SIG_DFL but preserves sa_flags. Linux clears +// sa_flags along with the handler, and Windows has no signal dispositions at all. +func SignalDispositionsLeakToChildren() bool { + return true +} + +// ResetSignalDispositions restores every catchable signal in the calling process to SIG_DFL +// with no flags and an empty mask. It must only be called by a process that is about to replace +// itself via exec, because it disables the Go runtime's own signal handling process-wide. +// +// The Go runtime installs a handler for nearly every signal at startup, and it always requests +// SA_SIGINFO|SA_ONSTACK|SA_RESTART, even where the disposition is SIG_DFL. Because Darwin's +// execve(2) preserves sa_flags, children inherit SIG_DFL together with SA_SIGINFO. Runtimes that +// read the existing disposition back before installing their own handler misinterpret that as a +// handler already being present: .NET, for example, re-registers a nil sa_sigaction and then +// jumps to address zero when it first uses SIGUSR1 to suspend threads for a garbage collection. +// +// Resetting in a process that then forks is not sufficient, because the Go runtime restores its +// own dispositions in the forked child before it reaches execve. The reset has to happen in the +// process that calls exec, which is what the 'fork-process-exec' command exists to do. +func ResetSignalDispositions() { + act := darwinSigactionNew{ + handler: darwinSigDfl, + tramp: 0, + mask: 0, + flags: 0, + } + + for sig := 1; sig < darwinNumSignals; sig++ { + if sig == int(syscall.SIGKILL) || sig == int(syscall.SIGSTOP) { + // sigaction(2) rejects these with EINVAL; their disposition can never change. + continue + } + + // Failures are deliberately ignored: there is no useful recovery, and one signal that + // cannot be reset must not prevent the child from being started. + _, _, _ = syscall.Syscall(syscall.SYS_SIGACTION, uintptr(sig), uintptr(unsafe.Pointer(&act)), 0) + } +} + +// signalDisposition reports the current handler and flags for a signal. +func signalDisposition(sig int) (darwinSigactionOld, error) { + var current darwinSigactionOld + if _, _, errno := syscall.Syscall(syscall.SYS_SIGACTION, uintptr(sig), 0, uintptr(unsafe.Pointer(¤t))); errno != 0 { + return current, errno + } + + return current, nil +} diff --git a/pkg/process/signal_disposition_darwin_test.go b/pkg/process/signal_disposition_darwin_test.go new file mode 100644 index 00000000..483c8157 --- /dev/null +++ b/pkg/process/signal_disposition_darwin_test.go @@ -0,0 +1,62 @@ +//go:build darwin + +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package process + +import ( + "os" + "os/exec" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/microsoft/dcp/pkg/testutil" +) + +// Marks the re-executed test binary as the helper that performs the reset. The reset disables +// the Go runtime's signal handling, so it cannot run in the main test process. +const resetSignalDispositionsHelperEnvVar = "DCP_TEST_RESET_SIGNAL_DISPOSITIONS_HELPER" + +func TestResetSignalDispositions(t *testing.T) { + t.Parallel() + + testCtx, testCancel := testutil.GetTestContext(t, 30*time.Second) + t.Cleanup(testCancel) + + helper := exec.CommandContext(testCtx, os.Args[0], "-test.run=TestResetSignalDispositionsHelper", "-test.v") + helper.Env = append(os.Environ(), resetSignalDispositionsHelperEnvVar+"=1") + + output, runErr := helper.CombinedOutput() + require.NoError(t, runErr, "helper process failed; output:\n%s", output) +} + +// Runs inside the process started by TestResetSignalDispositions and is skipped otherwise. +func TestResetSignalDispositionsHelper(t *testing.T) { + if os.Getenv(resetSignalDispositionsHelperEnvVar) != "1" { + t.Skip("helper for TestResetSignalDispositions") + } + + before, beforeErr := signalDisposition(int(syscall.SIGUSR1)) + require.NoError(t, beforeErr) + require.NotZero(t, before.flags, "the Go runtime should have left flags set on SIGUSR1") + + ResetSignalDispositions() + + for sig := 1; sig < darwinNumSignals; sig++ { + if sig == int(syscall.SIGKILL) || sig == int(syscall.SIGSTOP) { + continue + } + + after, afterErr := signalDisposition(sig) + require.NoError(t, afterErr, "signal %d disposition should be readable", sig) + require.Equal(t, darwinSigDfl, after.handler, "signal %d should be reset to SIG_DFL", sig) + require.Zero(t, after.flags, "signal %d should have no flags", sig) + require.Zero(t, after.mask, "signal %d should have an empty mask", sig) + } +} diff --git a/pkg/process/signal_disposition_other.go b/pkg/process/signal_disposition_other.go new file mode 100644 index 00000000..32014b8e --- /dev/null +++ b/pkg/process/signal_disposition_other.go @@ -0,0 +1,26 @@ +//go:build !darwin + +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package process + +// SignalDispositionsLeakToChildren reports whether signal handler flags set by the Go runtime +// survive into an exec'd child on this platform, and therefore whether the child needs +// ResetSignalDispositions to be called on its behalf. +// +// Only Darwin is affected: its execve(2) resets signal handlers to SIG_DFL but preserves +// sa_flags. Linux clears sa_flags along with the handler, and Windows has no signal dispositions +// at all. +func SignalDispositionsLeakToChildren() bool { + return false +} + +// ResetSignalDispositions restores every catchable signal in the calling process to its default +// disposition with no flags. It must only be called by a process that is about to replace itself +// via exec, because on platforms where it does something it disables the Go runtime's own signal +// handling process-wide. It is a no-op wherever SignalDispositionsLeakToChildren reports false. +func ResetSignalDispositions() { +} From e96d1c99774bee7a63df3c54fde12207a06232c3 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Wed, 19 Aug 2026 16:31:35 -0700 Subject: [PATCH 2/2] fix: report forked processes that could not be executed Routing the child through the exec shim made every start look successful, because the process being started became dcp itself, which always exists and is executable. A program that could not be run was reported as started and its PID was written to stdout, even though the process exited immediately. Commands are given absolute paths, which skip the PATH lookup that used to catch this before the child was started. The shim now reports the outcome of the exec on an inherited descriptor: a successful execve closes it, and a failure sends the errno first. 'fork-process' waits for that before reporting the PID, so a program that cannot be executed is once again a start failure carrying the original reason. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fcd61592-fa3e-49ab-8efc-d21136bb147d --- internal/dcpproc/commands/fork_process.go | 101 ++++++++++++++++-- .../dcpproc/commands/fork_process_exec.go | 27 ++++- .../dcpproc/commands/fork_process_test.go | 56 +++++++++- 3 files changed, 172 insertions(+), 12 deletions(-) diff --git a/internal/dcpproc/commands/fork_process.go b/internal/dcpproc/commands/fork_process.go index d1b08fea..912fa9ef 100644 --- a/internal/dcpproc/commands/fork_process.go +++ b/internal/dcpproc/commands/fork_process.go @@ -8,8 +8,12 @@ package commands import ( "context" "fmt" + "io" "os" "os/exec" + "strconv" + "strings" + "syscall" "github.com/go-logr/logr" "github.com/spf13/cobra" @@ -57,8 +61,12 @@ func forkProcess(log logr.Logger) func(cmd *cobra.Command, args []string) error logger.WithSessionId(childCmd) process.ForkFromParent(childCmd) - if wrapErr := useExecShim(childCmd); wrapErr != nil { - return wrapErr + execShim, shimErr := useExecShim(childCmd) + if shimErr != nil { + return shimErr + } + if execShim != nil { + defer execShim.close() } monitorEnabled := cmd.Flags().Changed("monitor") @@ -75,7 +83,7 @@ func forkProcess(log logr.Logger) func(cmd *cobra.Command, args []string) error } } - pid, childExitInfoCh, disposeChildExecutor, startErr := startForkedProcess(cmd, childCmd, monitorEnabled, log) + pid, childExitInfoCh, disposeChildExecutor, startErr := startForkedProcess(cmd, childCmd, execShim, monitorEnabled, log) if startErr != nil { return startErr } @@ -119,6 +127,7 @@ func forkProcess(log logr.Logger) func(cmd *cobra.Command, args []string) error func startForkedProcess( cmd *cobra.Command, childCmd *exec.Cmd, + execShim *execShimHandshake, observeExit bool, log logr.Logger, ) (process.Pid_t, <-chan process.ProcessExitInfo, func(), error) { @@ -145,6 +154,18 @@ func startForkedProcess( return process.UnknownPID, nil, nil, fmt.Errorf("could not start forked process: %w", startErr) } + // Starting the shim only means dcp itself started. The PID must not be reported before the + // requested program is known to be running, so that a program which cannot be executed is + // still reported as a start failure. + if execShim != nil { + if execErr := execShim.wait(); execErr != nil { + // The logger already carries the command and arguments. + log.Error(execErr, "Failed to execute forked process") + executor.Dispose() + return process.UnknownPID, nil, nil, fmt.Errorf("could not start forked process: %w", execErr) + } + } + pid := handle.Pid if _, writeErr := fmt.Fprintln(cmd.OutOrStdout(), pid); writeErr != nil { log.Error(writeErr, "Failed to write forked process PID", "PID", pid) @@ -175,25 +196,89 @@ func trimForkProcessArgSeparator(args []string) []string { // // The reset cannot be done here: the Go runtime restores its own signal dispositions in the // forked child before it reaches execve, so it has to happen in the process that calls exec. -func useExecShim(childCmd *exec.Cmd) error { +// +// Returns the handshake that reports whether the shim reached the requested program, or nil when +// the child is started directly. The caller owns the returned handshake and must close it. +func useExecShim(childCmd *exec.Cmd) (*execShimHandshake, error) { if !process.SignalDispositionsLeakToChildren() { - return nil + return nil, nil } if childCmd.Err != nil { // The program could not be located. Leave the command untouched so that starting it // reports that original failure rather than one from the shim. - return nil + return nil, nil } dcpPath, dcpPathErr := os.Executable() if dcpPathErr != nil { - return fmt.Errorf("could not determine the path of the current executable: %w", dcpPathErr) + return nil, fmt.Errorf("could not determine the path of the current executable: %w", dcpPathErr) + } + + statusR, statusW, pipeErr := os.Pipe() + if pipeErr != nil { + return nil, fmt.Errorf("could not create the exec status pipe: %w", pipeErr) } shimArgs := []string{dcpPath, ForkProcessExecCmdName, "--" + execPathFlagName, childCmd.Path, "--"} childCmd.Args = append(shimArgs, childCmd.Args...) childCmd.Path = dcpPath - return nil + // The shim reports the outcome of the exec on this descriptor. It is the only extra file, so + // the shim sees it as execStatusFd. + childCmd.ExtraFiles = append(childCmd.ExtraFiles, statusW) + + return &execShimHandshake{statusR: statusR, statusW: statusW}, nil +} + +// execShimHandshake reports whether the shim managed to exec the requested program. Starting the +// shim only proves that dcp itself could be started, so without this the caller would be told +// that a program which never ran had started successfully. +// +// The shim inherits the write end. A successful execve closes it and the read end reports EOF, +// while a failure sends the errno before the shim exits. +type execShimHandshake struct { + statusR *os.File + statusW *os.File +} + +// wait blocks until the shim either replaces itself with the requested program or reports why it +// could not. It returns the failure that a direct start would have reported. +func (h *execShimHandshake) wait() error { + // The write end is now owned by the shim. The parent's copy has to go, because the read below + // only reports EOF once every writer is closed. + h.closeWriteEnd() + + status, readErr := io.ReadAll(h.statusR) + if readErr != nil { + return fmt.Errorf("could not read the exec status: %w", readErr) + } + + if len(status) == 0 { + // EOF with nothing written: the descriptor was closed by a successful execve. + return nil + } + + errnoValue, parseErr := strconv.Atoi(strings.TrimSpace(string(status))) + if parseErr != nil { + return fmt.Errorf("the exec status %q could not be parsed: %w", status, parseErr) + } + + return syscall.Errno(errnoValue) +} + +func (h *execShimHandshake) closeWriteEnd() { + if h.statusW != nil { + _ = h.statusW.Close() + h.statusW = nil + } +} + +func (h *execShimHandshake) close() { + h.closeWriteEnd() + + if h.statusR != nil { + _ = h.statusR.Close() + h.statusR = nil + } } diff --git a/internal/dcpproc/commands/fork_process_exec.go b/internal/dcpproc/commands/fork_process_exec.go index 63a1905e..e835f596 100644 --- a/internal/dcpproc/commands/fork_process_exec.go +++ b/internal/dcpproc/commands/fork_process_exec.go @@ -6,6 +6,7 @@ package commands import ( + "errors" "fmt" "os" "syscall" @@ -25,8 +26,13 @@ const ( // the arguments so that the child keeps the argv[0] the caller asked for. execPathFlagName = "exec-path" + // The descriptor 'fork-process' passes as the only extra file, on which this command reports + // whether the exec succeeded. It is the first descriptor after the standard streams. + execStatusFd = 3 + // Reported when the image cannot be executed, matching the shell convention for a command - // that could not be run. + // that could not be run. 'fork-process' reports the underlying errno itself, so this is only + // a fallback for anything that inspects the shim's own exit code. execFailedExitCode = 127 ) @@ -73,6 +79,12 @@ func forkProcessExec(log logr.Logger) func(cmd *cobra.Command, args []string) er "Args", args[1:], ) + // 'fork-process' waits for this descriptor to close, which is how a successful execve is + // reported, so it must not survive into the new program. It is always supplied, because + // this command is only ever started by 'fork-process'. + statusFile := os.NewFile(execStatusFd, "exec-status") + syscall.CloseOnExec(execStatusFd) + // From this point on the process must not rely on the Go runtime's signal handling, // which the reset disables. The only remaining step is the exec. process.ResetSignalDispositions() @@ -80,7 +92,18 @@ func forkProcessExec(log logr.Logger) func(cmd *cobra.Command, args []string) er // Exec only returns when it fails; on success this process becomes the requested program. execErr := syscall.Exec(execPath, args, os.Environ()) + var execErrno syscall.Errno + if !errors.As(execErr, &execErrno) { + // Report something the parent can still parse; the message below stays accurate. + execErrno = syscall.EINVAL + } + + _, _ = fmt.Fprintf(statusFile, "%d", int(execErrno)) + _ = statusFile.Close() + + exitCode := execFailedExitCode + log.Error(execErr, "Could not execute the requested program") - return cmds.NewExitCodeError(fmt.Errorf("could not execute %q: %w", execPath, execErr), execFailedExitCode) + return cmds.NewExitCodeError(fmt.Errorf("could not execute %q: %w", execPath, execErr), exitCode) } } diff --git a/internal/dcpproc/commands/fork_process_test.go b/internal/dcpproc/commands/fork_process_test.go index e041ce10..05ad3b76 100644 --- a/internal/dcpproc/commands/fork_process_test.go +++ b/internal/dcpproc/commands/fork_process_test.go @@ -6,8 +6,10 @@ package commands import ( + "fmt" "os" "os/exec" + "syscall" "testing" "github.com/stretchr/testify/require" @@ -26,9 +28,14 @@ func TestUseExecShim(t *testing.T) { originalPath := childCmd.Path originalArgs := childCmd.Args - require.NoError(t, useExecShim(childCmd)) + execShim, shimErr := useExecShim(childCmd) + require.NoError(t, shimErr) + if execShim != nil { + defer execShim.close() + } if !process.SignalDispositionsLeakToChildren() { + require.Nil(t, execShim, "no handshake is needed on this platform") require.Equal(t, originalPath, childCmd.Path, "the command should not be redirected on this platform") require.Equal(t, originalArgs, childCmd.Args, "the arguments should not be rewritten on this platform") return @@ -44,6 +51,9 @@ func TestUseExecShim(t *testing.T) { require.Equal(t, dcpPath, childCmd.Path, "the command should run the current executable") require.Equal(t, expectedArgs, childCmd.Args, "the original program and arguments should be passed to the shim") + + require.NotNil(t, execShim, "the shim should report whether the exec succeeded") + require.Len(t, childCmd.ExtraFiles, 1, "the status descriptor should be passed to the shim") } // Verifies that a command that could not be resolved is left untouched, so that starting it @@ -57,8 +67,50 @@ func TestUseExecShimLeavesUnresolvedCommand(t *testing.T) { originalPath := childCmd.Path originalArgs := childCmd.Args - require.NoError(t, useExecShim(childCmd)) + execShim, shimErr := useExecShim(childCmd) + require.NoError(t, shimErr) + require.Nil(t, execShim, "an unresolved command should not be redirected through the shim") require.Equal(t, originalPath, childCmd.Path, "an unresolved command should not be redirected") require.Equal(t, originalArgs, childCmd.Args, "an unresolved command should not have its arguments rewritten") } + +// Verifies that the handshake reports a successful exec, which the shim signals by closing the +// status descriptor without writing to it. +func TestExecShimHandshakeReportsSuccess(t *testing.T) { + t.Parallel() + + handshake := newTestExecShimHandshake(t) + + // Stand in for the shim: a successful execve closes the inherited descriptor. + require.NoError(t, handshake.statusW.Close()) + + require.NoError(t, handshake.wait()) +} + +// Verifies that the errno the shim reports is surfaced to the caller. Without this the caller +// would be handed the PID of a process that never became the requested program. +func TestExecShimHandshakeReportsExecFailure(t *testing.T) { + t.Parallel() + + handshake := newTestExecShimHandshake(t) + + // Stand in for the shim reporting a failed execve. + _, writeErr := fmt.Fprintf(handshake.statusW, "%d", int(syscall.ENOENT)) + require.NoError(t, writeErr) + require.NoError(t, handshake.statusW.Close()) + + require.ErrorIs(t, handshake.wait(), syscall.ENOENT) +} + +func newTestExecShimHandshake(t *testing.T) *execShimHandshake { + t.Helper() + + statusR, statusW, pipeErr := os.Pipe() + require.NoError(t, pipeErr) + + handshake := &execShimHandshake{statusR: statusR, statusW: statusW} + t.Cleanup(handshake.close) + + return handshake +}