Skip to content

bundler: frame .jsc bytecode sidecars and verify them before decoding - #39396

Open
Jarred-Sumner wants to merge 2 commits into
mainfrom
claude/ledger-13066-bytecode-sidecar-validate
Open

bundler: frame .jsc bytecode sidecars and verify them before decoding#39396
Jarred-Sumner wants to merge 2 commits into
mainfrom
claude/ledger-13066-bytecode-sidecar-validate

Conversation

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

What

bun build --bytecode --target=bun --outdir writes <chunk>.js + <chunk>.js.jsc. At load time the sidecar was handed straight to JSC's cached-bytecode decoder, which decodes in place and trusts the buffer completely (only the JSC cache version / boot UUID / SourceCodeKey are checked — no length or checksum, by design, since JSC assumes it wrote the file). A truncated or single-byte-corrupted .jsc therefore gave a heap-buffer-overflow READ in the decoder and a heap WRITE in CodeBlock::finishCreation (ASan), SEGV/abort on release.

The sidecar now ends with a 24-byte footer — payload_len u64 LE | wyhash(payload) u64 LE | "\0bun.jsc" — written by both output paths (writeOutputFilesToDisk and the in-memory/Bun.build path in generateChunksInParallel), and the single sidecar reader in bundler/transpiler.rs verifies it before truncating to the payload and handing it to JSC; on any mismatch the sidecar is ignored and the source is parsed. A footer rather than a header keeps the payload at offset 0 (JSC needs the allocation's alignment; no copy on load). Bytecode embedded in --compile executables and the NODE_COMPILE_CACHE blobs are unchanged (the former is aligned inside the StandaloneModuleGraph, the latter already has its own sha256 header). Unframed .jsc files from older builds are ignored (source is parsed), which is the safe outcome and what a JSC version bump does anyway.

Repro (before)

bun build --bytecode --target=bun app.ts --outdir out && bun out/app.js     # ok, [Disk Cache] hit
cp -r out t1 && python3 -c "b=bytearray(open('t1/app.js.jsc','rb').read()); b[1152]^=0xFF; open('t1/app.js.jsc','wb').write(b)" && bun t1/app.js   # SEGV / ASan heap-buffer-overflow WRITE
cp -r out t2 && truncate -s 32 t2/app.js.jsc && bun t2/app.js               # abort / ASan heap-buffer-overflow READ

Tests

test/bundler/bun-build-api.test.ts — "a corrupted or truncated .jsc sidecar is ignored instead of decoded" (pristine → cache hit; truncated-to-32, truncated-by-1, one flipped byte, header smashed, trailing garbage → correct output, no cache hit, exit 0). Fails on the ASan canary and debug main; passes here. Existing --bytecode tests in bun-build-api, bundler_bun, bundler_compile exercise the framed and raw paths.

JSC decodes cached bytecode in place and trusts it completely, so a
truncated or corrupted <chunk>.jsc next to a --bytecode build was handed
straight to the decoder (heap overflow read/write). The sidecar now ends
with a 24-byte footer (payload length, wyhash of the payload, magic);
the loader verifies it and otherwise ignores the sidecar and parses the
source. Bytecode embedded in --compile executables is unchanged.
@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 16 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 71 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1eb5bd6a-6b75-4f35-9f77-2f51b7835c88

📥 Commits

Reviewing files that changed from the base of the PR and between fea1829 and 50170d0.

📒 Files selected for processing (5)
  • src/bundler/lib.rs
  • src/bundler/linker_context/generateChunksInParallel.rs
  • src/bundler/linker_context/writeOutputFilesToDisk.rs
  • src/bundler/transpiler.rs
  • test/bundler/bun-build-api.test.ts

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline nits, I checked that transpiler.rs is the only .jsc sidecar reader — jsc_hooks.rs and RuntimeTranspilerStore.rs both consume parse_result.already_bundled downstream of it, so the single verification site covers both load paths. Also confirmed write_output_files_to_disk is gated on !is_compile in generateChunksInParallel.rs, so its unconditional frame() (no is_executable() branch) is correct.

Extended reasoning...

This PR fixes a memory-safety issue (ASan heap-buffer-overflow READ/WRITE → SEGV) by adding integrity framing to on-disk bytecode sidecars. The design and implementation look correct — footer keeps payload alignment at offset 0, checked_sub guards short files, verification fails closed to source-parse — and the test covers pristine + five corruption variants. Given it introduces a new on-disk binary format on a security-sensitive load path, a maintainer should still sign off; the two inline findings are style nits only.

Comment thread test/bundler/bun-build-api.test.ts Outdated
Comment thread src/bundler/lib.rs
@robobun

robobun commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator
Updated 9:39 PM PT - Aug 16th, 2026

@Jarred-Sumner, your commit 50170d08eaa233417c6606bf83c18d194445550e passed in Build #99843! 🎉


🧪   To try this PR locally:

bunx bun-pr 39396

That installs a local version of the PR into your bun-39396 executable, so you can run:

bun-39396 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — both earlier nits (async spawn in the test, shared bytecode_sidecar::EXTENSION in the two linker files) are addressed in 50170d0.

What was reviewed:

  • frame()/payload_len() are symmetric; checked_sub(FOOTER_LEN) handles <24-byte files, magic/len/wyhash checked before the payload reaches JSC, and any mismatch falls back to parsing source (fail-safe).
  • writeOutputFilesToDisk is only reached when !is_compile, so its unconditional frame() is correct; the in-memory path in generateChunksInParallel correctly skips framing for --compile embeds.
  • contents.truncate(payload_len) keeps the payload at offset 0 of the allocation, preserving JSC's alignment expectation as described.
  • Test asserts the pristine sidecar round-trips ([Disk Cache] Cache hit) before exercising the five corruption variants, so the framing itself is proven live.
Extended reasoning...

Overview

Adds a 24-byte footer (payload_len u64 LE | wyhash(payload) u64 LE | "\0bun.jsc") to .jsc bytecode sidecars produced by bun build --bytecode, and verifies it in the single sidecar reader (src/bundler/transpiler.rs) before handing bytes to JSC's cached-bytecode decoder. Touches src/bundler/lib.rs (new bytecode_sidecar module), the two linker output paths, the transpiler load path, and adds a test in bun-build-api.test.ts.

Security risks

The motivating bug is a heap-buffer-overflow (READ and WRITE, per ASan) when JSC decodes a truncated/corrupted sidecar. This PR adds an integrity gate in front of that decoder. The gate is fail-safe: any footer mismatch (or file shorter than the footer) returns None and the module is parsed from source — the same outcome as no sidecar at all. wyhash is not cryptographic, but this is a build-artifact integrity check against corruption/truncation, not an authentication boundary; an attacker who can write app.js.jsc can also write app.js. --compile embeds and NODE_COMPILE_CACHE are intentionally untouched.

Level of scrutiny

Medium. It introduces a small on-disk format and touches the bytecode load path, but the change is ~40 lines of production logic per side, the write/read pair is symmetric and colocated in one module, and the failure mode of any verifier bug is benign (parse from source). The footer-not-header choice is justified (payload stays at allocation offset 0 for JSC alignment). Unframed sidecars from older builds are ignored, which is the same effect as a JSC cache-version bump.

Other factors

I left two nit-level comments on the previous revision (spawnSync → async Bun.spawn, and consolidating the duplicate ".jsc" constants onto bytecode_sidecar::EXTENSION); commit 50170d0 addresses both. This run of the bug-hunting system found no issues. The new test exercises the full round-trip (pristine → cache hit) plus five corruption variants (two truncations, one bit-flip, header smash, trailing garbage), each asserting correct stdout, no cache hit, and exit 0. I verified write_output_files_to_disk is gated on !is_compile in its sole caller, so its unconditional frame() cannot leak a framed payload into a compiled executable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants