Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/runtime/shell/builtin/exit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,24 @@
return Self::fail(interp, cmd, b"exit: too many arguments\n");
}
};
// Intentional divergence from bash: this completes only the current
// Cmd rather than unwinding the whole script.
Self::request_exit(interp, cmd, code);
Builtin::done(interp, cmd, code)
}

/// Like bash, a bad argument still ends the script, with status 1.
fn fail(interp: &Interpreter, cmd: NodeId, msg: &[u8]) -> Yield {
Self::state_mut(interp, cmd).state = State::WaitingIo;
Self::request_exit(interp, cmd, 1);
Builtin::write_failing_error(interp, cmd, msg, 1)
}

/// End the enclosing execution context with `code`. A subshell, command
/// substitution, or pipeline element owns its own `ShellExecEnv`, so
/// `exit` never escapes the context that ran it.
fn request_exit(interp: &Interpreter, cmd: NodeId, code: crate::shell::ExitCode) {
interp.as_cmd_mut(cmd).base.shell_mut().exit_requested = Some(code);
}

Check failure on line 52 in src/runtime/shell/builtin/exit.rs

View check run for this annotation

Claude / Claude Code Review

exit inside async command (&) leaks to parent context

`exit` inside an async command (`&`) leaks to the parent script. `Async::init` stores the parent's `ShellExecEnv` pointer without duping it, so `exit 5 &` calls `request_exit` on the parent env — once the async body runs, the parent's next `Stmt::next()` bails and the script exits 5 (bash runs `&` in a subshell, so `exit` never escapes). Before this PR `exit` wrote nothing to the env so sharing was harmless; the PR's claim that "`ShellExecEnv` is already duped at exactly the points bash forks a
Comment thread
robobun marked this conversation as resolved.

pub(crate) fn on_io_writer_chunk(
interp: &Interpreter,
cmd: NodeId,
Expand Down
7 changes: 7 additions & 0 deletions src/runtime/shell/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,7 @@ impl Interpreter {
__cwd: cwd_arr,
cwd_fd,
async_pids: SmolList::default(),
exit_requested: None,
}),
root_io: JsCell::new(IO {
stdin: crate::shell::io::InKind::Fd(stdin_reader),
Expand Down Expand Up @@ -1793,6 +1794,10 @@ pub struct ShellExecEnv {
pub __cwd: Vec<u8>,
pub cwd_fd: Fd,
pub async_pids: SmolList<PidT, 4>,
/// Status the `exit` builtin asked this execution context to end with.
/// Nodes sharing this env stop running children. A subshell, command
/// substitution, or pipeline element gets its own env, which scopes `exit`.
pub exit_requested: Option<ExitCode>,
}

pub enum Bufio {
Expand Down Expand Up @@ -1958,6 +1963,8 @@ impl ShellExecEnv {
__cwd: self.__cwd.clone(),
cwd_fd: dupedfd,
async_pids: SmolList::default(),
// Fresh execution context: `exit` neither carries in nor escapes.
exit_requested: None,
});
Ok(bun_core::heap::into_raw(duped))
}
Expand Down
9 changes: 8 additions & 1 deletion src/runtime/shell/states/Base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
//! `parent: NodeId` and the `*mut ShellExecEnv` (which may be owned or
//! borrowed — see field doc) are stored here.

use crate::shell::interpreter::{NodeId, ShellExecEnv, StateKind};
use crate::shell::interpreter::{ExitCode, NodeId, ShellExecEnv, StateKind};

pub struct Base {
pub kind: StateKind,
Expand Down Expand Up @@ -49,6 +49,13 @@ impl Base {
// time.
unsafe { &mut *self.shell }
}

/// `Some(status)` once `exit` ran in this node's execution context: stop
/// walking children and unwind with it. See `ShellExecEnv::exit_requested`.
#[inline]
pub fn exit_requested(&self) -> Option<ExitCode> {
self.shell().exit_requested
}
}

/// `error{Sys}` — see `Interpreter::try_`.
Expand Down
6 changes: 6 additions & 0 deletions src/runtime/shell/states/Binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ impl Binary {
};
let n = node.get();

// Checked before the short-circuit below: `exit 0 && echo hi` must not
// run the right-hand side even though the left side succeeded.
if let Some(code) = interp.as_binary(this).base.exit_requested() {
return interp.child_done(parent, this, code);
}

if let Some(right) = right_exit {
return interp.child_done(parent, this, right);
}
Expand Down
6 changes: 6 additions & 0 deletions src/runtime/shell/states/Stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@
me.base.shell,
)
};
// Every command in a script body or an `if` branch is spawned as a
// Stmt, so refusing to run one here is what unwinds the enclosing list.
// `Binary` spawns its right side directly, and checks separately.
if let Some(code) = interp.as_stmt(this).base.exit_requested() {
return interp.child_done(parent, this, code);
}

Check warning on line 66 in src/runtime/shell/states/Stmt.rs

View check run for this annotation

Claude / Claude Code Review

exit code lost when `if exit N` (no else) is a pipeline element

This invariant ("`If` needs no check of its own") holds for suppressing execution but not for propagating the exit code when `If`'s parent is `Pipeline` — e.g. `echo hi | if exit 5; then echo t; fi` yields 0 instead of bash's 5. `Pipeline` spawns `If` directly with a duped env (Pipeline.rs:257), and when the cond fails with no `else`, `If::next` emits `Action::Done(0)` (If.rs:112, and the elif path at :142); Pipeline records that 0 verbatim and frees the duped env without reading `exit_requested
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
if idx >= len {
return interp.child_done(parent, this, last.unwrap_or(0));
}
Expand Down
6 changes: 2 additions & 4 deletions test/js/bun/shell/bunshell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2644,12 +2644,10 @@ describe("subshell", () => {
// test_oE 'effect of subshell'
TestBuilder.command /* sh */ `
a=1
# (a=2; echo $a; exit; echo not reached)
# NOTE: We actually implemented exit wrong so changing this for now until we fix it
(a=2; echo $a; exit; echo reached)
(a=2; echo $a; exit; echo not reached)
echo $a
`
.stdout("2\nreached\n1\n")
.stdout("2\n1\n")
.runAsTest("effect of subshell");

// test_x -e 23 'exit status of subshell'
Expand Down
83 changes: 83 additions & 0 deletions test/js/bun/shell/commands/exit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,87 @@ describe("exit", async () => {

// prettier-ignore
TestBuilder.command`exit abc`.exitCode(1).stderr("exit: numeric argument required\n").runAsTest("numeric argument required");

describe("ends the script", async () => {
TestBuilder.command`echo start; exit 5; echo never`
.exitCode(5)
.stdout("start\n")
.runAsTest("skips the statements after it");

TestBuilder.command`exit 1; exit 2`.exitCode(1).runAsTest("the first exit wins");

// Not short-circuiting on a status: `exit 0` ends an && chain and
// `exit 5` ends an || chain, where the status alone would keep going.
TestBuilder.command`exit 0 && echo never`.exitCode(0).stdout("").runAsTest("exit 0 ends an && chain");

TestBuilder.command`exit 5 || echo never`.exitCode(5).stdout("").runAsTest("exit 5 ends an || chain");

TestBuilder.command`false || exit 3; echo never`
.exitCode(3)
.stdout("")
.runAsTest("from the right side of ||");

TestBuilder.command`if true; then exit 7; echo never; fi; echo never2`
.exitCode(7)
.stdout("")
.runAsTest("from an if body");

TestBuilder.command`if exit 5; then echo t; else echo f; fi`
.exitCode(5)
.stdout("")
.runAsTest("from an if condition");

// A compound command may be followed by another expression in the same
// statement (`fi` is not a statement terminator).
TestBuilder.command`if true; then exit 5; fi echo never`
.exitCode(5)
.stdout("")
.runAsTest("from a compound command sharing a statement");

TestBuilder.command`exit abc; echo never`
.exitCode(1)
.stdout("")
.stderr("exit: numeric argument required\n")
.runAsTest("on a numeric argument error");

TestBuilder.command`exit 3 5; echo never`
.exitCode(1)
.stdout("")
.stderr("exit: too many arguments\n")
.runAsTest("on too many arguments");
});

// `exit` ends the execution context that ran it, not the whole interpreter:
// a subshell, command substitution, or pipeline element is its own context.
describe("stays inside its execution context", async () => {
TestBuilder.command`(echo sub; exit 6; echo never); echo after`
.exitCode(0)
.stdout("sub\nafter\n")
.runAsTest("subshell");

TestBuilder.command`(echo sub; exit 6; echo never) && echo never2`
.exitCode(6)
.stdout("sub\n")
.runAsTest("subshell status reaches the parent");

TestBuilder.command`echo cs=$(echo sub; exit 4; echo never); echo after`
.exitCode(0)
.stdout("cs=sub\nafter\n")
.runAsTest("command substitution");

TestBuilder.command`echo a; exit 5 | cat; echo b`
.exitCode(0)
.stdout("a\nb\n")
.runAsTest("pipeline element");

TestBuilder.command`(if true; then exit 2; fi; echo never); echo after`
.exitCode(0)
.stdout("after\n")
.runAsTest("if body nested in a subshell");

TestBuilder.command`if true; then (exit 2); fi; echo after`
.exitCode(0)
.stdout("after\n")
.runAsTest("subshell nested in an if body");
});
});
Loading