node:vm: reject invalid cachedData instead of crashing - #32839
Conversation
|
Updated 5:10 PM PT - Aug 9th, 2026
❌ @robobun, your commit 8507750 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32839That installs a local version of the PR into your bun-32839 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughNodeVM cached-data serialization now uses a validated header. Script, function, and module readers validate the header before decoding. Writers emit the wrapped format. Tests cover corrupted, mismatched, and intact cached data. ChangesCached-data hardening
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/NodeVM.cpp`:
- Around line 144-179: The cached-data path in unwrapCachedData and
decodeCachedData is relying on a forgeable hash in CachedDataHeader, so
untrusted input can still reach decodeCodeBlock. Replace the public
checksum-only check in createCachedDataBuffer/hashCachedDataPayload with an
unforgeable validation approach (for example, a secret-backed tag or structural
validation) and ensure decodeCachedData rejects any payload that is not verified
before decoding. Keep the fix localized around CachedDataHeader,
unwrapCachedData, and decodeCachedData so malformed cachedData cannot proceed
past validation.
- Around line 312-315: The `NodeVM::createCachedDataBuffer()` result can be null
on allocation failure, but the current `function->putDirect()` path still stores
it and sets `cachedDataProduced` to true. In the `NodeVM.cpp` bytecode caching
flow, add a null check immediately after calling `createCachedDataBuffer()` and
before the `putDirect()` calls, mirroring the existing
`RETURN_IF_EXCEPTION`-style guarded paths. If the buffer is null, skip setting
`cachedData` and ensure `cachedDataProduced` is not marked successful.
In `@src/jsc/bindings/NodeVMSourceTextModule.cpp`:
- Around line 510-514: The cached-data path in
NodeVMSourceTextModule::cachedData() needs null checks for both bytecode() and
createCachedDataBuffer(). After calling bytecode(globalObject), verify the
RefPtr is non-null before using cachedBytecode->span(), and similarly verify the
JSUint8Array* returned by createCachedDataBuffer() before storing it in
m_cachedBytecodeBuffer. Keep the existing RETURN_IF_EXCEPTION checks, but add
early returns for the nullptr cases so the function never dereferences or caches
a missing bytecode/buffer.
In `@test/js/node/vm/vm.test.ts`:
- Around line 814-821: The crash-regression assertion in the `vm.test.ts` spawn
test is only checking `stdout` and `exitCode`, which hides useful failure
diagnostics when the child aborts. Update the `Bun.spawn` result handling in
this test to include `stderr` and `signalCode` in the asserted object, using the
existing `stderr` capture from `proc.stderr.text()` so regressions surface
native crash output and termination signals in the diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cdcdd3d-5b22-4daf-86b5-f6ab81bb7043
📒 Files selected for processing (5)
src/jsc/bindings/NodeVM.cppsrc/jsc/bindings/NodeVM.hsrc/jsc/bindings/NodeVMScript.cppsrc/jsc/bindings/NodeVMSourceTextModule.cpptest/js/node/vm/vm.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Addressed the review feedback in f28e900:
Two suggestions were not taken, with reasoning in the threads: the header is an integrity check rather than an authentication boundary (the same contract V8 uses for cachedData, and it must stay a public deterministic function of the bytes to keep cross-process reuse working), and createCachedDataBuffer() only returns null with a pending exception, which the RETURN_IF_EXCEPTION after each call already handles. test/js/node/vm/vm.test.ts and the Node vm cachedData tests pass locally with the debug (ASAN) build. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/jsc/bindings/NodeVMSourceTextModule.cpp (1)
514-520: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBuffer null check from the prior review is still missing.
The new guard handles
cachedBytecode == nullptr, butcreateCachedDataBuffer()can also returnnullptr(its[[unlikely]]allocation-failure branch) without throwing. Line 520 then caches a null buffer intom_cachedBytecodeBufferand line 523 returns it. Guard the buffer too, matchingNodeVM::createCachedData.Proposed fix
JSUint8Array* buffer = createCachedDataBuffer(globalObject, cachedBytecode->span()); RETURN_IF_EXCEPTION(scope, nullptr); + if (!buffer) [[unlikely]] { + throwVMError(globalObject, scope, "createCachedData failed"_s); + return nullptr; + } m_cachedBytecodeBuffer.set(vm, this, buffer);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/NodeVMSourceTextModule.cpp` around lines 514 - 520, The cached bytecode path in NodeVMSourceTextModule::createCachedData still misses the null check for the JSUint8Array returned by createCachedDataBuffer(). Add a guard after creating the buffer, before m_cachedBytecodeBuffer.set, and mirror the failure handling used in NodeVM::createCachedData by throwing a VM error and returning nullptr if the buffer allocation fails. Keep the existing cachedBytecode null handling unchanged and ensure the returned value is only used when the buffer is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/jsc/bindings/NodeVMSourceTextModule.cpp`:
- Around line 514-520: The cached bytecode path in
NodeVMSourceTextModule::createCachedData still misses the null check for the
JSUint8Array returned by createCachedDataBuffer(). Add a guard after creating
the buffer, before m_cachedBytecodeBuffer.set, and mirror the failure handling
used in NodeVM::createCachedData by throwing a VM error and returning nullptr if
the buffer allocation fails. Keep the existing cachedBytecode null handling
unchanged and ensure the returned value is only used when the buffer is valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e647e475-19d9-4c2b-bdb1-d71613b12109
📒 Files selected for processing (2)
src/jsc/bindings/NodeVMSourceTextModule.cpptest/js/node/vm/vm.test.ts
There was a problem hiding this comment.
Thanks — 00f190c addresses my last note (cachedDataProduced now reports false when production fails, and the test locks it in), so I have nothing further. Deferring to a human for the final sign-off since this introduces a new cachedData wire format and changes the bytecode payload's memory ownership in core JSC bindings.
Extended reasoning...
Overview
This PR hardens node:vm's cachedData handling so that corrupted, truncated, or foreign buffers are rejected (cachedDataRejected = true / ERR_VM_MODULE_CACHED_DATA_REJECTED) instead of crashing inside JSC's bytecode decoder. It does so by wrapping serialized bytecode in a 16-byte { magic, version, XXH3-64 } header on the write side (createCachedDataBuffer) and verifying it on the read side (unwrapCachedData) before calling decodeCodeBlock. The verified payload is copied into a MallocSpan<uint8_t, JSC::VMMalloc> owned by the CachedBytecode (since JSC retains the decoder for lazy function-body decoding). All three entry points — vm.Script, vm.compileFunction, vm.SourceTextModule — are updated, along with their producers. Over the review cycle the PR also picked up fixes for three pre-existing null-deref sites and a stale cachedDataProduced(true) override, each with test coverage.
Files touched: src/jsc/bindings/NodeVM.{h,cpp}, NodeVMScript.cpp, NodeVMSourceTextModule.cpp, test/js/node/vm/vm.test.ts.
Security risks
The change is strictly a hardening: it gates an unsafe decoder behind an integrity check that previously did not exist. The "forgeable checksum" objection was raised and correctly dismissed in-thread — cachedData is an integrity/compatibility contract (matching V8's design), not an authentication boundary, and any caller who can craft a header is already executing arbitrary JS in-process. The payload copy removes a potential use-after-free where JSC's lazily-decoded functions could outlive the caller's buffer. I see no new exposure introduced.
Level of scrutiny
Moderate-to-high. This is ~150 lines of native C++ in core JSC bindings that (a) defines a new persisted binary format, (b) changes memory-ownership semantics for decoded bytecode, and (c) threads through three separate code paths. None of it is mechanical. The implementation looks correct and is well-tested (corruption matrix across all three entry points in a child process, plus the existing Node test-vm-cached-data.js cross-process round-trip), but the format and ownership decisions are the kind of thing a maintainer should ratify rather than a bot.
Other factors
The author has been responsive across four review rounds; every concern I and CodeRabbit raised has been addressed or rebutted with a sound argument, and all threads are resolved. The bug-hunting system found nothing on the latest revision. CI build #65446 is in flight for the head commit. Given the scope (native bindings, new wire format, lifetime change), I'm deferring rather than auto-approving.
|
Status at 8507750: the change is complete and verified. A from-scratch debug (ASAN) build at this head plus the full The red CI entries on recent builds have all been infrastructure rather than test failures: darwin agents failing artifact downloads or VM ssh before any test ran, one build queue not assigning an agent, and automated verification runs that failed to compile regardless of whether this diff was applied (a corrupted working tree in the runner environment, since repaired). No failure in any run has touched Summary of the change for review: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found this pass, and all earlier findings are addressed — but this is memory-safety C++ that introduces a new on-disk cachedData format, so it's worth a human sign-off on the header design and the owned-copy lifetime.
What was reviewed:
- Header validation in
unwrapCachedData— every field checked beforedecodeCodeBlocksees the payload;memcpyavoids alignment UB; length is compared against the actual span, not trusted. - The payload copy into
MallocSpan<uint8_t, VMMalloc>so JSC's lazy decoder does not borrow the caller's buffer. - All three consumers (
vm.Script,compileFunction,SourceTextModule) route through the same wrap/unwrap pair; the produce side symmetrically prepends the header at every site. - The
getBytecodeBuffer()/cachedData()null guards and the removedcachedDataProduced(true)override — now defensive-only after eagercheckSyntaxfrom #31827 closed the known repro.
Extended reasoning...
Overview
The PR wraps JSC's serialized bytecode in a 24-byte integrity header (magic, payloadLength, sourceHash, jscVersion, XXH3-64 payloadHash) so node:vm's three cachedData consumers can reject corrupted/truncated/mismatched buffers before handing them to JSC::decodeCodeBlock, which otherwise dereferences payload-relative offsets without bounds checks and segfaults. It also copies the accepted payload into memory owned by the CachedBytecode (JSC keeps the decoder alive for lazily decoded function bodies), and picks up three same-class defensive fixes on the produce side (getBytecodeBuffer null guard, SourceTextModule::cachedData null guard, dropped unconditional cachedDataProduced(true) override plus its now-dead setter).
Security risks
The input is user-controlled bytes flowing into a bytecode decoder — the fix is strictly a hardening in that direction. The header is an integrity check, not authentication (matching V8's contract for cachedData); a caller who can forge the header can also just supply matching bytecode, which is the documented capability. I did not find a way for the new code to make things worse: validation happens before allocation of the CachedBytecode, size arithmetic is on size_t spans with no user-multiplied quantities, and payload.size() != header.payloadLength rejects both truncation and extension.
Level of scrutiny
High. This is C++ in the JSC bindings, on a path that previously crashed the process from plain JS, and it defines a persistent binary format that becomes part of the user-visible createCachedData() contract. That combination — untrusted-input handling plus a new wire format — is exactly the kind of change a maintainer should eyeball, particularly the choice of header fields (e.g. whether source.hash() and computeJSCBytecodeCacheVersion() are the right staleness keys) and the VMMalloc allocator for the owned copy.
Other factors
Over several review rounds every finding was addressed: the module-path null-deref, the third sibling in getBytecodeBuffer, the cachedDataProduced flag inconsistency, the dead setter, and the comment-cop flags. CI was green across all completed lanes at 9ed4358 per the author's status comment. The corruption-matrix test spawns a child and asserts {stdout, stderr, exitCode, signalCode}, so a regression to the old crash would fail loudly. Deferring rather than approving because the format decision and the JSC lifetime reasoning deserve a maintainer's eyes, not because anything looks wrong.
cachedData was passed straight to JSC::decodeCodeBlock, which follows
offsets stored inside the buffer and is only safe on an intact copy of
its own serializer's output. A corrupted or truncated buffer crashed the
process (or was silently accepted), where Node sets cachedDataRejected
and recompiles.
createCachedData() and produceCachedData now prepend a
{ magic, version, xxh3 } header, and new vm.Script, vm.compileFunction,
and new vm.SourceTextModule verify it before decoding. Anything that
does not match takes the existing rejection path. The accepted payload
is copied into memory owned by the CachedBytecode because decoded
functions retain the decoder for lazy code block decoding.
…ld stderr in test
…fails
getBytecodeBuffer dereferenced m_cachedBytecode after cacheBytecode()
without checking it, but bytecode production fails without throwing for
source that does not parse as a program, so
new vm.Script("export default {};", { produceCachedData: true }).cachedData
crashed the process.
constructScript overrode the flag cacheBytecode had just computed, so a script whose source cannot be serialized reported cachedDataProduced as true while cachedData was undefined.
…che version The envelope now mirrors V8's SerializedCodeData header: magic + payload length + source hash + engine version + payload checksum. Each field is checked before the bytecode reaches JSC's decoder, so truncation and extension are caught by the length field, mismatched or stale source by the source hash and version, and any interior corruption by the payload hash. The decoder only ever sees bytes that round-trip the full header. Also brings the PR up to current main and drops a test whose premise no longer holds now that vm.Script validates syntax at construction.
Drops the header declaration doc comments (the definitions already explain the format) and collapses the remaining multi-line comments to one line each.
a8baf5e to
cae82eb
Compare
unwrapCachedData copies the payload into a MallocSpan owned by the CachedBytecode, so m_options.cachedData is dead storage once constructScript returns. Free it rather than retaining a second copy of the payload for the script's lifetime.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/vm/vm.test.ts`:
- Around line 906-924: Update corruptions() to include explicit empty and
zero-filled cached-data variants using Buffer.alloc(0) and
Buffer.alloc(good.length), alongside the existing cases, so both boundary
representations are passed through rejection validation before header decoding.
- Around line 945-970: Extend the cached-data tests in the compileFunction and
SourceTextModule blocks to create cached data from a distinct source, then
supply it to the target source. Assert compileFunction rejects the mismatched
data and still executes normally, while SourceTextModule throws
ERR_VM_MODULE_CACHED_DATA_REJECTED; retain the existing corruption and
matching-data checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4eb3345-9772-4223-9fb9-6348c07f9cf9
📒 Files selected for processing (6)
src/jsc/bindings/NodeVM.cppsrc/jsc/bindings/NodeVM.hsrc/jsc/bindings/NodeVMScript.cppsrc/jsc/bindings/NodeVMScript.hsrc/jsc/bindings/NodeVMSourceTextModule.cpptest/js/node/vm/vm.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/bindings/NodeVMScript.h
…matrix The consumers gated validation on the buffer being non-empty, which made an explicitly passed empty buffer behave as if the option were absent. Node reports it as rejected (vm.Script and compileFunction set cachedDataRejected, SourceTextModule throws ERR_VM_MODULE_CACHED_DATA_REJECTED). Gate on whether the option was provided so the empty payload flows into the same rejection path. Also adds empty, zero-filled, and cross-source variants for compileFunction and SourceTextModule to the corruption matrix.
There was a problem hiding this comment.
I reviewed this PR again after 8507750 and didn't find any bugs. Because it changes memory-safety C++ in the JSC bindings on a security-relevant path (user-controlled bytes reaching decodeCodeBlock), a human look would still be worthwhile.
What was reviewed:
unwrapCachedData: header is read via memcpy after a size check, every field is compared before the payload copy, and the accepted payload is copied into an ownedMallocSpansocompileFunction/SourceTextModuleno longer hand JSC a span into a stack Vector.- The provided-vs-empty gate in all three consumers now keys on presence (
cachedDataProvided/!isUndefined());validateCachedDatareturns true for a zero-length view, soBuffer.alloc(0)reaches the header check and is rejected. createCachedDataBufferwrites header+payload into a fresh Uint8Array; thepayloadLengthnarrow touint32_tis fine for realistic bytecode sizes.- The subprocess test asserts
signalCode === nulland covers the full corruption matrix (including empty/zeros and cross-source) across Script, compileFunction, and SourceTextModule.
Extended reasoning...
Overview
The PR adds a 24-byte integrity header (magic, payload length, source hash, JSC bytecode-cache version, XXH3-64 payload hash) to the buffers createCachedData()/produceCachedData emit, and a matching unwrapCachedData() that verifies every field before JSC::decodeCodeBlock sees the payload. All three cachedData consumers — new vm.Script, vm.compileFunction, new vm.SourceTextModule — route through it, and the accepted payload is copied into memory owned by the CachedBytecode instead of borrowed. The latest commit (8507750) switches the consumers from gating on !cachedData.isEmpty() to gating on whether the option was provided, so an explicitly-passed empty buffer is rejected like Node does, and extends the test matrix with empty/zero-fill variants and cross-source rejection for compileFunction/SourceTextModule.
Security risks
This is exactly the security-relevant surface: user-controlled bytes previously flowed straight into JSC's bytecode decoder, which derives pointers from self-relative offsets in the payload with no bounds check — truncation produced wild heap reads and a SIGSEGV. The fix is the right shape (validate-before-decode with a checksum, mirroring V8's SerializedCodeData header), and the validated payload is copied so the decoder's lazy function-body decoding can't outlive the caller's buffer. I did not find a bypass: length is an exact match (catches both truncation and extension), and the XXH3-64 payload hash catches bit flips that leave length intact. The header is read via memcpy after a size < sizeof(CachedDataHeader) guard, so there's no unaligned or short read.
Level of scrutiny
High. This is hand-written C++ in src/jsc/bindings/, on a path where the pre-fix behavior was a native crash from user input, and it changes ownership of a buffer that JSC retains for lazy decoding. REVIEW.md's memory-safety and untrusted-input sections apply directly. The change is well-scoped and has been through ten iterations with every prior review finding addressed, but it is not the kind of mechanical change I'd approve without a human look.
Other factors
All prior inline findings from this reviewer and CodeRabbit are marked resolved and I verified the fixes in the current diff: the redundant m_options.cachedData retention is released after unwrapCachedData copies (cachedData = {}); the dead cachedDataProduced(true) setter was removed; the getBytecodeBuffer() null guard is defensive-only after #31827 (no deterministic fixture exists). The test follows harness conventions — subprocess with concurrent pipe drain, combined {stdout, stderr, exitCode, signalCode} assertion, deterministic corruption seeding — and the PR description's evidence block shows it fails on main (SIGSEGV/assertion) and passes on the branch under both ASAN debug and release.
1 participant
Problem
new vm.Script(source, { cachedData })with a buffer that is not an intact copy ofcreateCachedData()output can crash the process. Truncation, the most common real-world cache corruption (partial write, stale file), is enough:A debug build reports the same input as an assertion inside JSC's bytecode decoder:
Depending on which bytes differ the buffer can instead be silently accepted (
cachedDataRejected === false). Node rejects and recompiles from source.vm.compileFunction()andnew vm.SourceTextModule()accept the same option and behaved the same way.Cause
options.cachedDatais only type-checked and then handed directly toJSC::decodeCodeBlock(). JSC's decoder derives pointers from self-relative offsets stored in the payload and expects its own serializer's output; there is no entry point that validates an arbitrary buffer (the format carries no length field and no checksum, and the staleness check itself reads payload-relative offsets). The entry header andSourceCodeKeyare encoded first and the code block last, so a truncated tail leaves decoded offsets pointing past the exactly-sized copy: wild heap reads. V8 solves this in the layer that owns the format: the code cache carries a header with a magic number, version, source hash, payload length and payload checksum, and a mismatch is what setscachedDataRejectedin Node.Fix
Do the same in the
node:vmbindings:createCachedData()andproduceCachedDataprepend a 24-byte{ magic, payloadLength, sourceHash, jscVersion, payloadHash }header (XXH3-64 of the serialized bytecode) to the buffer they hand to JS.new vm.Script,vm.compileFunction, andnew vm.SourceTextModuleverify every field before decoding. A buffer that is not byte for byte a previouscreateCachedData()output for the same source (corrupted, truncated, extended, for different source, or not produced by this Bun build) is rejected up front and takes the existing rejection path:cachedDataRejected === true, orERR_VM_MODULE_CACHED_DATA_REJECTEDfor modules, and the source is compiled normally.CachedBytecodeinstead of borrowing the caller's buffer, since JSC keeps the decoder alive for lazily decoded function bodies.cachedDatais only usable by the runtime that produced it (as in Node across V8 versions), so the format change does not invalidate anything that previously worked.Defense-in-depth bounds checks inside JSC's decoder itself are in oven-sh/WebKit#368 (the decoder entry points reject short buffers and out-of-range embedded offsets instead of reading past the span); that lands via a WebKit version bump and is independent of this fix.
Tests
New test in
test/js/node/vm/vm.test.tsruns a corruption matrix ({len-1, len/2, 16, 1}truncations, 20 single-byte flips seeded across the buffer, extension, unrelated bytes) through all three entry points in a child process, asserting each variant is rejected and the code still runs, that intact data is still accepted, and that intact data for a different source is rejected. Without the fix the child crashes with the panic above (release) or theDecoder::offsetOfassertion (debug), or accepts corrupted data.Existing coverage still passes: the
cachedDatatests intest/js/node/vm/vm.test.ts, andtest-vm-cached-data.js(cross-process produce/consume),test-vm-createcacheddata.js,test-vm-module-cached-data.js,test-vm-basic.jsfrom the Node suite.[review] gate passed · iteration 10 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 10
evidence per changed file