module_loader: generate bun:main wrapper on demand instead of caching it - #32493
module_loader: generate bun:main wrapper on demand instead of caching it#32493robobun wants to merge 2 commits into
Conversation
ServerEntryPoint previously stored the generated wrapper source in a buffer that was written at reload_entry_point time and read back in getHardcodedModule when bun:main was fetched. Sentry BUN-36H7 (269 events, 100% standalone executable, same stack as #27192) shows getHardcodedModule faulting at page-aligned high addresses while reading that buffer, i.e. the stored slice had gone stale between the write and a later import("bun:main"). The wrapper is a pure function of vm.main and the watch flag, both of which are stable VM fields. Regenerate it on demand at fetch time and drop the stored contents buffer entirely so there is no cached slice that can dangle. The generated source is byte-identical to before. Adds a compiled-standalone-executable case to bun-main-entry-point.test.ts covering the crash scenario.
|
Updated 11:25 AM PT - Jun 18th, 2026
❌ @autofix-ci[bot], your commit 1e68d52 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32493That installs a local version of the PR into your bun-32493 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
More reviews will be available in 5 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
Changesbun:main on-demand source regeneration
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Closing this. After the find-issues bot linked #30613, the actual root cause is now clear and this PR does not address it. Actual root causeThe The Reproducible on any host with Why this PR does not helpWith the Actual fix#30642 probes simdutf at startup, fails fast with a clear diagnostic on hosts below the ISA floor, and recovers dispatch under Rosetta 2. That covers BUN-36H7, #30613, and #27192 (which #29450 also did not actually fix, since the BUN-36H7 should be deduped against #30613 and tracked against #30642. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -13,7 +13,11 @@ import { join } from "node:path"; | |||
| // free-then-reallocate on each reload. | |||
There was a problem hiding this comment.
🟡 The file-header comment here (lines 6-13) and the --hot reloads test comment (~lines 166-169) still describe bun:main as "backed by ServerEntryPoint.contents" with a "free-then-reallocate on each reload" — the exact mechanism this PR removes. Since this PR already edits this file and the whole point of the change is that there is no stored contents buffer anymore, these two comments should be updated in the same pass to describe the regenerate-on-fetch behavior.
Extended reasoning...
What's stale
Two comments in test/js/bun/resolve/bun-main-entry-point.test.ts describe the implementation that this PR deletes:
-
File header, lines 6-13:
bun:mainis backed by ServerEntryPoint.contents — a slice that is regenerated on every hot-reload cycle. Previously the backinglogger.Sourcedefaulted toundefined... These tests exercise the read path directly and the regenerate path under --hot so ASAN covers the new free-then-reallocate on each reload. -
--hot reloadstest, ~lines 166-169:Each reload calls ServerEntryPoint.generate() again, which now frees the previous
contentsbuffer before allocating a fresh one. Drive several reloads ... under ASAN this catches any use-after-free of the prior buffer.
Why they're wrong after this PR
Step through the new code:
ServerEntryPoint(src/bundler/entry_points.rs) now has onlypub generated: bool. Thecontents: Box<[u8]>field is gone.ServerEntryPoint::generate()is now a two-liner:entry.generated = true; Ok(()). It allocates nothing and frees nothing — there is no "free-then-reallocate on each reload".- The wrapper source is produced by the new
ServerEntryPoint::generate_source(is_hot_reload_enabled, path_to_use) -> Vec<u8>, a pure function called fromget_hardcoded_moduleat fetch time usingvm.main()andvm.is_watcher_enabled(). - So
bun:mainis no longer "backed by ServerEntryPoint.contents"; it is regenerated on demand, and there is no "prior buffer" for ASAN to catch a UAF on.
Both comments therefore describe a mechanism that no longer exists — and not incidentally: removing that mechanism is the entire point of this PR.
Why it's in scope for this PR
The PR already edits this exact file: it changes the imports, rewrites stripAsanWarning, and adds a new test directly between the header comment and the --hot test. The new test's own comment (lines ~90-99) even states the correct new behavior ("The wrapper source is now regenerated on demand from vm.main at fetch time, so there is no stored buffer that can go stale"), which now sits a few lines away from a header comment asserting the opposite. A future reader will see two contradictory descriptions in the same file.
Impact
No runtime effect — this is documentation only. But the stale comments are actively misleading: they point at a field (ServerEntryPoint.contents) and a lifecycle ("free-then-reallocate") that someone debugging bun:main will go looking for and not find. The --hot test's stated rationale ("under ASAN this catches any use-after-free of the prior buffer") no longer matches what the test actually exercises.
Suggested fix
Rewrite both comments to match the new model, e.g.:
- Header: "
bun:mainis a synthetic wrapper whose source is regenerated on demand fromvm.main()each time it is fetched (no cached buffer). These tests exercise the fetch path directly and across --hot reloads." --hottest: "Each reload re-fetchesbun:main, which regenerates the wrapper source from the currentvm.main(). Drive several reloads and verify it re-evaluates correctly each time."
| // Sentry BUN-36H7 / https://github.com/oven-sh/bun/issues/27192: | ||
| // import("bun:main") inside a compiled standalone executable faulted in | ||
| // getHardcodedModule reading the stored `entry_point.contents` slice | ||
| // (SEGV at a page-aligned high address, i.e. a freed mimalloc segment). | ||
| // The wrapper source is now regenerated on demand from `vm.main` at fetch | ||
| // time, so there is no stored buffer that can go stale between | ||
| // `reload_entry_point` and the fetch. This exercises the exact crash | ||
| // scenario: the bundled entry both (a) is reached via the initial | ||
| // bun:main fetch at boot and (b) explicitly re-imports bun:main after the | ||
| // top-level evaluation has completed. |
There was a problem hiding this comment.
🟡 nit: per repo convention (root CLAUDE.md), regression-test comments should be just the issue URL — the Sentry signature, freed-mimalloc diagnosis, and fix mechanism here all duplicate the PR description and will drift as the implementation changes. Suggest reducing this block to // https://github.com/oven-sh/bun/issues/27192 (and likewise dropping (Sentry BUN-36H7) from the new comment in jsc_hooks.rs).
Extended reasoning...
What
Root CLAUDE.md (line 241) states the repo convention for test comments explicitly:
Comments carry only durable non-obvious content … no bug history — that belongs in the PR description. Regression tests get exactly one comment: the issue URL.
The new comment block at test/js/bun/resolve/bun-main-entry-point.test.ts:90-99 is ten lines of bug history: the Sentry crash signature (BUN-36H7), the SEGV symptom ("page-aligned high address, i.e. a freed mimalloc segment"), the old mechanism (entry_point.contents read from getHardcodedModule), and how the fix works ("regenerated on demand from vm.main"). All of this is already in the PR description nearly verbatim — the "Cause" and "Fix" sections cover it word for word.
Why it matters
The rule exists because bug-history comments go stale. This one already references implementation internals that this very PR removes — entry_point.contents no longer exists after this change, and reload_entry_point no longer writes a buffer — so the comment is describing a code shape that's gone. The next time someone touches ServerEntryPoint they'll have to either update three copies of the same narrative (PR description, test comment, and the new (Sentry BUN-36H7) reference at src/runtime/jsc_hooks.rs:3440-3443) or let them drift. The issue URL alone is durable: it points to the canonical record and never needs editing.
The same file does have a pre-existing long bug-history comment at lines 6-13 (from #29450), so the author was following file-local precedent — but adding another instance compounds the problem rather than fixing it, and CLAUDE.md is unambiguous that this is a hard rule, not a suggestion.
Step-by-step
CLAUDE.md:241→ "Regression tests get exactly one comment: the issue URL."- New test added at
bun-main-entry-point.test.ts:100is a regression test for issue Segmentation fault at address 0x2813E000000 #27192 / Sentry BUN-36H7. - Comment at lines 90-99 contains: Sentry ID, SEGV diagnosis, freed-mimalloc explanation, old-vs-new implementation description, and test-scenario rationale — i.e. bug history, not the issue URL alone.
- Every sentence in that comment also appears in the PR description's "What" / "Cause" / "Fix" sections.
- ⇒ The comment violates the stated convention and duplicates the PR description.
Fix
Replace lines 90-99 with:
// https://github.com/oven-sh/bun/issues/27192Optionally also trim the new source comment at src/runtime/jsc_hooks.rs:3440-3443 to drop the (Sentry BUN-36H7) tag, since the issue link in the test already provides the back-reference and the PR description carries the Sentry ID.
| entry: &mut ServerEntryPoint, | ||
| is_hot_reload_enabled: bool, | ||
| path_to_use: &[u8], | ||
| _is_hot_reload_enabled: bool, | ||
| _path_to_use: &[u8], | ||
| ) -> Result<(), bun_core::Error> { | ||
| // Use the global arena so this buffer's lifetime is decoupled | ||
| // from whichever arena the caller's VM happens to be using; the | ||
| // slice is read later from `getHardcodedModule` which outlives any | ||
| // per-transpile arena. | ||
| entry.generated = true; | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 ServerEntryPoint::generate() now ignores both _is_hot_reload_enabled and _path_to_use and unconditionally returns Ok(()), leaving dead parameters and an infallible Result return type. Per CLAUDE.md ("Delete dead code in the same PR that makes it dead … parameters discarded in the body"), drop the dead params and change the return to (); the jsc_hooks.rs wrapper can then stop threading watch/entry_path and stop calling .is_ok() on a never-Err value (the RuntimeHooks fn-pointer signature can adapt in the wrapper if you'd rather not touch VirtualMachine.rs).
Extended reasoning...
What changed
Before this PR, ServerEntryPoint::generate() consumed is_hot_reload_enabled and path_to_use to render the wrapper source into entry.contents, and the write! calls could (nominally) fail, justifying Result<(), bun_core::Error>. After this PR the rendering moved to the new pure generate_source(), and generate() (src/bundler/entry_points.rs:267-275) is reduced to:
pub fn generate(
entry: &mut ServerEntryPoint,
_is_hot_reload_enabled: bool,
_path_to_use: &[u8],
) -> Result<(), bun_core::Error> {
entry.generated = true;
Ok(())
}Both non-self parameters are discarded (underscore-prefixed) and the return is unconditionally Ok(()).
Why it's flagged
CLAUDE.md:239 states this as required scope, not optional cleanup:
Delete dead code in the same PR that makes it dead (required scope — name the deletions in the description): … fields nothing reads, parameters discarded in the body, guards a new validator makes redundant.
This PR is exactly the change that made these parameters dead, so per the project's own contributing guide they should be removed here rather than left for a follow-up.
Dead plumbing in the caller chain
Walking outward from generate():
src/runtime/jsc_hooks.rs:608-616—generate_entry_point(_vm, watch, entry_path) -> boolthreadswatchandentry_paththrough solely to discard them, and calls.is_ok()on aResultthat can never beErr. The doc comment on this function ("Returnsfalseon error (the error is already logged intovm.logbygenerate)") is now also stale —generateno longer logs anything and never fails.src/jsc/VirtualMachine.rs:1639— theRuntimeHooks.generate_entry_pointfn-pointer signature carrieswatch: bool, entry_path: &[u8]and returnsbool.src/jsc/VirtualMachine.rs:2289-2291— the call site branches on thebooland has anErr(bun_core::err!("ServerEntryPointGenerate"))arm that is now unreachable (the only condition under which the hook returnsfalseisruntime_state().is_null(), which would already have prevented the VM from booting).
Step-by-step proof it's dead
generate()has exactly one caller:generate_entry_pointat jsc_hooks.rs:615 (verified by grep —ServerEntryPoint::generate(appears nowhere else).- That caller passes
watchandentry_pathstraight through; neither is read anywhere else in the wrapper. generate()'s body contains no?, noreturn Err, no fallible call — onlyentry.generated = true; Ok(()). So.is_ok()at jsc_hooks.rs:615 is staticallytrue.- Therefore
generate_entry_pointreturnstruewheneverruntime_state()is non-null, and thewatch/entry_patharguments contribute nothing to its result or side effects.
Impact
No runtime impact — this is purely dead surface area. But it leaves a misleading API: a reader of RuntimeHooks.generate_entry_point's signature and doc comment will assume entry-point generation can fail and depends on watch/entry_path, when in fact the real inputs are now read at fetch time in get_hardcoded_module via vm.main() / vm.is_watcher_enabled().
Suggested fix
Minimal (keeps the cross-crate RuntimeHooks vtable shape untouched):
impl ServerEntryPoint {
pub fn mark_generated(entry: &mut ServerEntryPoint) {
entry.generated = true;
}
// generate_source() unchanged
}and in jsc_hooks.rs:
fn generate_entry_point(_vm: &VirtualMachine, _watch: bool, _entry_path: &[u8]) -> bool {
let state = runtime_state();
if state.is_null() { return false; }
ServerEntryPoint::mark_generated(unsafe { &mut (*state).entry_point });
true
}Full cleanup (preferred, since RuntimeHooks is an in-tree Rust fn-pointer table with a single implementation, not a stable C ABI): change the vtable entry to fn(vm: &VirtualMachine), drop the watch/entry_path plumbing and the unreachable ServerEntryPointGenerate error branch at VirtualMachine.rs:2289-2291, and update the stale doc comments at jsc_hooks.rs:605-607 and VirtualMachine.rs:1636-1638.
1 participant
What
Sentry BUN-36H7: 269 events (100% Linux x86_64 baseline, 100% standalone executable) with this stack on 1.3.14:
Fault addresses are page-aligned high values (0x2C767000000, 0x5E4E3400000, ...), i.e. freed mimalloc segments. Same signature as #27192, which #29450 was supposed to close.
Cause
ServerEntryPointstored the generatedbun:mainwrapper source in a buffer (contents) that is written once inreload_entry_pointand read back wheneverbun:mainis fetched. The fetch can happen an arbitrary time later (ShadowRealm, userimport("bun:main"), module registry clear), and the fault shape says the stored slice had gone stale by then. I audited every writer of the struct and could not construct a deterministic reproduction; #29450 already eliminated theundefineddefault and moved the allocation tobun.default_allocator, so whatever is corruptingcontentsis not obvious from the invariants alone.Fix
Stop storing the wrapper. It is a pure function of
vm.main()andvm.is_watcher_enabled(), both of which are plain VM fields that are valid at fetch time.get_hardcoded_module(BunMain)now regenerates the source on demand from those andServerEntryPointkeeps only thegeneratedflag. The generated source is byte-identical to before, so auto-serve / hot-reload behaviour is unchanged; there is just no longer a cached allocation that can dangle betweenreload_entry_pointand a later fetch.Verification
bun bd test test/js/bun/resolve/bun-main-entry-point.test.ts(4 pass, including the new compiled-standalone case that doesimport("bun:main")from inside the bundled entry)bun bd test test/js/bun/http/bun-serve-html-entry.test.ts -t bun:mainbun bd test test/regression/issue/440.test.ts test/regression/issue/26142.test.ts(auto-serve wrapper behaviour unchanged)export default { port: 0, fetch: ... }still printsStarted ... server: http://...The crash itself is not deterministically reproducible (same as noted in #29450), so the new standalone-executable test exercises the fetch path and wrapper shape rather than the fault directly; the fix removes the state that was being read at the fault site.