Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
26 changes: 26 additions & 0 deletions src/js_parser/p.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,12 @@ pub struct P<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> {
/// hard-overflow. Same `MAX_STMT_DEPTH` rationale as `interchange/json.rs`.
pub parse_stmt_depth: u32,

/// Set once `report_stack_overflow` has logged "Maximum call stack size
/// exceeded" so sibling subtrees that hit the same stack limit don't each
/// log a duplicate error. `Cell` because some reporters (`SideEffects::
/// to_boolean`) only hold `&P`.
pub reported_stack_overflow: core::cell::Cell<bool>,

/// When this flag is enabled, we attempt to fold all expressions that
/// TypeScript would consider to be "constant expressions". This flag is
/// enabled inside each enum body block since TypeScript requires numeric
Expand Down Expand Up @@ -776,6 +782,19 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
unsafe { &mut *self.log.as_ptr() }
}

/// Logs "Maximum call stack size exceeded" (once per parse) when a pass
/// that recurses over the AST runs out of stack. `_parse` halts with a
/// SyntaxError as soon as the log has errors, so callers only need to stop
/// recursing after calling this.
pub fn report_stack_overflow(&self, loc: bun_ast::Loc) {
if self.reported_stack_overflow.get() {
return;
}
self.reported_stack_overflow.set(true);
self.log()
.add_error(Some(self.source), loc, b"Maximum call stack size exceeded");
}

/// Safe mutable projection of `nearest_stmt_list`.
///
/// The pointer targets a `ListManaged` living on a parent
Expand Down Expand Up @@ -5845,6 +5864,12 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
}

fn expr_can_be_removed_if_unused_without_dce_check(&mut self, expr: &Expr) -> bool {
// This walks arbitrarily deep ASTs; report the stack overflow instead
// of crashing. The parse fails with that error, so the answer is moot.
if !self.stack_check.is_safe_to_recurse() {
self.report_stack_overflow(expr.loc);
return false;
}
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
match &expr.data {
js_ast::ExprData::ENull(_)
| js_ast::ExprData::EUndefined(_)
Expand Down Expand Up @@ -9203,6 +9228,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
log,
stack_check: bun_core::StackCheck::init(),
parse_stmt_depth: 0,
reported_stack_overflow: core::cell::Cell::new(false),
arena,
then_catch_chain: ThenCatchChain {
next_target: null_expr_data(),
Expand Down
14 changes: 14 additions & 0 deletions src/js_parser/scan/scan_side_effects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,13 @@
if !p.options.features.dead_code_elimination {
return Some(expr);
}
// This walks arbitrarily deep ASTs; report the stack overflow instead
// of crashing. The parse fails with that error, so the value returned
// here never reaches the printer.
if !p.stack_check.is_safe_to_recurse() {
p.report_stack_overflow(expr.loc);
return Some(expr);
Comment thread
robobun marked this conversation as resolved.
}
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
// PORT NOTE: `Expr`/`ExprData`/`StoreRef<_>` are all `Copy`. We match on
// `expr.data` *by value* so `expr` itself is never borrowed across a
// recursive `simplify_unused_expr(p, ..)` call. Mutations to boxed
Expand Down Expand Up @@ -883,6 +890,13 @@
if !p.options.features.dead_code_elimination {
return Result::default();
}
// Recurses through nested unary/binary operands; report the stack
// overflow instead of crashing. The parse fails with that error, so
// the "unknown" result returned here never affects emitted code.
if !p.stack_check.is_safe_to_recurse() {
p.report_stack_overflow(p.lexer.loc());
return Result::default();

Check warning on line 898 in src/js_parser/scan/scan_side_effects.rs

View check run for this annotation

Claude / Claude Code Review

to_boolean reports stack overflow at EOF loc instead of expression loc

Minor: this is the only `report_stack_overflow()` call site that passes `p.lexer.loc()` instead of an expression `loc` — but `to_boolean` runs during the visit pass, after the lexer has consumed the whole source, so `lexer.loc()` points at EOF rather than the deep expression. In practice the once-per-parse guard means `visit_expr_in_out` almost always reports first with a correct `e.loc` and this call is a no-op, so it's a consistency nit rather than a UX bug; `Loc::EMPTY` (or threading a `loc`
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
}
match exp {
ExprData::ENull(_) | ExprData::EUndefined(_) => Result {
value: false,
Expand Down
11 changes: 11 additions & 0 deletions src/js_parser/visit/visit_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,17 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
}

pub fn visit_expr_in_out(&mut self, e: &mut Expr, in_: ExprIn) {
// The visit pass uses noticeably more stack per AST level than the
// parse pass, so an expression that parsed under `parse_expr_common`'s
// stack check (e.g. tens of thousands of chained unary operators or
// nested calls) can still overflow here. Stop descending and report the
// same error the parse pass uses; `_parse` halts on logged errors right
// after the visit pass, so the partially-visited AST never gets printed.
if !self.stack_check.is_safe_to_recurse() {
self.report_stack_overflow(e.loc);
return;
}
Comment thread
claude[bot] marked this conversation as resolved.

if in_.assign_target != js_ast::AssignTarget::None && !self.is_valid_assignment_target(e) {
self.log()
.add_error(Some(self.source), e.loc, b"Invalid assignment target");
Expand Down
31 changes: 31 additions & 0 deletions src/js_printer/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1758,6 +1758,14 @@ pub mod __gated_printer {

pub binary_expression_stack: Vec<BinaryExpressionVisitor<'a>>,

/// `print_expr` recurses per AST level and its frames can be larger than
/// the parser's, so an AST that parsed fine can still exhaust the stack
/// here. When that's about to happen `print_expr` bails out and sets
/// this flag; the print entry points turn it into an error instead of
/// returning truncated output.
pub stack_check: bun_core::StackCheck,
pub stack_overflowed: bool,

pub was_lazy_export: bool,
// PORT NOTE: Zig used `if (!may_have_module_info) void else ?*ModuleInfo` — in Rust we always
// carry the Option and gate at call sites with MAY_HAVE_MODULE_INFO.
Expand Down Expand Up @@ -3256,7 +3264,21 @@ pub mod __gated_printer {
}
}

/// Returns an error if `print_expr` had to bail out on a too-deep AST,
/// so callers don't hand back truncated output.
pub fn check_stack_overflow(&self) -> Result<(), bun_core::Error> {
if self.stack_overflowed {
return Err(bun_core::err!("StackOverflow"));
}
Ok(())
}

pub fn print_expr(&mut self, expr: Expr, level: Level, in_flags: ExprFlagSet) {
if !self.stack_check.is_safe_to_recurse() {
self.stack_overflowed = true;
return;
}

let mut flags = in_flags;

match &expr.data {
Expand Down Expand Up @@ -7079,6 +7101,8 @@ pub mod __gated_printer {
symbol_counter: 0,
temporary_bindings: Vec::new(),
binary_expression_stack: Vec::new(),
stack_check: bun_core::StackCheck::init(),
stack_overflowed: false,
was_lazy_export: false,
module_info: None,
};
Expand Down Expand Up @@ -8147,6 +8171,7 @@ pub fn print_ast<'a, W: WriterTrait, const ASCII_ONLY: bool, const GENERATE_SOUR
printer.print_semicolon_if_needed();
}
}
printer.check_stack_overflow()?;

let have_module_info = PrinterType::<W, ASCII_ONLY, GENERATE_SOURCE_MAP>::MAY_HAVE_MODULE_INFO
&& printer.module_info.is_some();
Expand Down Expand Up @@ -8244,6 +8269,7 @@ pub fn print_json<W: WriterTrait>(
printer.binary_expression_stack = Vec::new();

printer.print_expr(expr, js_ast::op::Level::Lowest, ExprFlagSet::empty());
printer.check_stack_overflow()?;
printer.writer.get_error()?;
printer.writer.done()?;

Expand Down Expand Up @@ -8392,6 +8418,10 @@ pub fn print_with_writer_and_platform<
}
}

if let Err(err) = printer.check_stack_overflow() {
return PrintResult::Err(err);
}

if let Err(err) = printer.writer.done() {
// In bundle_v2, this is backed by an arena, but incremental uses
// `dev.allocator` for this buffer, so it must be freed.
Expand Down Expand Up @@ -8472,6 +8502,7 @@ pub fn print_common_js<
printer.print_semicolon_if_needed();
}
}
printer.check_stack_overflow()?;

// Add a couple extra newlines at the end
printer.writer.print_slice(b"\n\n");
Expand Down
51 changes: 51 additions & 0 deletions test/bundler/transpiler/transpiler.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3676,6 +3676,57 @@
expect(async () => await transpiler.transform(code)).toThrow(`Maximum call stack size exceeded`);
});

// Deeply nested expressions (chained prefix unary operators, nested call
// arguments, nested array literals, ...) used to parse fine but then overflow
// the native stack in the visit pass, killing the process with SIGSEGV instead
// of reporting "Maximum call stack size exceeded" like other deep inputs do.
it("deeply nested expressions error instead of crashing the process", () => {
const script = `
const repeat = (fill, count) => Buffer.alloc(fill.length * count, fill).toString();
const shapes = [
n => repeat("- ", n) + "1",
n => repeat("f(", n) + "1" + repeat(")", n),
n => repeat("[", n) + "1" + repeat("]", n),
n => "void " + repeat("- ", n) + "1",
];
for (const shape of shapes) {
for (const n of [4000, 20000, 100000]) {
try {
new Bun.Transpiler({ loader: "js" }).transformSync(shape(n));
} catch (e) {
if (!String(e?.message).includes("Maximum call stack size exceeded")) throw e;
}

Check warning on line 3698 in test/bundler/transpiler/transpiler.test.js

View check run for this annotation

Claude / Claude Code Review

Test catch rejects the printer StackOverflow error path

The catch only swallows errors containing `Maximum call stack size exceeded`, but this PR also adds a printer-side guard that surfaces as `"StackOverflow Failed to print code"` (lib.rs:3270 → JSTranspiler.rs:1621 → JSGlobalObject.rs:904) — that string doesn't match, so if a build config ever trips the print guard before the visit guard the catch re-throws, the child exits non-zero without printing `depth-ok`, and the test fails even though nothing crashed. The PR description itself lists "/ a pr
Comment thread
robobun marked this conversation as resolved.
Outdated
}
}
console.log("depth-ok");
`;
const { stdout, exitCode, signalCode } = Bun.spawnSync({
cmd: [bunExe(), "-e", script],
stdout: "pipe",
stderr: "pipe",
env: bunEnv,
});

expect(stdout.toString()).toBe("depth-ok\n");
expect([exitCode, signalCode ?? undefined]).toEqual([0, undefined]);
});

it("running a file with deeply nested unary operators does not crash the process", () => {
const code = Buffer.alloc(2 * 4000, "- ").toString() + "1";
const { exitCode, signalCode } = Bun.spawnSync({
cmd: [bunExe(), "-e", code],
stdout: "pipe",
stderr: "pipe",
env: bunEnv,
});

// Depending on the available stack this either evaluates fine or reports a
// clean "Maximum call stack size exceeded" error; it must never die from a
// signal.
expect(signalCode ?? undefined).toBeUndefined();
expect(exitCode).not.toBeNull();
});
Comment thread
claude[bot] marked this conversation as resolved.

describe("arrow function parsing after const declaration (scope mismatch bug)", () => {
const transpiler = new Bun.Transpiler({ loader: "tsx" });

Expand Down
Loading