Conversation
The PE writer emits the .bun section with IMAGE_SCN_MEM_READ only (no write bit), and on Windows the payload is read straight out of the loaded image. StandaloneModuleGraph claimed JSC mutates the bytecode buffer in place and therefore carried *mut [u8] write-provenance end to end, while ELF and Mach-O map the payload RW. Verified on Windows that a --compile --bytecode executable runs to completion with the .bun section at PAGE_READONLY (VirtualQuery reports Protect=0x2), for both --format=cjs and --format=esm. Any in-place write would have faulted with STATUS_ACCESS_VIOLATION; none does. On the JSC side CachedBytecode::span() returns std::span<const uint8_t>, and Decoder caches decoded pointers in a side HashMap (m_offsetToPtrMap), never in the buffer. Bun's SourceProvider::updateCache / cacheBytecode / commitCachedBytecode are early-return no-ops. The mutation claim first appeared in the Rust rewrite; it was never in the Zig source and was never observed behavior. This change: - File.bytecode / File.module_info: *mut [u8] -> &'static [u8] - StandaloneModuleGraph.bytes: *const [u8] -> &'static [u8] - get_data() on all three platforms: *mut u8 -> *const u8 - ResolvedSource.bytecode_cache: *mut u8 -> *const u8 (Rust and C++) - Drops slice_to_mut and the write-provenance commentary - Rewrites the ELF alignment comment: the payload is PT_LOAD-mapped with the same 8-byte length prefix as PE/Mach-O (not heap-copied), so target_mod=120 is required on ELF for the same reason, not incidental overhead Mach-O PROT::WRITE and the ELF RW PT_LOAD extension are left as-is; they are not needed for this reason but changing segment permissions is a separate concern.
|
Updated 3:18 AM PT - Jul 20th, 2026
❌ @robobun, your commit 72c3aa6 has some failures in 🧪 To try this PR locally: bunx bun-pr 34801That installs a local version of the PR into your bun-34801 --bun |
|
Warning Review limit reached
Next review available in: 2 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 (7)
Comment |
Encodes the invariant the StandaloneModuleGraph read-only refactor depends on: the PE writer marks .bun without IMAGE_SCN_MEM_WRITE, so Windows maps it PAGE_READONLY, and a --compile --bytecode binary must still run to completion against those pages (i.e. the JSC decoder never writes the embedded bytecode).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked: the rewritten ELF alignment comment is accurate (elf.rs appends the payload at a page-aligned vaddr with the same 8-byte u64 prefix as PE/Mach-O — the old "heap-allocated buffer" claim was stale); bytecode_cache has no other C++ consumers, and the destructorPtr free path is unaffected (standalone uses needsDeref=false → no-op, and for the heap path .cast_const() doesn't strip deallocation provenance); and File.bytecode/.module_info have no other Rust consumers that need write access.
Extended reasoning...
Verified the factual claims underpinning the const-ification: the ELF writer (src/exe_format/elf.rs:211-358) PT_LOAD-maps the payload at a page-aligned vaddr with an 8-byte length prefix, so the rewritten target_mod = 120 comment is correct for all three platforms and the old "heap-allocated" text was wrong. Grep confirmed bytecode_cache is only read in ZigSourceProvider.cpp and file.bytecode/file.module_info only in jsc_hooks.rs (both read-only after this change). The const_cast at CachedBytecode::create is scoped and commented; updateCache/cacheBytecode/commitCachedBytecode remain early-return no-ops so no write path is reachable. Deferring to a human for sign-off on the core "JSC never mutates the bytecode span" claim since it governs unsafe provenance reasoning across the FFI boundary — the Windows PAGE_READONLY evidence is compelling but a maintainer should confirm.
There was a problem hiding this comment.
Thanks for addressing the stderr assertion — the combined { stdout, exitCode } check in 72c3aa6 looks good. No further issues found, but deferring to a human on this one: it rewrites unsafe-Rust provenance invariants in StandaloneModuleGraph and adds a const_cast justified by a reading of JSC's CachedBytecode internals, which a maintainer should sign off on.
Checked: all file.bytecode/file.module_info consumers in jsc_hooks.rs now go through .as_ptr()/&[u8] cleanly; from_bytes debug round-trip still type-checks (*mut u8 → *const u8 coercion); bytecode_cache has no other C++ write sites; the hint_source_pages_dont_need MADV_DONTNEED path is unaffected (file-backed pages fault back with identical bytes).
Extended reasoning...
Overview
This PR removes the "JSC mutates the bytecode buffer in place" claim from StandaloneModuleGraph and the *mut [u8] write-provenance plumbing it justified. Concretely: File.bytecode/File.module_info become &'static [u8], get_data() on all three platforms returns *const u8, ResolvedSource.bytecode_cache becomes *const u8 (Rust + headers-handwritten.h), slice_to_mut and ~79 lines of Stacked-Borrows commentary are deleted, and a const_cast is added at the CachedBytecode::create boundary in ZigSourceProvider.cpp. The ELF alignment comment is also rewritten to state the payload is PT_LOAD-mapped with the same 8-byte prefix as PE/Mach-O. A Windows-only regression test builds a --compile --bytecode exe, asserts .bun has no IMAGE_SCN_MEM_WRITE, and runs it to completion.
Security risks
None identified. The change tightens types (mutable → const) rather than loosening them; no new untrusted-input parsing, no auth/crypto surface. The const_cast in C++ is at a boundary where JSC's own span() accessor returns span<const uint8_t>, so it does not enable writes that weren't already possible.
Level of scrutiny
High. This is exactly the category REVIEW.md flags as most-blocked: unsafe Rust memory-provenance reasoning across an FFI boundary. The PR deletes deliberate write-provenance commentary added in #30412, and REVIEW.md is explicit that "before deleting odd-looking code, git-blame why it was written — it is usually load-bearing." The author has done that homework thoroughly (empirical PAGE_READONLY verification on Windows, JSC source citations for CachedBytecode::span()/Decoder::ptrForOffsetFromBase(), confirmation that Bun's updateCache/cacheBytecode/commitCachedBytecode are early-return no-ops), and the argument is compelling — but a maintainer who owns this code path should confirm the JSC-never-writes claim and the rewritten ELF alignment rationale.
Other factors
- My earlier inline nit (exact-empty stderr assertion) was addressed in 72c3aa6; the test now uses a combined
{ stdout, exitCode }assertion and drains stderr without asserting on it. - Verified no other consumers of
file.bytecode/file.module_infobeyondjsc_hooks.rs(which the diff updates), and no other C++ sites readbytecode_cacheas non-const. - The
uint8_t*→const uint8_t*change inheaders-handwritten.his ABI-identical. - The debug-assertions round-trip in
to_bytesstill compiles (string_builder.ptr.unwrap().as_ptr()coerces*mut u8→*const u8). - The new Windows test cannot be run on this machine; CI is the source of truth for it.
|
CI build #76182: 284 jobs passed. The two red jobs are both The new Windows-only test in Diff is ready for review. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-20, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
Removes the "JSC mutates the bytecode buffer in place" claim and the
*mut [u8]write-provenance plumbing it justified inStandaloneModuleGraph. Also corrects the stale ELF alignment comment.Why
There was a contradiction in the
--compilecode: the PE writer emits.bunasIMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READonly (src/exe_format/pe.rs:647), and on Windows the payload is read straight out of the loaded image (src/jsc/bindings/c-bindings.cpp:1123-1130). YetStandaloneModuleGraphinsisted JSC mutates the buffer in place and carried*mut [u8]write-provenance end to end, while ELF/Mach-O map RW. Only one could be right.Verification
Built a
--compile --bytecodeexecutable on Windows (released bun 1.4.0-canary) and ran it:The section is
PAGE_READONLYat runtime, bytecode is embedded (28 KB with--bytecodevs 3 KB without), and the program runs to completion (exit 0) for both--format=cjsand--format=esm. Any in-place write would have faulted with0xC0000005; none does.From JSC source:
CachedBytecode::span()returnsstd::span<const uint8_t>(vendor/WebKit/Source/JavaScriptCore/runtime/CachedBytecode.h:72)Decoder::ptrForOffsetFromBase()returnsconst void*; decoded pointers are cached in a sidem_offsetToPtrMapHashMap (CachedTypes.h:94-108), never written into the bufferSourceProvider::updateCache/cacheBytecode/commitCachedBytecodeare early-return;no-ops (ZigSourceProvider.cpp:294-322)module_infois consumed viacreate_from_cached_record(&*file.module_info)which only reads and copies outThe mutation claim first appeared in the Rust rewrite (#30412); it was never in the Zig source and was never observed behavior.
Changes
File.bytecode/File.module_info:*mut [u8]->&'static [u8]StandaloneModuleGraph.bytes:*const [u8]->&'static [u8]get_data()(macho/pe/elf):*mut u8->*const u8ResolvedSource.bytecode_cache:*mut u8->*const u8(Rust +headers-handwritten.h)ZigSourceProvider.cpp:const_castat theCachedBytecode::createboundary (its signature takesspan<uint8_t>but the payload is only ever exposed asspan<const>)slice_to_mutand the write-provenance commentary (net -79 lines)u64length prefix as PE/Mach-O (verified via/proc/self/maps+readelf -l), sotarget_mod = 120is required on ELF for the same reason, not incidental overheadMach-O
PROT::WRITE(macho.rs:142-143) and the ELF RW PT_LOAD extension (elf.rs:402-424) are left as-is; they are not needed for this reason but changing segment permissions is a separate concern.Tests
bun bd test test/bundler/bundler_compile.test.ts -t Bytecode: 16/16 bytecode tests pass.cargo checkclean on all targets (linux/macos/windows x x64/aarch64).no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/regression/issue/pe-codesigning-integrity.test.ts