Repository navigation
Conversation
…eys in a loop The walk that turns import(a ? b : c) into a ? import(b) : import(c), and its copies for require() and require.resolve(), recursed once for each link of a conditional chain with no stack check. So did conditional_namespace_records and is_dot_define_match. They run after the visit pass, on the stack that it left, so a long chain or a long define key ended the process with a SIGSEGV. transpose_each_branch replaces the five transposers. It loops along `no` and recurses only into `yes`, behind the stack check of the visit pass. On an overflow it returns None and the caller keeps the call as it is. conditional_namespace_records takes the same loop and check. is_dot_define_match peels one key part for each member access in a loop.
|
Status
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe parser now traverses conditional chains iteratively in transposition and namespace-record collection, and traverses define-key accessors iteratively. Transposition helpers return optional expressions, and the visitor preserves the original expression when transposition fails. Regression tests cover conditional expressions and deep inputs. ChangesParser stack safety
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The stack-safety changes appear mergeable 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.
I reviewed this PR and didn't find any bugs. Because it restructures the parser's conditional-transposer and define-matcher walks and adds stack-depth-searching subprocess tests, a human look at the test cost on debug/ASAN lanes would still be worthwhile.
What was reviewed:
transpose_each_branchagainst the five deleted transposers: leaf call order (yes-subtree, then no) and node construction are unchanged; the stack guard matches the existing pattern at p.rs:2922 and scan_side_effects.rs:128.- The
Nonepath at the three visit_expr.rs call sites: the untouchedimport()/require()call is a shape the base already emits for non-string arguments, andreport_stack_overflowfails the parse. is_dot_define_matchloop: traced each base arm (shortpartson EDot/EIndex, non-utf8 index string, EImportMeta, EIdentifier) and each still reaches the same result.- No remaining callers of the deleted
transpose_known_to_be_if_*helpers.
Extended reasoning...
The change touches the JS parser's import()/require()/require.resolve() conditional transposer, the bundler's conditional namespace-record walk, and the define-key matcher in src/js_parser/p.rs, plus three call sites in visit_expr.rs and ~250 lines of new tests. It touches no security-sensitive surface; it converts unbounded recursion into loops with the parser's existing stack guard, fixing SIGSEGVs on adversarial input. I found the refactor behavior-preserving on every path I traced and the deleted helpers have no stale callers. I deferred rather than approved because the change is non-trivial parser-core code and the new tests binary-search stack limits in spawned subprocesses, where runtime on debug/ASAN lanes (one of the search tests is not skipped on debug) is best judged by a maintainer.
This branch has not been deployed
2 participants
Problem
import(),require()orrequire.resolve()ends the process withSegmentation faultand no message. So does adefinekey of 54,000 parts.require()anddefinecases. Bun 1.3.13 handles all.src/js_parser/p.rsrecurse per link with no stack check:maybe_transpose_if_importand four copies (:1059),conditional_namespace_records(:1356),is_dot_define_match(:7342).Fix
transpose_each_branchreplaces the five transposers. It loops alongnoand recurses intoyesbehind the stack check. On overflow it returnsNoneand the call stays.conditional_namespace_recordstakes the same loop and check.is_dot_define_matchbecomes a loop.no, where a loop takes no stack. Inyesthe visit pass recurses first, in a 640-byte frame against 176.test/bundler/transpiler/transpiler.test.js, 6 new tests, 5 fail on main. Self-reviewed: 11 concerns raised, 8 addressed, 3 moved to the highlighter's own PR.Background
import(a ? b : c)intoa ? import(b) : import(c), so the bundler sees a string in each branch.Maximum call stack size exceededand the parse fails.Downsides
require(…)at the first depth that the visit pass refuses still aborts (panic: index out of bounds), as on main. js_parser: replace skipped expressions with E::Missing and skip argument checks after a stack overflow #44467 has the fix.import("a")costs 25 more parser instructions (of 3,083)..textgrows 3,328 bytes. No allocation is added.Notes
Found by a fuzz ledger and a read of the code. There is no issue and no user report. No real input comes near these depths.
Repro (release builds, Linux x64, 8 MB stack. "died" is SIGSEGV with nothing on stderr.)
x = 1;x = 1;"reports" is
Maximum call stack size exceeded.conditional_namespace_recordsneeds the bundler:Bun.buildofconst ns = c ? require("./a") : … : nullwith a long chain inside a deep nest of calls dies on main on a bundler thread.Why only some depths died. The parser recurses once per link of a chain, 304 bytes a link, and asks the stack check. The visit pass takes no stack along
no:e_ifvisitsnolast, and this toolchain compiles that call as a jump. So a chain can be as long as the parser takes. The walks then recursed over it with no check:maybe_transpose_if_importtook 336 bytes a link, more than the parser. A chain alone ran it out of stack from 24,852 links. Bun 1.4.2 was compiled with recursion fornoin the visit pass, which reported first.requirewalks took 128 and 144 bytes a link. They ran out when the call was deep in a nest, where the visit pass had left little stack. On 1.4.2 the visit pass reported the depth, ande_callstill handed the whole chain to the walk.is_dot_define_matchtook 160 bytes a part (208 with TypeScript), and no other pass bounds adefinekey.Per function
transpose_each_branch(replacesmaybe_transpose_if_import,maybe_transpose_if_require,transpose_known_to_be_if_require,maybe_transpose_if_require_resolve,transpose_known_to_be_if_require_resolve)no, recursion intoyesbehind the stack checkNone. The caller leaves theimport()orrequire()call in place, and the parse failsconditional_namespace_recordsNone, which its caller already takes as "leave the namespace escaped"is_dot_define_matchThe check in the walker is the usual pair,
!stack_check.is_safe_to_recurse() || reported_stack_overflow.get(). The second half is what stops the walk after the visit pass gave up inside the argument. Without it each branch that was not visited logs an error of its own ("This require() expression will not be bundled because the argument is not a string literal").Measured (release builds of the merge base e655c58 and of this branch, Linux x64)
nolinkimport(352 with TypeScript), 128require, 144require.resolveyeslevelconditional_namespace_records, bytes anolinkx=import(chain)throughscan(), 3% stepsf(chain)transform()on a pool threadrequire(chain)in 12,455 nested callsimportfrom 2,000)f(chain)givesf,import,requireandrequire.resolvealikescan,transformSync,transform,Bun.build,import(),require(),Worker,bun FILE,bun test,bun build)definekey that matchesx=import("a")x=require("a")x=require.resolve("a")x=import(c?"a":"b")x=require(c?"a":"b")x=import(c?"a":d?"b":e?"f":"g")x=f("a"), the controlis_dot_define_matchfor one matcheda.b.c.dimport("a"),import(c?"a":"b"), the 3-link chain.textqemu-x86_64 -one-insn-per-tb -d exec,nochain -dfilterover the address range of the parser crate, for 1,000 and 2,000 identical statements throughtransformSync. The number is the difference divided by 1,000.llvm-nm -Sandllvm-objdump -don the unstripped binaries. Both self-calls in the compiled walker are theyescall. The walker tests theEIftag first, so an argument that is not a conditional runs no check. The +25 forimport("a")is one more call level: main inlined the leaf into the recursive function.ulimit -s 8192.Output unchanged. 153 fixtures under
test/bundler,test/js/bun/transpilerandtest/js/webthat containimport(orrequire(, through 4 transpiler configurations each: 13,691,165 bytes of printed output and import scans, byte-identical between the two builds. 19,200 random conditional trees inimport(),require(),require.resolve()andawait import()(8 seeds, 8 option sets): 20,531,605 bytes, byte-identical. 88,332 random member expressions against 18,000 randomdefinekeys (dot, index, optional chain,import.meta): byte-identical.Tests
import() and require() of a conditional move into each brancha long conditional chain in import() does not crash the processa conditional chain in import() or require() in a deep nest does not crash the processBun.build logs only the stack overflow when the argument of require() is too deepBun.build takes a conditional chain inside a deep nesta define key of many parts is matched instead of crashing the processf(chain)gives.require(…)at the exact first depth that the visit pass refuses. See Downsides.Still open, not in this PR
e_call, the directevalcheck).require(…)at the first refused depth aborts withpanic: index out of bounds: the len is 7 but the index is 7. js_parser: replace skipped expressions with E::Missing and skip argument checks after a stack overflow #44467.rewrite_private_accesses_in_*(decorated#privatemembers): js_parser: walk binary chains on the heap and check the stack when rewriting decorated #private accesses #44662.Expr::deep_clonefor a deepdefinevalue: json, pnpm: build parsed trees in the arena of their owner and delete Expr::deep_clone #44222. The React compiler passes: react-compiler: check the stack in every pass that recurses as deep as the source nests #43230.Binding::to_expr_innerandP::convert_expr_to_bindinghave no stack check. Handed to their own change.Bun.markdown.ansi(`${nesting): its own change.definekey that almost matches a long member chain is still quadratic in time (the matcher runs at every link). That cost is on main too.#28693 and #40857 edit other arms of
is_dot_define_matchand keep its recursion. Their edits apply to the loop.Suites on the release build:
transpiler.test.js243 pass,bundler_dynamic_import_dce333,esbuild/default155,esbuild/extra222,bundler_splitting200,bundler_edgecase182,esbuild/dce87,esbuild/importstar99,esbuild/ts59,bundler_jsx64,esbuild/splitting26,esbuild/importstar_ts23,bundler_bun12,bundler_env7. On the debug + ASAN build:transpiler.test.js241 pass, 3 skip.