bundler: stop an export star ambiguity that loops back into a re-export chain from overflowing the stack - #38984
Conversation
match_import_with_export traces every "export * from" alternative of a name with a recursive call, but the cycle check only looked at the trackers pushed by the current call. An alternative whose re-export chain led back into a chain that an outer call was still tracing was therefore traced again from scratch, recursing until the stack overflowed. Scan the whole cycle detector (as esbuild does), so such an alternative resolves to a cycle and the import is reported as ambiguous. Tracing alternatives still recurses once per nested alternative, so check the remaining stack before each recursive call and fail the build with an error instead of overflowing on a very long acyclic chain.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: reproduced and fixed.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes cycle-detection semantics in the bundler linker's import-matching loop, a human look would still be worthwhile to confirm the argument that scanning the whole cycle_detector cannot alter the outcome for any input that previously terminated.
What was reviewed:
- The one-line
cycle_detector[cycle_detector_top..]→cycle_detectorchange: verified the top-level caller clears the vec first (so top-level behavior is unchanged) and each recursive call truncates on return (so sequential alternatives don't see each other's entries — pinned by the newAlternativesShareReExporttest). - The new
MatchImportKind::StackOverflowvariant: checked both callers ofmatch_import_with_export(the recursive site and the top-level match at ~3891) handle it, and that thebreak 'loop_paths reach thetruncatebefore the early return. - Confirmed
StackCheck::init().is_safe_to_recurse()is the same pattern the parser/printer use.
Extended reasoning...
Overview
This PR fixes a stack overflow in LinkerContext::match_import_with_export (src/bundler/LinkerContext.rs) when an export * ambiguity chain loops back into a re-export that is already being traced. Two changes:
- The cycle check at the top of the loop now scans all of
self.cycle_detectorinstead ofself.cycle_detector[cycle_detector_top..], so a recursive call tracing an alternative sees the trackers its callers have already pushed. This matches esbuild'smatchImportWithExport. - A
bun_core::StackCheckguard before each recursive call converts a would-be overflow on very deep acyclic chains into a build error via a newMatchImportKind::StackOverflowvariant, propagated up through the recursion and reported the same way asCycle.
Four tests are added to test/bundler/bundler_edgecase.test.ts: two crash reproductions that now emit esbuild's ambiguity error, one negative test pinning that sequential alternatives sharing a re-export are not mistaken for a cycle, and a 600-barrel deep-chain test in a child process that accepts either a successful bundle (release) or the new stack-size error (debug+ASAN).
Security risks
None. This is bundler linker control flow with no auth, crypto, filesystem, or network surface. The only user-facing effect is that inputs which previously segfaulted now fail the build with an error message.
Level of scrutiny
Medium-high. The linker is a core code path and the cycle-detection change is a semantic change (which entries are compared), not a mechanical refactor. The PR argues convincingly — and I verified on the two example graphs — that any input where the new full-vec scan fires would have recursed forever under the old windowed scan, so no previously-working build changes outcome. That argument leans on the determinism of advance_import_tracker and on each frame's truncate-on-return, both of which I checked in the surrounding code. It also matches the esbuild reference. Still, this is the kind of subtle graph-traversal invariant a maintainer familiar with the linker should sign off on.
Other factors
- The exhaustive
match result.kindat the sole top-level call site handles the new variant; no other consumers ofMatchImportKindexist. - Both new
break 'loop_exits reachcycle_detector.truncate(cycle_detector_top)before the earlyStackOverflowreturn, so the detector stays balanced across recursion. self.cycle_detector.clear()before each top-level call meanscycle_detector_top == 0there, so the change is a no-op at the outermost frame and only affects recursive calls.- Tests follow harness conventions (
tempDir,bunEnv,await using, concurrent subprocess draining,signalCode: nullassertion) and sit next to the existingDeepImportChain/DeepImportDiamondDAGtests with the same 120s timeout. - The PR notes overlap with #31667 and #38966; whichever lands second takes a small conflict here.
Problem
bun buildsegfaults (panic(main thread): Segmentation fault; a debug build reportsAddressSanitizer: stack-overflowwithLinkerContext::match_import_with_exportcalling itself) on a barrel whoseexport *reaches a module that re-exports the same name from the barrel:Two barrels whose second
export *each lead to a re-export from the other barrel crash the same way.Cause: when a name reaches a file through two
export *statements,match_import_with_export(src/bundler/LinkerContext.rs) traces each alternative with a recursive call to itself. The cycle check at the top of its loop only scanned the trackers pushed by the current call (cycle_detector[cycle_detector_top..]), so the recursive call tracingb's re-export did not see that this same import was already being traced by its caller, followed it intobarrelagain, met the same alternative, and recursed until the stack was gone. esbuild'smatchImportWithExportscans its wholecycleDetector; the frame-local slice dates back to the Zig version of this function.The same recursion also grows by one frame per barrel on an acyclic chain of such alternatives, so a long enough chain overflowed the 2 MiB stack of the thread
Bun.build()links on (an unfixed debug build died at a little over 400 barrels, the released binary somewhere between 4000 and 5000).Fix
cycle_detector. Every call still truncates it back to its entry length when it returns (esbuild's save/restore of the detector around the recursive call), so the entries a call sees are exactly the chains currently being traced on the stack; alternatives traced one after another do not see each other's entries.Cycle, and the existing comparison of the alternatives reportsAmbiguous import "x" has multiple matching exports, which is what esbuild (0.21.5) prints for both graphs above. (A spec-following module loader would ignore the circular alternative and resolvextoa.x; esbuild chose to report the ambiguity instead, and this keeps Bun in line with it.)StackCheck(as the parser and printer do) and fail the build withMaximum call stack size exceeded while resolving import "x"instead of overflowing. A newMatchImportKind::StackOverflowcarries this out of the recursion: each call in between stops tracing and passes it up, and the outermost call reports it at the import being matched, the wayCycleis reported, so the build fails with that one error.ExportStarAmbiguityCycleIntoBarrelandExportStarAmbiguityCycleAcrossBarrels: the two graphs above, expecting the ambiguity error. Before the fix thebun buildchild is killed (SIGSEGV with the released 1.4.0 canary, ASAN stack-overflow with an unfixed debug build).ExportStarAmbiguityAlternativesShareReExport: two alternatives that pass through the same re-export one after another must not be taken for a cycle. Passes before and after; it pins the truncate-on-return behavior the fix relies on.ExportStarAmbiguityDeepReExportChain: a 600-barrel acyclic chain built withBun.build()in a child process. Unfixed debug build: the child dies. Fixed debug build: the new error. Release builds, where 600 barrels fit on the stack, bundle it; the test accepts either outcome and requires the child to exit normally. Its fail-before is therefore debug-only; the released binary bundles a chain of this length. It takes about 9 to 11 s here under debug+ASAN, almost all of it parsing the 1200 files, hence the explicit timeout (same as the neighbouring deep-chain tests).Background
import {x}orexport {x} fromstatement;advance_import_trackerfollows it one step to the export it names in the target file, andmatch_import_with_exportloops on that until it reaches a real symbol (or a CommonJS file, a missing export, etc.).resolved_exports[file][name]is computed before imports are matched. When a name reaches a file through severalexport *statements that point at different symbols, the first is stored as the export and the rest are attached to it aspotentially_ambiguous_export_star_refs. While matching an import, every alternative is traced to its final symbol and the import is ambiguous unless they all agree (export * from "./c"next toexport { x } from "./c"is the legitimate case this exists for, covered by the existingReExportStarNameCollisionNotAmbiguousImporttest). Tracing an alternative that is itself a re-export is where the recursion comes from.cycle_detectoris aVecof the trackers on the chains currently being followed. Each call notes its length on entry, pushes the trackers it visits, and truncates back to the noted length when it returns.StackCheckcompares the stack pointer with the stack bounds recorded when the thread started and reports whether more than 128 KiB (256 KiB on Windows) remain.bun buildlinks on the main thread;Bun.build()links on a dedicated thread with the default 2 MiB stack.Measurements (chain of N barrels, x resolving through all of them)
Released 1.4.0 canary (unfixed),
Bun.build()on the bundle thread: N = 4000 bundles, N = 5000 segfaults.bun buildon the main thread bundles N = 20000.Unfixed debug+ASAN build,
Bun.build(): N = 400 bundles, N = 425 dies.Fixed debug+ASAN build,
Bun.build(): N = 375 bundles, N = 400 fails withMaximum call stack size exceeded while resolving import "x"(the guard keeps 128 KiB in reserve, about 25 frames of this function in a debug build).esbuild 0.21.5 on the first graph in the description:
Bun stops after the first file with an import error, so it reports the
b.jsone.