Fix type-confused write through ReadableStream's private custom setters - #37058
Conversation
…essors ReadableStream.prototype installs $bunNativePtr/$bunNativeType/$disturbed as DOMAttribute custom accessors. JSC validates the annotated ClassInfo for getters (PropertySlot::customGetter) but not for setters, so an ordinary put whose receiver merely inherits ReadableStream.prototype, e.g. a node Readable after Object.setPrototypeOf(EventEmitter.prototype, ReadableStream.prototype), invoked the setters with that foreign receiver, and uncheckedDowncast<JSReadableStream> then wrote a JSValue through a type-confused pointer into the receiver's inline property storage. On release builds this silently overwrote the Readable's own Symbol(kCapture) slot; ASAN debug builds abort in reportZappedCellAndCrash. Replace uncheckedDowncast with dynamicDowncast in all six accessors and throw the matching DOMAttribute getter/setter type error on a foreign receiver. Also assign $bunNativePtr with $putByIdDirectPrivate in native-readable.ts so Readable.fromWeb defines the own property directly instead of consulting the prototype chain, keeping fromWeb working under such prototype surgery.
|
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 (2)
WalkthroughChangesThe change protects ReadableStream safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches memory-safety-critical JSC bindings (type-confusion fix via uncheckedDowncast → dynamicDowncast brand checks) and changes how the private $bunNativePtr slot is defined on node Readables, so it's worth a human look.
Checked: the six accessors follow the same dynamicDowncast + throwVMDOMAttributeGetterTypeError / throwDOMAttributeSetterTypeError pattern used by generate-classes.ts codegen and JSDOMAttribute.h; ThrowScope is declared in each. $putByIdDirectPrivate(stream, "bunNativePtr", ...) matches the existing use in ConsoleObject.ts and the later this.$bunNativePtr reads in native-readable.ts / tty.ts / webstreams_adapters.ts will find the own private-name property. No other JS-side writes to $bunNativePtr / $bunNativeType / $disturbed on non-ReadableStream receivers exist. No CODEOWNERS coverage on the touched files.
Extended reasoning...
Overview
The PR fixes a type-confused write reachable through ReadableStream.prototype's private custom setters ($bunNativePtr / $bunNativeType / $disturbed). Three files change: JSReadableStream.cpp replaces uncheckedDowncast<JSReadableStream> with dynamicDowncast + a receiver brand check in all six accessors, throwing throwVMDOMAttributeGetterTypeError / throwDOMAttributeSetterTypeError on mismatch; native-readable.ts swaps the ordinary put stream.$bunNativePtr = ptr for $putByIdDirectPrivate so it defines an own property without walking a possibly-tampered prototype chain; and a subprocess regression test lands in node-stream.test.js.
Security risks
This is a security fix — the unfixed setters wrote through a type-confused JSReadableStream* when a foreign receiver inherited the prototype (silent inline-storage clobber in release, ASAN abort in debug). The new code strictly narrows behavior: foreign receivers now throw instead of writing through a bad pointer. I don't see new attack surface introduced.
Level of scrutiny
High. This is JSC C++ bindings + memory safety, which is the most-blocked review category in this repo. Even though the change is mechanical (six near-identical stanzas following the exact idiom already used by generated bindings and JSDOMAttribute.h), a maintainer who knows JSC's put path and the private-name intrinsic semantics should confirm that (a) the getter-side brand check is redundant-but-harmless given PropertySlot::customGetter's central check, and (b) $putByIdDirectPrivate on the node Readable interoperates with every downstream .$bunNativePtr read (I verified all read sites in src/js/ use ordinary private-name gets, which will find the own property).
Other factors
The test follows harness conventions (subprocess isolation for global prototype mutation, drains stdout/stderr concurrently, asserts output before exit code). No sibling JS-side writes to these private names on non-ReadableStream objects remain. throwDOMAttributeSetterTypeError and throwVMDOMAttributeGetterTypeError are JSC-namespace helpers already available via <JavaScriptCore/Error.h> and using namespace JSC. No CODEOWNERS match. Deferring rather than approving because per the review guidelines memory-safety changes in bindings warrant human sign-off even when they look correct.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
CI status: 195 of 196 jobs passed on 98f8b27. The one failed job is a darwin 14 x64 test shard that died from runner infrastructure, not from this diff: the box's nightly cleanup (builds wipe plus reboot, about 06:30 box-local) fired mid-job, so the checkout and test binary vanished under the running job and then user lookups failed with uv_os_get_passwd ENOENT. The previous build failed the same way on a different box inside the same window, and sibling boxes ran the identical shard green at the same time. The other annotations are retry-passed flakes on unrelated lanes (windows napi node-gyp, worker_blob parallel batch, fastutf8stream on darwin aarch64). A retry of that single job should come back green; the diff needs no further changes. |
There was a problem hiding this comment.
No issues found, but this is a memory-safety fix in JSC C++ bindings (type-confused write via uncheckedDowncast), so it's worth a human look.
What was reviewed:
- The six accessors now match the
dynamicDowncast+ throw pattern used by every other accessor/host function in this file;throwDOMAttributeSetterTypeErroris the same helpergenerate-classes.tsemits. $putByIdDirectPrivate(stream, "bunNativePtr", ...)writes the same private name later read asthis.$bunNativePtr; greppedsrc/jsfor other ordinary-put writers to$bunNativePtr/$bunNativeType/$disturbedon non-ReadableStream receivers — none found.- The regression test asserts both the no-clobber invariant and that the stream still delivers data (i.e.,
fromWebdoesn't now throw in the grafted case).
Extended reasoning...
Overview
Three files: JSReadableStream.cpp replaces uncheckedDowncast<JSReadableStream> with dynamicDowncast + brand-check-and-throw in the six private-name custom accessors ($bunNativePtr / $bunNativeType / $disturbed getter+setter). native-readable.ts replaces stream.$bunNativePtr = ptr (a prototype-chain-walking put) with $putByIdDirectPrivate(stream, "bunNativePtr", ptr) (own-property define). A subprocess regression test is added to node-stream.test.js.
Security risks
The bug being fixed is itself security-relevant: an unchecked downcast followed by m_nativePtr.set() writes a JSValue at a fixed member offset into whatever cell happens to be the receiver, which the PR demonstrates clobbering an inline property slot on a node Readable. The fix closes that with the standard DOMAttribute brand check. I don't see the fix introducing new risk — dynamicDowncast is the established safe pattern, and the throw helpers are the same ones IDLAttribute::set and generated bindings use. The TS-side change avoids a functional regression (with only the C++ fix, Readable.fromWeb would throw a TypeError when the prototype chain is grafted; $putByIdDirectPrivate defines the own slot without consulting the chain, so the stream keeps working — verified by the test's read:y assertion).
Level of scrutiny
High. This is native JSC binding code on a hot Web API, and the failure mode of the original bug is memory corruption reachable from user JS. The change itself is small and mechanical (matches the pattern of jsReadableStreamPrototypeGetter_locked and every host function in the same file), but a maintainer who knows the JSC put/setter dispatch path should confirm the analysis in the PR description — specifically that putInlineSlow really does invoke DOMAttribute custom setters without a receiver brand check, and that no other setter entry point (e.g., putDirect, IC'd puts) needs separate handling.
Other factors
The comment-cop bot flagged the code comments twice; the author trimmed them and the threads are resolved. I grepped src/js for other ordinary-put assignments to these three private names on possibly-foreign receivers and found none, so the one fixed site appears to cover the whole class on the JS side. The test follows the harness conventions (subprocess with bunEnv, drains stdout/stderr/exited concurrently, asserts stderr/stdout before exitCode).
2 participants
Problem
ReadableStream.prototypeinstalls$bunNativePtr/$bunNativeType/$disturbedasDOMAttributeGetterSettercustom accessors. JSC brand-checks DOMAttribute getters centrally (PropertySlot::customGetterthrows for a foreign receiver), but nothing checks custom setters:JSObject::putInlineSlowinvokes the setter for any put whose receiver merely inherits the slot. The three setters diduncheckedDowncast<JSReadableStream>(thisValue)and wrote through the result.Bun's own
Readable.fromWebpath (internal/streams/native-readable.ts) assignsstream.$bunNativePtr = ptron a nodeReadablewith an ordinary put. Once user code graftsReadableStream.prototypeinto the node stream prototype chain, that put walks the chain into the setter with theReadableasthis, andm_nativePtr.set()writes a JSValue at an offset that lands inside theReadable's inline property storage.On Bun 1.4.0 this prints
clobbered own slot Symbol(kCapture) [object Boolean] -> [object BlobInternalReadableStreamSource]and exits 0 (silent type-confused write). ASAN debug builds abort inreportZappedCellAndCrashunderuncheckedDowncast<WebCore::JSReadableStream>called fromjsReadableStreamPrototype_nativePtrSetter.Fix
JSReadableStream.cpp: all six private-name accessors nowdynamicDowncast<JSReadableStream>the receiver and throw the matchingthrowVMDOMAttributeGetterTypeError/throwDOMAttributeSetterTypeErroron a mismatch, same as the check JSC already applies on the getter path.native-readable.ts: assign$bunNativePtrwith$putByIdDirectPrivate, which defines the own property without consulting the prototype chain, soReadable.fromWebkeeps working even when the prototype chain has been rearranged.Verification
New test in
test/js/node/stream/node-stream.test.jsruns the repro in a subprocess, asserts no own slot is clobbered, and that the stream still delivers its data. It fails on the unfixed build (release: clobbered slot; ASAN debug: abort) and passes with this change. Fullnode-stream.test.js,web/streams/streams.test.js,process-stdin.test.ts, andchild-process-stdio.test.jspass.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/stream/node-stream.test.js