git.iko.soy/crush
on main help sign in
fix(shell): isolate child processes from Crush's session (#3097)
d3af321Kieran Klukas
internal/shell/dispatch.go+2−1
compare with main
unifiedsplit
@@ -197,6 +197,7 @@ func dispatchShebang(ctx context.Context, scriptPath string, probe []byte, args
197 cmd.Stdin = hc.Stdin
197 cmd.Stdin = hc.Stdin
198 cmd.Stdout = hc.Stdout
198 cmd.Stdout = hc.Stdout
199 cmd.Stderr = hc.Stderr
199 cmd.Stderr = hc.Stderr
200+ isolateProcess(cmd)
200
201
201 if err := cmd.Run(); err != nil {
202 if err := cmd.Run(); err != nil {
202 var exitErr *exec.ExitError
203 var exitErr *exec.ExitError
@@ -392,7 +393,7 @@ func runShellSource(ctx context.Context, path string, args []string, blockFuncs
392 interp.Interactive(false),
393 interp.Interactive(false),
393 interp.Env(hc.Env),
394 interp.Env(hc.Env),
394 interp.Dir(hc.Dir),
395 interp.Dir(hc.Dir),
395− interp.ExecHandlers(standardHandlers(blockFuncs)...),
396+ execHandlerOption(blockFuncs),
396 }
397 }
397 if len(args) > 1 {
398 if len(args) > 1 {
398 // Params with a leading "--" avoids any of args[1:] being
399 // Params with a leading "--" avoids any of args[1:] being
internal/shell/exec_unix.gonew file+108
@@ -0,0 +1,108 @@
1+//go:build !windows
2+
3+package shell
4+
5+import (
6+ "context"
7+ "fmt"
8+ "io"
9+ "os/exec"
10+ "syscall"
11+ "time"
12+
13+ "mvdan.cc/sh/v3/interp"
14+)
15+
16+// defaultKillTimeout matches mvdan's DefaultExecHandler default. Extracted
17+// so the coupling to upstream is explicit rather than a buried literal.
18+const defaultKillTimeout = 2 * time.Second
19+
20+// isolateProcess sets SysProcAttr so the child runs in its own session,
21+// fully detached from Crush's controlling terminal. This prevents shells
22+// like zsh from grabbing the TTY for job control, which would cause
23+// "suspended (tty input)" and corrupt Bubble Tea's terminal rendering.
24+// It also prevents child processes from sending signals to Crush's process
25+// group.
26+func isolateProcess(cmd *exec.Cmd) {
27+ if cmd.SysProcAttr == nil {
28+ cmd.SysProcAttr = &syscall.SysProcAttr{}
29+ }
30+ cmd.SysProcAttr.Setsid = true
31+}
32+
33+// processGroupExecHandler returns an ExecHandlerFunc that replaces
34+// interp.DefaultExecHandler with one that fully isolates child processes
35+// from Crush's session and controlling terminal.
36+//
37+// Without this, shells like zsh that enable job control when sourcing
38+// framework files will attempt to take over the TTY, causing SIGTTIN/SIGTTOU
39+// signals and corrupting the parent terminal state.
40+//
41+// The implementation mirrors interp.DefaultExecHandler with two additions:
42+// isolateProcess(&cmd) after construction, and negative-PID signal targeting
43+// in the cancellation callback so the entire child process group is killed.
44+func processGroupExecHandler(killTimeout time.Duration) interp.ExecHandlerFunc {
45+ return func(ctx context.Context, args []string) error {
46+ hc := interp.HandlerCtx(ctx)
47+ path, err := interp.LookPathDir(hc.Dir, hc.Env, args[0])
48+ if err != nil {
49+ fmt.Fprintln(hc.Stderr, err)
50+ return interp.ExitStatus(127)
51+ }
52+
53+ cmd := exec.Cmd{
54+ Path: path,
55+ Args: args,
56+ Env: execEnvList(hc.Env),
57+ Dir: hc.Dir,
58+ Stdin: hc.Stdin,
59+ Stdout: hc.Stdout,
60+ Stderr: hc.Stderr,
61+ }
62+ isolateProcess(&cmd)
63+
64+ err = cmd.Start()
65+ if err == nil {
66+ stopf := context.AfterFunc(ctx, func() {
67+ if killTimeout <= 0 {
68+ _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
69+ return
70+ }
71+ // Signal the child's process group (negative PID) so
72+ // grandchildren also receive it.
73+ _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGINT)
74+ time.Sleep(killTimeout)
75+ _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
76+ })
77+ defer stopf()
78+
79+ err = cmd.Wait()
80+ }
81+
82+ return exitStatusFromError(ctx, hc.Stderr, err)
83+ }
84+}
85+
86+// exitStatusFromError translates an exec error into an interp exit status,
87+// matching the conventions of interp.DefaultExecHandler. Extracted so the
88+// platform-specific exec handler stays focused on isolation mechanics.
89+func exitStatusFromError(ctx context.Context, stderr io.Writer, err error) error {
90+ if err == nil {
91+ return nil
92+ }
93+ switch err := err.(type) {
94+ case *exec.ExitError:
95+ if status, ok := err.Sys().(syscall.WaitStatus); ok && status.Signaled() {
96+ if ctx.Err() != nil {
97+ return ctx.Err()
98+ }
99+ return interp.ExitStatus(128 + uint8(status.Signal()))
100+ }
101+ return interp.ExitStatus(uint8(err.ExitCode()))
102+ case *exec.Error:
103+ fmt.Fprintf(stderr, "%v\n", err)
104+ return interp.ExitStatus(127)
105+ default:
106+ return err
107+ }
108+}
internal/shell/exec_windows.gonew file+23
@@ -0,0 +1,23 @@
1+//go:build windows
2+
3+package shell
4+
5+import (
6+ "os/exec"
7+ "time"
8+
9+ "mvdan.cc/sh/v3/interp"
10+)
11+
12+// defaultKillTimeout matches mvdan's DefaultExecHandler default.
13+const defaultKillTimeout = 2 * time.Second
14+
15+// isolateProcess is a no-op on Windows. Session isolation via Setsid is a
16+// Unix-only concept; Windows uses CREATE_NEW_PROCESS_GROUP which mvdan's
17+// default handler already handles adequately.
18+func isolateProcess(_ *exec.Cmd) {}
19+
20+// processGroupExecHandler returns interp.DefaultExecHandler on Windows.
21+func processGroupExecHandler(killTimeout time.Duration) interp.ExecHandlerFunc {
22+ return interp.DefaultExecHandler(killTimeout)
23+}
internal/shell/expand.go+1−1
@@ -93,7 +93,7 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error
93 interp.Interactive(false),
93 interp.Interactive(false),
94 interp.Env(expand.ListEnviron(env...)),
94 interp.Env(expand.ListEnviron(env...)),
95 interp.Dir(s.cwd),
95 interp.Dir(s.cwd),
96− interp.ExecHandlers(standardHandlers(s.blockFuncs)...),
96+ execHandlerOption(s.blockFuncs),
97 }
97 }
98 if strict {
98 if strict {
99 // Match the outer NoUnset: an unset $VAR inside
99 // Match the outer NoUnset: an unset $VAR inside
internal/shell/isolation_unix_test.gonew file+143
@@ -0,0 +1,143 @@
1+//go:build !windows
2+
3+package shell
4+
5+import (
6+ "bytes"
7+ "context"
8+ "os"
9+ "os/exec"
10+ "os/signal"
11+ "strings"
12+ "syscall"
13+ "testing"
14+ "time"
15+)
16+
17+// TestProcessIsolation_SignalDoesNotReachParent verifies that a child
18+// sending SIGINT to its own process group does not deliver the signal to
19+// the parent (test) process. Without Setsid isolation, the child shares
20+// the parent's process group and the signal would kill the test.
21+func TestProcessIsolation_SignalDoesNotReachParent(t *testing.T) {
22+ t.Parallel()
23+
24+ sigCh := make(chan os.Signal, 1)
25+ signal.Notify(sigCh, syscall.SIGINT)
26+ defer signal.Stop(sigCh)
27+
28+ var stderr bytes.Buffer
29+ err := Run(t.Context(), RunOptions{
30+ Command: `sh -c 'kill -INT -$$'`,
31+ Cwd: t.TempDir(),
32+ Env: os.Environ(),
33+ Stderr: &stderr,
34+ })
35+
36+ if err == nil {
37+ t.Log("child exited 0 after self-signal (unexpected but not a failure)")
38+ }
39+
40+ select {
41+ case sig := <-sigCh:
42+ t.Fatalf("SIGINT leaked to parent process: %v (stderr=%q)", sig, stderr.String())
43+ case <-time.After(200 * time.Millisecond):
44+ }
45+}
46+
47+// TestProcessIsolation_TTYAccessDoesNotBlock verifies that a child trying
48+// to read from /dev/tty does not block or suspend the parent. With Setsid,
49+// the child has no controlling terminal, so /dev/tty access fails immediately
50+// with ENXIO.
51+func TestProcessIsolation_TTYAccessDoesNotBlock(t *testing.T) {
52+ t.Parallel()
53+
54+ ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second)
55+ defer cancel()
56+
57+ var stderr bytes.Buffer
58+ done := make(chan error, 1)
59+ go func() {
60+ done <- Run(ctx, RunOptions{
61+ Command: `sh -c 'cat < /dev/tty'`,
62+ Cwd: t.TempDir(),
63+ Env: os.Environ(),
64+ Stderr: &stderr,
65+ })
66+ }()
67+
68+ select {
69+ case err := <-done:
70+ if err == nil {
71+ t.Fatal("expected error from /dev/tty access in detached session, got nil")
72+ }
73+ case <-time.After(3 * time.Second):
74+ t.Fatalf("/dev/tty access blocked for >3s; session isolation may be broken (stderr=%q)", stderr.String())
75+ }
76+}
77+
78+// TestProcessIsolation_ZshJobControlDoesNotSuspend simulates the exact
79+// scenario that caused the original bug: running zsh with job control
80+// enabled. Without session isolation, zsh tries to become the foreground
81+// process group leader on the controlling terminal, gets SIGTTIN, and
82+// suspends. Skips if zsh is not installed.
83+func TestProcessIsolation_ZshJobControlDoesNotSuspend(t *testing.T) {
84+ t.Parallel()
85+
86+ if _, err := exec.LookPath("zsh"); err != nil {
87+ t.Skip("zsh not installed")
88+ }
89+
90+ ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second)
91+ defer cancel()
92+
93+ var stdout, stderr bytes.Buffer
94+ done := make(chan error, 1)
95+ go func() {
96+ done <- Run(ctx, RunOptions{
97+ Command: `zsh -i -c 'echo ok'`,
98+ Cwd: t.TempDir(),
99+ Env: os.Environ(),
100+ Stdout: &stdout,
101+ Stderr: &stderr,
102+ })
103+ }()
104+
105+ select {
106+ case err := <-done:
107+ if err != nil {
108+ t.Logf("zsh exited with error (may be expected): %v", err)
109+ }
110+ out := strings.TrimSpace(stdout.String())
111+ if !strings.Contains(out, "ok") {
112+ t.Errorf("expected 'ok' in stdout, got %q (stderr=%q)", out, stderr.String())
113+ }
114+ case <-time.After(5 * time.Second):
115+ t.Fatalf("zsh -i -c did not complete within 5s; likely suspended on TTY (stderr=%q)", stderr.String())
116+ }
117+}
118+
119+// TestProcessIsolation_ChildProcessGroupKill verifies that when context
120+// is cancelled, the signal reaches the entire child process group (not
121+// just the direct child).
122+func TestProcessIsolation_ChildProcessGroupKill(t *testing.T) {
123+ t.Parallel()
124+
125+ ctx, cancel := context.WithTimeout(t.Context(), 500*time.Millisecond)
126+ defer cancel()
127+
128+ start := time.Now()
129+ err := Run(ctx, RunOptions{
130+ Command: `sh -c 'sleep 60 & wait'`,
131+ Cwd: t.TempDir(),
132+ Env: os.Environ(),
133+ })
134+ elapsed := time.Since(start)
135+
136+ if err == nil {
137+ t.Fatal("expected error from cancelled context")
138+ }
139+ // Allow up to 4s: 500ms context timeout + 2s kill timeout + margin.
140+ if elapsed > 4*time.Second {
141+ t.Fatalf("cancellation took %v; expected prompt process group kill", elapsed)
142+ }
143+}
internal/shell/run.go+21−1
@@ -4,6 +4,7 @@ import (
4 "context"
4 "context"
5 "fmt"
5 "fmt"
6 "io"
6 "io"
7+ "slices"
7 "strings"
8 "strings"
8
9
9 "mvdan.cc/sh/moreinterp/coreutils"
10 "mvdan.cc/sh/moreinterp/coreutils"
@@ -95,10 +96,29 @@ func newRunner(cwd string, env []string, stdin io.Reader, stdout, stderr io.Writ
95 interp.Interactive(false),
96 interp.Interactive(false),
96 interp.Env(expand.ListEnviron(env...)),
97 interp.Env(expand.ListEnviron(env...)),
97 interp.Dir(cwd),
98 interp.Dir(cwd),
98− interp.ExecHandlers(standardHandlers(blockFuncs)...),
99+ execHandlerOption(blockFuncs),
99 )
100 )
100}
101}
101
102
103+// execHandlerOption returns an interp.RunnerOption that installs the
104+// standard Crush middleware chain (builtins, script dispatch, block list)
105+// on top of a process-group-isolated base exec handler.
106+//
107+// We use interp.ExecHandler (singular) with a manually-built chain rather
108+// than interp.ExecHandlers because the latter always appends
109+// interp.DefaultExecHandler as the final handler, which lacks process group
110+// isolation. Without isolation, shells like zsh that set up job control
111+// when sourcing framework files can send SIGINT/SIGTERM to Crush's process
112+// group and crash the parent.
113+func execHandlerOption(blockFuncs []BlockFunc) interp.RunnerOption {
114+ base := processGroupExecHandler(defaultKillTimeout)
115+ handler := base
116+ for _, mw := range slices.Backward(standardHandlers(blockFuncs)) {
117+ handler = mw(handler)
118+ }
119+ return interp.ExecHandler(handler) //nolint:staticcheck // ExecHandlers always appends DefaultExecHandler which lacks process isolation.
120+}
121+
102// nonInteractiveEnvVars are forced on every shell execution to prevent
122// nonInteractiveEnvVars are forced on every shell execution to prevent
103// commands from hanging on a nonexistent TTY. These are always applied
123// commands from hanging on a nonexistent TTY. These are always applied
104// regardless of the caller's environment because Crush shells are never
124// regardless of the caller's environment because Crush shells are never