Repository navigation
Conversation
…writing decorated #private accesses The lowering of a class with a decorated #private member walks the class body a second time to rewrite each access to that member. That walk recursed once per AST node with no stack check, so a chain of binary operators, which the parser and the visit pass take in a loop, overflowed the native stack. The EBinary arm of the walk now takes the left side of a chain in a loop, on the parser's reusable stack, so a chain of any length is lowered. Its three recursive entry points check the stack like the visit pass does, so a depth the walk cannot take ends in the error the parser reports for every other deep input.
|
How this was reproduced, for anyone re-running it. // decorated-private-long-chain.mjs
import { spawnSync } from "node:child_process";
const n = +(process.argv[2] ?? 100000);
const src = (head, n) => `class A { ${head} m() { return ${"a+".repeat(n)}1 } }`;
if (process.argv[3]) {
const head = { decorated: "@d #p() {}", plain: "#p() {}", public: "@d p() {}" }[process.argv[3]];
const out = new Bun.Transpiler({ loader: "ts" }).transformSync(src(head, n));
console.log("transpiled to " + out.length + " characters");
process.exit(0);
}
let bad = 0;
for (const mode of ["plain", "public", "decorated"]) {
const r = spawnSync(process.execPath, [import.meta.filename, String(n), mode], { encoding: "utf8" });
if (r.signal) bad++;
console.log(mode + ": " + (r.signal ? "killed by " + r.signal : "exit " + r.status + ", " + r.stdout.trim()));
}
process.exit(bad ? 1 : 0);On On this branch:
With the decorator on a private method, field, This is not a regression. 1.4.2, 1.4.1, 1.4.0 and 1.3.14 die the same way. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. WalkthroughThe decorator private-access rewrite now checks recursion safety and traverses left-associated binary expressions through a simplifier stack. New stress tests cover long chains, overflow reporting, and operand rewrite order. ChangesDecorator private-access rewriting
Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is identified; merge after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the new iterative EBinary arm against the old recursion: it visits the same nodes in the same order (innermost left, its right, then each parent's right bottom-up), the two whole-node special cases (x.#p = v, #p in x) can only occur at the innermost link and still go through the recursive call, and the reuse of binary_expression_simplify_stack is bounded by stack_bottom, so nested entry from the visit pass cannot pop another frame's entries.
Extended reasoning...
The change touches only the decorator-lowering private-access rewrite in src/js_parser/lower/lower_decorators.rs (three stack-check guards plus a loop-based walk of left-leaning binary chains) and adds subprocess tests in test/bundler/transpiler/es-decorators.test.ts; no security-sensitive surface is involved. Inline findings were posted, so this note only records the control-flow equivalence and stack-reuse checks that were examined and ruled out.
…, and assert the one error The depth probe returned at the first shape that reported, so the tagged template shape never ran, and it passed when no depth reported at all. Each shape is now its own test and the probe fails when nothing is reported. The test for a chain that the minifier builds from flat statements accepted the printer's error too. It now asserts the error of the walk.
This branch has not been deployed
2 participants
Problem
#privatemember dies on a long chain of binary operators: exit 139, no message, fromBun.Transpiler,bun buildandbun run. Without the decorator the same file transpiles.rewrite_private_accesses_in_expr(src/js_parser/lower/lower_decorators.rs:274) recurses per AST node with no stack check. The parser and the visit pass take a chain in a loop, so their checks never see its length.@d #m(){}members with the comma operator, and--minify-syntaxjoins 9,000 plain statements.Fix
EBinaryarm walks downleftin a loop, on the parser's reusablebinary_expression_simplify_stack. This walk takes a chain of any length.Maximum call stack size exceeded.es-decorators.test.ts, 13 new tests, 11 fail on main with SIGSEGV. Also the decorator, stack overflow andtranspiler.test.jssuites, 847 pass.Background
#privatemember. The lowering makes one aWeakMapand walks the class body again to rewrite eachx.#p. Only such a class runs that walk.is_safe_to_recurse()is the parser's guard against native stack overflow, merged for this class of crash in Error instead of crashing on deeply nested expressions in the transpiler #31242 and Error instead of crashing on deeply nested statements in the transpiler #31333. Four other passes still take a frame per link.#privatemember #42651, js_parser: keepthisfor a tagged template through a decorated#privatemember #42677 and Optional chains through lowered private members do not short-circuit in decorated classes #31910 are open on the same walk. Considered the check alone, which turns the reported file into an error. Considered esbuild's rewrite inside the visit, which renumbers the lowering temporaries.Downsides
#privateclass pays about 12 instructions more per call of the walk: 439,600 to 592,400 for 200 classes, +1.2% of that transform. Other files run 0 of it. Code: +876 B.panic: index out of bounds, exit 134) where main printed output. That panic is main's own, reachable with no class, and js_parser: replace skipped expressions with E::Missing and skip argument checks after a stack overflow #44467 fixes it.Notes
Repro (
main13a98b0 and Bun 1.4.3-canary:Segmentation fault. This branch: the output.)How the loop keeps the output. The two forms that replace a whole binary node (
x.#p = vand#p in x) need aleftthat is not a binary, so only the innermost link of a chain can be one, and that link goes through the ordinary recursive call. The loop keeps the order of the recursion: the innermost link, then each right operand from the bottom up. The receiver temporaries (_obj$N) follow that order and keep their names.Measurements (x64 Linux, release builds of
main13a98b0 and of this branch, one build directory)+,&&, comma,inchain that transpiles,transformSynctransform()(pool thread)@d #m{i}() {}members,transformSync/transform()bun build --minify-syntax, n plain statements in a method()linksMaximum call stack size exceeded, also at 2,000,000a?b:or tagged templates throughscan(), first depth that errorsconststatements, run withbun_in_exprfor a chain of n links_in_expr/_in_stmts/_in_binding.text#privateclasses, onetransformSync@d p() {}only, or withaccessor #xonlya + b,a + b + c, 48 linksqemu-x86_64 -one-insn-per-tb -d exec,nochain -dfilterover the symbols of the walk. Three calls log exactly three times one call. The wholetransformSyncof that fixture is about 13.1 M instructions.llvm-nm -Sandllvm-objdump -d.visit_expr_in_outis 17,694 and 17,555 B on both builds, and the stripped binaries have the same size.gdbbreakpoints on the release builds. Limits: bisection, one fresh process per probe.valgrind,perf,straceandbloatyare not installed here.Output unchanged. 145 source strings from
es-decorators.test.tsandes-decorators-esbuild.test.ts, plus 9 chains with rewritten accesses, through 4 transpiler configurations: 580 results. The two result files are byte-identical, soEXPECTED_VERSIONof the transpiler cache stays 34.Tests. Each one runs the transpiler in a child process, so a build without the fix fails with
signalCode: "SIGSEGV"and not with a dead test runner. The field-initializer and computed-key rows are the ones that do not pass through the entry for a statement list. The twoscan()probes are the ones that need!is_safe_to_recurse(): the undecorated source still compiles at those depths, so the reported flag is clear. Each probe fails when no depth reports the overflow. A debug build gets a shorter chain: its frames are larger and it is 20 times slower.The same defect, not in this PR.
Data::write_to_hasher(src/ast/expr.rs) under--react-fast-refresh: js_parser: include the hook binding in the React Fast Refresh signature #43750, the same shape.Expr::deep_clone: json, pnpm: build parsed trees in the arena of their owner and delete Expr::deep_clone #44222.P::is_dot_define_match(src/js_parser/p.rs:7260, adefinekey of 100,000 parts) andP::maybe_transpose_if_import(p.rs:1013,import(with 25,000 nested conditionals, plain code). Both run for ordinary files, so each gets its own change with its own numbers.Measured on x64 Linux only. Every number above is from that platform. Windows pool threads reserve 18 MB, so the depths in the tests are sized above their unfixed limit (about 35,700 links). Nothing in the change is platform specific.
[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file