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..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,6 +61,14 @@ func forkProcess(log logr.Logger) func(cmd *cobra.Command, args []string) error logger.WithSessionId(childCmd) process.ForkFromParent(childCmd) + execShim, shimErr := useExecShim(childCmd) + if shimErr != nil { + return shimErr + } + if execShim != nil { + defer execShim.close() + } + monitorEnabled := cmd.Flags().Changed("monitor") var monitorCtx context.Context var monitorCtxCancel context.CancelFunc @@ -71,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 } @@ -115,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) { @@ -141,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) @@ -163,3 +188,97 @@ 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. +// +// 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, 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, nil + } + + dcpPath, dcpPathErr := os.Executable() + if dcpPathErr != nil { + 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 + + // 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 new file mode 100644 index 00000000..e835f596 --- /dev/null +++ b/internal/dcpproc/commands/fork_process_exec.go @@ -0,0 +1,109 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package commands + +import ( + "errors" + "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" + + // 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. '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 +) + +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:], + ) + + // '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() + + // 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), exitCode) + } +} diff --git a/internal/dcpproc/commands/fork_process_test.go b/internal/dcpproc/commands/fork_process_test.go new file mode 100644 index 00000000..05ad3b76 --- /dev/null +++ b/internal/dcpproc/commands/fork_process_test.go @@ -0,0 +1,116 @@ +/*--------------------------------------------------------------------------------------------- + * 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" + "os/exec" + "syscall" + "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 + + 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 + } + + 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") + + 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 +// 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 + + 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 +} 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() { +}