node:zlib: pin _writeState buffer so transfer() can't free it mid-work#31206
node:zlib: pin _writeState buffer so transfer() can't free it mid-work#31206alii wants to merge 6 commits into
Conversation
|
Updated 6:37 PM PT - Jun 1st, 2026
❌ @robobun, your commit 6bacf38 has 11 failures in
The baseline build contains instructions not available on Static scan violations
|
|
@robobun adopt |
WalkthroughThis PR pins JS ArrayBuffer backing stores during native async compression work: it tightens an Image.rs FFI binding, adds pin/unpin FFI and helper infrastructure, stores pinned JSValues in native zlib/brotli/zstd objects, integrates pinning/unpinning into async write/close flows, and adds tests for ArrayBuffer.transfer() mid-write. ChangesArrayBuffer pinning for safe async compression
Suggested reviewers:
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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 `@src/runtime/node/node_zlib_binding.rs`:
- Around line 331-343: finalize() currently bypasses the unpin logic so pinned
ArrayBuffers can leak; ensure the finalize path runs the same unpin code on the
JS thread by invoking release_lifetime_pinned() (or inlining its logic) before
destroying the native object. Specifically, update the finalize() branch that
does T::deref(Box::into_raw(this)) to first call release_lifetime_pinned()
(which uses pinned_write_state() and pinned_dictionary() and
JSC__JSValue__unpinArrayBuffer) or otherwise unpin those JSValues on the JS
thread, so that resources are released even when close_internal() is never
called.
In `@test/js/node/zlib/zlib.test.js`:
- Around line 659-689: Extend the test to also exercise detaching the source
input buffer mid-write: after s.write(compressed) call, invoke
compressed.buffer.transfer(0) and ensure the same assertions (stdout === "OK"
and exitCode === 0). Concretely, add a third iteration (or an additional it
block) alongside the existing detach loop that performs
compressed.buffer.transfer(0) (referencing the local variable compressed),
keeping the same use of z.${compressSync}, z.${createDecompress},
s.write(compressed), s.end() and the spawned Bun process so the test verifies
the pinned input-buffer path as well.
🪄 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: 0555edcb-e832-481f-bb81-6c982b3d1c0d
📒 Files selected for processing (6)
src/runtime/image/Image.rssrc/runtime/node/node_zlib_binding.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rstest/js/node/zlib/zlib.test.js
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![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 is a memory-safety fix with pin/unpin lifecycle management across FFI + worker-thread boundaries and several error-path cleanups, so it's worth a human look at the pin/unpin balance (especially the lifetime-scoped pins when close() is never called before finalize) and the reordered write_in_progress/pending_close checks.
Extended reasoning...
Overview
This PR fixes a use-after-free in node:zlib/brotli/zstd's async write() path: the native code caches raw pointers into JS-owned input/output/_writeState ArrayBuffers, then schedules work on the threadpool. In that window JS can call .transfer(0) and free the backing store under the worker thread. The fix pins the ArrayBuffers via the existing JSC__JSValue__pinArrayBuffer helper before reading their pointers, and unpins on the JS-thread completion path (per-write for in/out, stream-lifetime for _writeState/dictionary). It touches node_zlib_binding.rs (mixin trait + write()/run_from_js_thread()/close_internal()), the three Native{Zlib,Brotli,Zstd}.rs structs (new pinned_* fields + init() pin calls), a small Image.rs change to make the shared unpinArrayBuffer extern safe fn, and adds spawned-subprocess tests for all three codecs.
Security risks
The change itself reduces security risk (closes a UAF reachable from JS). The risk surface is in getting the pin/unpin balance wrong: a missed unpin leaks (benign), a double-unpin or unpin-of-stale-JSValue could be problematic. The per-write pins look balanced (idempotent release_pinned with replace(ZERO), called on every error path and once in run_from_js_thread). The lifetime pins are only released in close_internal(); if a stream is GC'd without close(), the stale JSValues in the cells are never dereferenced (no unpin is attempted from deinit/finalize), so that appears safe but is worth confirming.
Level of scrutiny
High. This is FFI + cross-thread raw-pointer lifecycle code in a Node-compat module, fixing a memory-safety bug. The pin-before-read ordering matters (pinning a FastTypedArray relocates storage), the error-path cleanup must be exhaustive, and the write_in_progress/pending_close checks were reordered ahead of buffer parsing (minor observable change in which error wins). The reasoning in the comments is thorough and the pattern mirrors Image.rs, but the number of interacting states (per-write vs lifetime pins, error paths, pending_close deferral, finalize-without-close) makes this worth a careful human pass.
Other factors
No prior reviews on this PR. Tests are added (including spawned subprocesses so a pre-fix crash doesn't take down the test runner), and the bug-hunting system found nothing. The Image.rs hunk is mechanical (safe fn on a by-value extern that the C++ side null-checks).
|
✅ Rebased onto current main as requested ( I investigated every red lane on the rebased run (build 59631) — none touch this diff:
No zlib test or zlib code appears in any failure. I've already used my one CI re-roll on this PR, so I'm not pushing further empty retriggers — the diff is done, reviewed (all threads resolved), and verified; it needs a maintainer to retry the flaky jobs or merge. |
There was a problem hiding this comment.
I didn't find any new correctness issues, but this is a UAF fix that threads pin/unpin state across the WorkPool boundary and reorders the async write() validation path — worth a human pass on the lifecycle (per-write vs lifetime pins, error-path unpin coverage, and the finalize() question CodeRabbit raised).
Extended reasoning...
Overview
This PR fixes a heap-use-after-free in the async write() path of node:zlib/brotli/zstd: raw pointers into JS-owned input/output/_writeState/dictionary ArrayBuffers were cached and handed to a threadpool worker, while JS could call buffer.transfer(0) and free the backing store mid-work. The fix pins those ArrayBuffers via the existing JSC__JSValue__pinArrayBuffer helper before reading their pointers, with two scopes: per-write pins (in/out, released in run_from_js_thread) and lifetime pins (_writeState + zlib dictionary, released in close_internal). It touches node_zlib_binding.rs (trait extension + helpers + write-path reordering), the three Native{Zlib,Brotli,Zstd}.rs structs (4 new Cell<JSValue> fields each + init() pin calls), a small Image.rs extern-decl harmonization (safe fn), and a new spawned-process test file covering all three codecs.
Security risks
This is itself a memory-safety hardening change against ArrayBuffer.transfer()-induced UAF. The risks to review are in the fix's own lifecycle: pin/unpin balance on every error path in write(), the hoisted write_in_progress/pending_close checks (now run before pinning — a behavior-preserving reorder, but worth confirming), correctness of pinning before as_array_buffer() (since pin can relocate FastTypedArray storage), and whether stashing JSValues in non-rooted Cells is safe to deref at unpin time (the code argues yes via _handle[owner_symbol] keeping the JS wrapper alive). No new attack surface is introduced.
Level of scrutiny
High. This is Rust↔JSC FFI managing raw-pointer lifetimes across threads, in code that runs for every async zlib/brotli/zstd operation. The pattern mirrors Image.rs but is applied to a different lifecycle (init-time lifetime pins + per-write pins vs. per-task pins), so it isn't a mechanical copy. Mistakes here manifest as UAF, double-unpin, or pin leaks rather than test failures.
Other factors
CI shows failures on the head commit. CodeRabbit raised a question about whether finalize() (GC path without explicit close()) needs to unpin the lifetime-scoped buffers — looking at bindings.cpp, pin() is a flag on the ArrayBuffer impl (not a GC root or extra ref), and the struct comment says "Not GC roots — the JS wrapper already keeps the values alive; these only block detach", so it's likely benign when the whole graph is collected, but a maintainer should confirm. github-actions also flagged three potentially overlapping PRs (#28250/#30118/#30120) addressing the same buffer-lifetime family of bugs, which is worth reconciling before merge.
alii
force-pushed
the
ali/zlib-arraybuffer-pin
branch
from
1276759 to
bda7b60
Compare
alii
changed the title
|
@robobun adopt |
There was a problem hiding this comment.
All prior feedback addressed (GC-visited writeState slot, Zstd pin ordering, it.concurrent.each) and no new issues found — deferring only because native UAF-hardening with cached raw pointers into JSC-managed buffers is worth a maintainer glance.
Extended reasoning...
Overview
Pins the _writeState Uint32Array's backing store in each of NativeZlib/NativeBrotli/NativeZstd init() so stream._writeState.buffer.transfer(0) can't free the storage under the cached write_result raw pointer that flush_write_result writes through after every async write. Adds a writeState entry to the GC-visited values: list in zlib.classes.ts (and the matching codegen_cached_accessors! entry) so the view is rooted for the wrapper's lifetime, and adds a 3-codec subprocess regression test. Net change is small (~40 lines of logic across three near-identical init() bodies plus codegen wiring and a test file).
Security risks
The PR fixes a UAF (write-through-freed-pointer) reachable via underscore-private property abuse. The fix reuses the exact as_pinned_arraybuffer + GC-visited cached-value pattern that write() already uses for pendingInput/pendingOutput a few lines away. My earlier 🔴 (storing the pinned JSValue in a non-GC-visited Cell<JSValue>, creating a stale-JSCell deref in close()) was addressed by moving to the codegen'd cached slot and dropping the unpin-on-close path entirely — close_internal() is unchanged in the final diff. No new attack surface introduced.
Level of scrutiny
Medium-high: this is Rust/JSC FFI memory-safety code that caches a raw *mut u32 across async work and GC boundaries. The change is narrow, pattern-matched to existing code, and well-tested, but raw-pointer lifetime reasoning against JSC's GC/pin semantics is exactly where a maintainer (CodeRabbit suggested dylan-conway/RiskyMH) should confirm the invariants.
Other factors
All review threads are resolved and the fixes match what was asked. The bug-hunting system found nothing on the current revision. CI shows two musl build failures, but the error ("Linking two modules of different data layouts … regular-lto-flag-stub.bc") is an LTO infrastructure issue unrelated to this diff. No CODEOWNERS entry covers these paths.
There was a problem hiding this comment.
All prior review feedback is addressed and I found no remaining issues, but since this touches memory-safety-critical native code (cached raw pointers into JS-owned buffers, GC-visited slots, ArrayBuffer pin semantics across the JSC boundary), it's worth a human sign-off.
Extended reasoning...
Overview
This PR closes a UAF gap in the node:zlib native bindings: _writeState's backing pointer is cached once in init() and written through on every async-write completion, but main never pinned it, so stream._writeState.buffer.transfer(0) could free the storage under the cached pointer. The fix pins the buffer via as_pinned_arraybuffer (the same idiom main already uses for the in/out buffers in write()), reads the pointer after pinning (FastTypedArray relocation), and stores the view in a new GC-visited writeState cached-value slot (added to zlib.classes.ts values:) so it survives even if user code nulls stream._writeState. Touches zlib.classes.ts (codegen), node_zlib_binding.rs (SAFETY comment + cached-accessor macro), the three Native{Zlib,Brotli,Zstd}.rs init() bodies, and adds a 3-codec subprocess regression test.
Security risks
The change reduces a memory-safety hazard (UAF / silent corruption via adversarial _writeState detach). I see no new injection, auth, or data-exposure surface. The one subtlety — that a pin alone doesn't root, so a non-GC-visited Cell<JSValue> would have introduced a freed-JSCell deref in close() — was raised in an earlier round and resolved by switching to the GC-visited cached-value slot; the current diff has no Cell<JSValue> field and no unpin in close_internal(), so that path is closed.
Level of scrutiny
This is native Rust at the JSC FFI boundary, caching a raw *mut u32 into a JS-owned ArrayBuffer for the lifetime of the handle. The soundness argument depends on three interacting mechanisms (pin blocks detach; GC-visited slot blocks collection; pointer re-read after pin handles FastTypedArray relocation). That's exactly the kind of reasoning where a second pair of human eyes is cheap insurance, even though the pattern mirrors what's already on main for pendingInput/pendingOutput. Not a rubber-stamp config change.
Other factors
The PR has been through several bot-review iterations (coderabbit + five rounds of my own inline comments), all of which are now addressed and resolved — including the NativeZstd validator-ordering nit, the GC-visited-slot fix, the it.concurrent.each test convention, and the SAFETY-comment sync in 25b48dd. The bug-hunting system found nothing on the current revision. CI build #58342 is referenced in the timeline. No outstanding human reviewer comments. Given all that, this is in good shape — I'm deferring purely on the "memory-safety in a runtime" criterion, not on any specific concern with the diff.
|
@robobun this was green earlier but CI went red after main moved (a single sub-job failing — looks flaky/infra). Please rebase onto main and re-run CI; if a test genuinely fails, investigate and fix it. |
Store the pinned Uint32Array in a new `writeState` cached value (zlib.classes.ts) instead of a non-visited Cell<JSValue> on the native struct, so the view stays alive for the wrapper's lifetime and close_internal() no longer dereferences a JSValue the GC may have collected (e.g. `s._writeState = null; Bun.gc(true); s.close()`). The pin still blocks transfer(); rooting makes the cached write_result pointer unable to dangle, and no unpin bookkeeping is needed. Also pin after the remaining argument validators in NativeZstd::init() so its error paths leave nothing pinned, matching NativeZlib/NativeBrotli.
…r.test.js The regression tests run in isolation alongside the other focused zlib lifetime tests (zlib-reset-race, zlib-onerror-reentrancy), keeping zlib.test.js untouched.
…d model The write_result SAFETY comment and the two init() comments still cited the user-breakable this._writeState / _handle[owner_symbol] path. Point them at the pin + GC-visited writeState slot instead — the robust mechanism this change relies on.
robobun
force-pushed
the
ali/zlib-arraybuffer-pin
branch
from
25b48dd to
6bacf38
Compare
2 participants
Narrowed from the original scope. main's hardening rounds independently fixed the primary
write()in/out ArrayBuffer UAF (now pinned viaas_pinned_arraybuffer), and thedictionaryis now copied (to_vec) rather than borrowed — so both are already safe on main.The one gap left is
_writeState. Its backing pointer (write_result) is cached once ininit()and written (2×u32) byflush_write_resultafter each async write, but main only validates the buffer — it never pins it. Sostream._writeState.buffer.transfer(0)still frees the storage under the cached pointer (debug: assertion/UAF; release: silent corruption).Pin
_writeStateininit()using the sameas_pinned_arraybufferidiom main already uses for the in/out buffers, reading the pointer after the pin (pinning relocates a small FastTypedArray), and store the view in a GC-visitedwriteStatecached value (zlib.classes.ts) so it stays alive for the wrapper's lifetime — the pin blocks detach, the slot blocks collection, so the cached pointer can never dangle and no unpin bookkeeping is needed. Adds_writeState-detach regression tests for inflate/brotli/zstd intest/js/node/zlib/zlib-writestate-transfer.test.js, alongside the other focused zlib lifetime tests.