Conversation
…orting extractCachedData copied the option into a WTF::Vector, which holds at most INT_MAX bytes and crashes on a larger capacity. It now checks the length before the copy. new vm.Script and vm.compileFunction set cachedDataRejected, and new vm.SourceTextModule throws ERR_VM_MODULE_CACHED_DATA_REJECTED. Node gives the same results for a buffer that long, unless the buffer starts with a valid cache.
|
Status Reproduction, on a release build of main ( const vm = require("node:vm");
new vm.Script("1", { cachedData: new Uint8Array(2 ** 31) });The process aborts with
The fix is in this PR (#42734). |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughCached-data extraction now distinguishes invalid, copied, and oversized inputs. Script, compile-function, and source-text-module paths propagate oversized state and reject the data safely. A subprocess regression test covers 2 GiB-or-larger buffers. ChangesCached-data handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Oversized cached data is rejected safely across the supported VM APIs, with no actionable merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — a narrow bounds check that turns a user-reachable abort into the same rejected result Node reports.
What was reviewed:
extractCachedDatanow takes a span first and gates the Vector copy onisValidCapacityForVector<uint8_t>; the tri-state enum is exhaustively switched at all three call sites and thecachedDataTooLongfield is default-initialized in both option structs and the SourceTextModule local.- All three sibling entry points (
vm.Script,vm.compileFunction,SourceTextModule) are covered; the too-long branch falls through to compile-from-source withcachedDataRejected = truefor the first two and throwsERR_VM_MODULE_CACHED_DATA_REJECTEDafter the syntax check for modules, matching the cited Node ordering. - No new JS-entering calls are introduced under the existing ThrowScopes, so exception-check discipline is unchanged.
- Test follows harness conventions: subprocess via
bunExe -e,{...bunEnv, ASAN_OPTIONS: ...},Promise.allpipe drain, combined-object assertion beforeexitCode, and address-space-only 2 GiB reservation with a SKIP path.
Extended reasoning...
Overview
This PR fixes a process abort in node:vm when cachedData of 2 GiB or more is passed to vm.Script, vm.compileFunction, or vm.SourceTextModule. The root cause was extractCachedData in src/jsc/bindings/NodeVM.cpp copying the buffer into a WTF::Vector<uint8_t>, whose allocator crashes past INT_MAX. The fix converts the helper's bool return to a three-value CachedDataExtraction enum, checks WTF::isValidCapacityForVector<uint8_t> before the copy, and threads a cachedDataTooLong flag through BaseVMOptions::validateCachedData into ScriptOptions/CompileFunctionOptions and a local in NodeVMSourceTextModule::create. Callers set cachedDataRejected = true (Script/compileFunction) or throw ERR_VM_MODULE_CACHED_DATA_REJECTED (SourceTextModule, after the syntax check) instead of attempting the decode. A new subprocess test in test/js/node/vm/vm.test.ts reserves a 2 GiB ArrayBuffer (address space only) and asserts rejection for Uint8Array, DataView, and bare ArrayBuffer variants.
Security risks
None introduced. The change is a defensive hardening: it adds a bounds check on a user-supplied buffer length before allocation, converting a DoS-class abort into a recoverable error. No new allocations, no GC-visible objects, no ref-count changes, and no new JS-entering calls are added under the existing ThrowScopes. The span is taken from the buffer view synchronously and copied (or not) before any user code can run again, so no new detach hazard.
Level of scrutiny
Moderate. C++ JSC bindings are memory-safety-sensitive, but this diff is small (~90 lines across 6 files), mechanical, and adds — rather than removes — a guard. It is exactly the "treat all size/index/length arithmetic on external data as adversarial" pattern REVIEW.md calls for, and the "user-reachable failures are recoverable errors, never panics" rule. I verified the enum is exhaustively handled at every switch, the new bool fields default to false at every declaration site, and the too-long branch in constructAnonymousFunction leaves unlinkedProgramCodeBlock null so control falls through to the compile-from-source path with bytecodeAccepted = TriState::False.
Other factors
The bug hunt ran to dry_streak with no findings. No CODEOWNERS entry covers these paths. The PR description cites the exact Node source lines (node_contextify.cc:1027-1028, module_wrap.cc:419) that produce the same observable behavior, and the test extends the existing test/js/node/vm/vm.test.ts per the test-organization rule. The test follows every harness convention I checked: bunExe()/bunEnv spread, ASAN_OPTIONS appended (not overwritten), concurrent pipe drain via Promise.all, combined {stdout, stderr} object assertion before exitCode, and a graceful SKIP when the 2 GiB reservation fails. No outstanding third-party reviews on the timeline.
Problem
new vm.Script("1", { cachedData: new Uint8Array(2 ** 31) })aborts:panic(main thread): abort() called, exit 134. It costs 0.1 s and 30 MB of RSS: nothing writes to the buffer.vm.compileFunctionandnew vm.SourceTextModuleabort too.extractCachedData(src/jsc/bindings/NodeVM.cpp:111). It copies the option into aWTF::Vector<uint8_t>, which holds at mostINT_MAXbytes.VectorBufferBase::allocateBuffer<FailureAction::Crash>callsCRASH()for more.Fix
extractCachedDatachecks the length withWTF::isValidCapacityForVector<uint8_t>before the copy. It does not copy a longer buffer, and the three entry points report it as rejected without a decode:cachedDataRejected === trueforScriptandcompileFunction,ERR_VM_MODULE_CACHED_DATA_REJECTEDforSourceTextModule(after the syntax check).test/js/node/vm/vm.test.ts. The new test aborts on main and passes here, withBUN_JSC_validateExceptionChecks=1too. The 97test-vm-*.jsNode tests pass.Bun::maxVectorSize, and anifwrapper in place of the secondthrowError.Background
cachedDatais the serialized bytecode thatscript.createCachedData()returns. A constructor decodes it and setscachedDataRejectedwhen the decode fails.intof V8'sScriptCompiler::CachedData. V8 then rejects bytes that do not start with a valid cache header. Bun rejects by the length and copies nothing.Notes
Node v26.3.0 and this branch print the same line for the script below (
node --experimental-vm-modules). It is the fixture of the new test with two changes for Node: aBufferin place of the bareArrayBuffer(Node refuses one withERR_INVALID_ARG_TYPE, Bun accepts one and that arm aborted too), and a distinct source per call (V8 skipscachedDatafor a source that is already in its compilation cache).How Node gets there (v26.3.0).
src/node_contextify.cc:1027-1028passescached_data_buf->ByteLength()toScriptCompiler::CachedData(const uint8_t*, int length)(deps/v8/include/v8-script.h:470). 2^31 becomesINT_MIN. V8 stores it asuint32_t size_(deps/v8/src/snapshot/snapshot-data.h:65), so the size check inSerializedCodeData::SanityCheckWithoutSource(code-serializer.cc:788) passes and the magic number check rejects the zero bytes. Modules throw atsrc/module_wrap.cc:419.Where Bun now differs from Node, on purpose.
cachedDataRejected === false. Bun reportstrue. No producer emits such a buffer, and to match Node here Bun must copy 2 GiB or more.new Uint8Array(new ArrayBuffer(2 ** 31 + 8), 1, 2 ** 31). Node aborts:FATAL ERROR: NewArray Allocation failed - process out of memory(V8 copies unaligned data, with the negative length). Bun reportstrue.Why rejected and not a RangeError. Node returns a usable
Scriptand compiles from source. A throw would turn working Node code into an exception. A buffer of 2^31 - 1 bytes already gave these results in Bun. The state costs oneboolnext to the copied bytes, placed so thatScriptOptionsandCompileFunctionOptionsstay at 64 bytes.The test runs a child that reserves one 2 GiB
ArrayBufferand passes it as aUint8Array, aDataViewand a bareArrayBuffer. It follows the 2 GiB tests inweb-crypto.test.tsandsqlite.test.js: the child printsSKIPwhen it cannot reserve the memory. On the debug ASAN build the reservation takes 5 ms and the test 0.5 s.setSyntheticAllocationLimitForTesting(throughBun::maxVectorSizefromVectorSizeLimit.h). Below the real bound, the unfixed code copies the bytes and the decoder rejects them, so both builds print the same result. A difference would need a valid cache that is padded to the limit and still accepted, and node:vm: reject invalid cachedData instead of crashing #32839 makes the decoder reject padded data. The abort at the real bound is the one stable observable, and it is cheap to reach.cachedData" for modules, although this branch keeps that order (see the output above). WithBUN_JSC_validateExceptionChecks=1, which the ASAN lane sets, every module with a syntax error and a non-emptycachedDatatripsUnchecked JS exception ... getUnlinkedCodeBlock @ ModuleProgramExecutable.cpp:61. A 10 bytecachedDatadoes it on main. Bump WebKit sonew Functionwith a syntax error survives validateExceptionChecks #40866 fixes that.Unchanged on purpose. An empty
cachedDatastill counts as absent (Node reports it as rejected), andproduceCachedDatais still ignored whencachedDatais given. A general "cachedDatawas provided" bit in place ofcachedDataTooLongwould also change the result for an emptycachedData. That change belongs to #32839.Other
cachedDatawork in flight. #42229 gives the payload to theCachedBytecodethat decodes it. #32839 validates a header before the decode. #41769 drops the baseline compile inconstructScript. The first two change theCachedBytecode::create(std::span(cachedData), ...)lines, and the third edits the block around one of them. This change does not touch those lines. None of the three bounds the length or removes the copy inextractCachedData. Whichever lands second rebases.The same class in other places. #42648 tracks containers that abort when script makes them too large. One more site has no owner:
buffer.transcode(new Uint8Array(2 ** 31), "latin1", "utf16le")aborts injsBufferTranscode(src/jsc/modules/NodeBufferModule.cpp:131,result.grow(length * 2)on aWTF::Vector<uint8_t>). It is a different module, so it is not in this change.A different abort in the same file, also not in this change.
const p = Buffer.alloc(2 ** 30, "a").toString("latin1"); vm.compileFunction("", [p, p])aborts instringifyAnonymousFunction(NodeVM.cpp:424): theStringBuilderfor the parameter list crashes when it passes the string length limit. It needs 2 GiB of real string memory, so it is the string limit class (#37215), not this one, and it has no cheap test.The second
throwErrorinNodeVMSourceTextModule.cpp. Anif (!cachedDataTooLong) { ... }around the decode gives one throw site, but it re-indents the lines that #42229 and #32839 edit. The early reject leaves them byte-identical to main.Found by a fuzzing run. It reproduces on 1.3.14 through 1.4.2, canary and main, so it is not a regression.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/vm/vm.test.ts