node:zlib: re-resolve writeState buffer on each write instead of storing raw pointer

[robobun]

Problem

NativeZlib/NativeBrotli/NativeZstd capture a raw *mut u32 into the _writeState Uint32Array's backing store at init() time (write_result: Cell<Option<*mut u32>>) and write avail_in/avail_out through it after every write()/writeSync() (flush_write_result).

A typed array's vector is not a stable address:

• Accessing .buffer on a small Uint32Array migrates it from FastTypedArray (inline storage) to WastefulTypedArray (new heap allocation) — the vector moves.
• ArrayBuffer.prototype.transfer() / structuredClone(..., { transfer }) / postMessage transfer detach the buffer entirely — the storage is now owned by someone else (or freed once they're collected).

After either, the stored pointer is stale and flush_write_result writes 8 bytes into memory the handle no longer owns.

Repro

const zlib = require("zlib");
const z = zlib.createDeflate();
const handle = z._handle;

const state = new Uint32Array(8);
void state.buffer; // pin to WastefulTypedArray before init() captures the vector
handle.init(15, -1, 8, 0, state, () => {}, undefined);

handle.writeSync(0, Buffer.from("hi"), 0, 2, Buffer.alloc(1024), 0, 1024);

const stolen = new Uint32Array(state.buffer.transfer()); // we now own that memory
stolen.fill(0xdeadbeef);

handle.writeSync(0, Buffer.from("hi"), 0, 2, Buffer.alloc(1024), 0, 1024);
console.log(stolen[0].toString(16)); // prints '400' — native scribbled over it

Fix

• Add writeState to the codegen values[] list in zlib.classes.ts so the Uint32Array JSValue is cached on the wrapper (GC-visited), and to the codegen_cached_accessors! list in the __impl_compression_stream! macro.
• Drop the write_result: Cell<Option<*mut u32>> field from all three structs; init() now stores the JSValue via write_state_set_cached (keeping the existing Uint32Array/length validation).
• flush_write_result (called from run_from_js_thread and write_sync, always on the JS thread) re-resolves the cached JSValue → as_array_buffer → as_u32() and only writes when the buffer still has ≥ 2 elements, so a detached/migrated _writeState is a safe no-op instead of a wild write. A byte_len guard avoids reinterpreting a detached view's poisoned vector pointer.

(Originally implemented against the Zig bindings; rebased onto the Rust runtime after #30412 — the Rust port carried the same raw-pointer pattern, and the old Zig sources are no longer compiled, so the fix now targets node_zlib_binding.rs / NativeZlib.rs / NativeBrotli.rs / NativeZstd.rs only.)

Verification

New test test/js/node/zlib/zlib-writestate-detached.test.ts covers zlib sync, zlib async, brotli sync, and zstd sync. Each transfers the writeState backing store to a sentinel-filled buffer and asserts the native handle does not scribble over it.

Before fix: all 4 fail (CORRUPTED 0 400 / 3fe — the handle writes avail_out/avail_in into the transferred buffer). After fix: all 4 pass. The rest of test/js/node/zlib/ has an identical pass/fail set with and without this change (the only differences are the 4 new tests), and cargo check --workspace is clean.

Note: #28250 and the pendingInput/pendingOutput rooting added in #31221 address the in/out buffers passed to write(); this PR addresses the writeState buffer passed to init() — a separate stale-pointer path.

[coderabbitai]

Walkthrough

Replace raw-pointer caching of native write-state buffers for zlib/Brotli/Zstd with a JS-cached writeState typed-array that is validated and re-resolved before native updates; add writeState to the API surface and add tests that detach/transfer the backing buffer to validate safety.

Changes

WriteState Memory Safety

Layer / File(s)	Summary
API Exposure src/runtime/api/zlib.classes.ts	Adds "writeState" to the exported values array so instance write state is exposed alongside writeCallback, errorCallback, and dictionary.
Core Update Mechanism src/runtime/node/node_zlib_binding.zig	Adds updateWriteResult(this, globalThis, this_value) which fetches the cached JS writeState, converts/validates it as a Uint32Array with length ≥ 2, and calls this.stream.updateWriteResult with the resolved elements; replaces direct raw-pointer writes in runFromJSThread and writeSync.
Native Stream Init — Brotli src/runtime/node/zlib/NativeBrotli.zig	Removes the write_result raw pointer field; init now validates writeResult is an ArrayBuffer/Uint32Array with len ≥ 2 and registers it via js.writeStateSetCached(...), then caches the write callback.
Native Stream Init — Zlib src/runtime/node/zlib/NativeZlib.zig	Removes stored raw write_result pointer; init resolves writeResult as an ArrayBuffer view, validates Uint32Array length ≥ 2, and registers it using js.writeStateSetCached(...); writeCallback and dictionary caching remain.
Native Stream Init — Zstd src/runtime/node/zlib/NativeZstd.zig	Deletes the write_result pointer field; init validates writeState is a Uint32Array with len ≥ 2 and uses js.writeStateSetCached(...) instead of saving a raw pointer.
Tests / Validation test/js/node/zlib/zlib-writestate-detached.test.ts	Adds four fixtures and Bun test.concurrent cases (zlib deflate sync, zlib deflate async, Brotli sync, Zstd sync) that initialize with a Uint32Array writeState, perform writes, transfer/detach the backing ArrayBuffer and sentinel-fill it, perform further writes, and assert the detached memory is not corrupted and the child process prints OK with exit code 0.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: moving from storing a raw pointer to re-resolving the writeState buffer on each write to prevent memory corruption from detached/moved typed arrays.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description comprehensively documents the problem, fix, and verification, exceeding the template requirements.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[robobun]

^{Updated 6:55 AM PT - May 24th, 2026}

❌ @robobun, your commit aec7f30 has 1 failures in Build #57561 (All Failures):

• test/js/third_party/mongodb/mongodb.test.ts - pid 7744 error: addresssanitizer on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 30118

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

bun-30118 --bun

[github-actions]

Found 2 issues this PR may fix:

1. cant NOT reproduced, close it if not help #22567 - Segfault at low address in createGunzip() — a stale writeState pointer after TypedArray migration would produce exactly this kind of crash
2. Safari WebSocket crashes with perMessageDeflate when mixing server.publish() and ws.send() #25673 - WebSocket perMessageDeflate corruption when mixing server.publish() and ws.send() — rapid sequential zlib writes could trigger a TypedArray migration, causing stale avail_in/avail_out accounting via the old pointer

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #22567
Fixes #25673

🤖 Generated with Claude Code

[robobun]

Re the auto-linked issues above — not adding either to the PR description:

• cant NOT reproduced, close it if not help #22567 (createGunzip segfault at 0x52): the internal _writeState = new Uint32Array(2) never has .buffer accessed and is never detached on the normal .pipe() path, so this fix doesn't apply there. fix(zlib): prevent use-after-free in WorkPool compression operations #28250 (in/out buffer GC during async work) is the more plausible candidate and already claims it.
• Safari WebSocket crashes with perMessageDeflate when mixing server.publish() and ws.send() #25673 (Safari WebSocket perMessageDeflate): that's Bun.serve's uWS deflate path, which doesn't go through node:zlib / NativeZlib at all.

This PR fixes a distinct bug: the native handle writing through a stale pointer when user code reaches into _writeState and detaches or migrates its backing store.

[claude]

No issues found and the fix follows the existing cached-JSValue pattern (writeCallback/dictionary), but this is native memory-safety code on the hot path for every zlib/brotli/zstd write — worth a human pass on the JSC typed-array lifetime reasoning.

Extended reasoning...

Overview

This PR replaces a raw [*]u32 pointer (captured at init() time from the _writeState Uint32Array's backing store) with a GC-visited cached JSValue that is re-resolved to a slice on each write completion. It touches the codegen class definition (zlib.classes.ts), the shared CompressionStream mixin in node_zlib_binding.zig, and the three native handle structs (NativeZlib, NativeBrotli, NativeZstd). A new test file exercises detach-after-init for sync zlib, async zlib, and sync brotli.

Security risks

The PR removes a memory-safety hazard (write through a stale pointer after TypedArray vector migration or ArrayBuffer detachment) and adds input validation where there was previously an unconditional .? unwrap. The new updateWriteResult helper is only invoked on the JS thread (writeSync and runFromJSThread), so re-resolving the JSValue there is safe with respect to GC. I don't see any new risks introduced; the length check (slice.len < 2) makes detached/short buffers a no-op rather than an OOB write.

Level of scrutiny

High. This is native Zig ↔ JSC interop on a path that runs for every node:zlib write, and the correctness argument depends on JSC-specific behavior (FastTypedArray → WastefulTypedArray transitions, asArrayBuffer/asU32() returning empty on detach, codegen values[] GC-visiting semantics). The change is small and pattern-matches the existing writeCallback/errorCallback/dictionary cached values, but a maintainer familiar with Bun's bindings codegen should confirm the lifetime reasoning.

Other factors

The bug-hunting system found no issues. My earlier nit (use test.concurrent) was addressed in e063239. The author reports the full existing zlib test suite and Node compat test-zlib*.js suite still pass. No CODEOWNERS cover these paths. Given the guidelines to not auto-approve changes to critical/memory-safety code paths, I'm deferring rather than approving.

[claude]

🟣 Pre-existing, not introduced by this PR — but the dictionary argument one line down has the identical stale-pointer hazard this PR fixes for writeState: init() captures a raw byteSlice() into the typed array's backing store and stores it long-term in Context.dictionary, which setDictionary() (via reset()) and doWorkInflate() later dereference. js.dictionarySetCached keeps the JSValue GC-rooted but doesn't stop the backing store moving (FastTypedArray→WastefulTypedArray) or being detached via transfer()/postMessage, leaving inflateSetDictionary/deflateSetDictionary reading freed memory. Note the writeState-style fix won't suffice here since doWorkInflate runs on a WorkPool thread — the dictionary bytes need to be copied into native-owned memory at init time. Worth a follow-up since you're already in this function fixing this exact pattern.

Extended reasoning...

What the bug is

NativeZlib.init() at line 86 captures a raw byte slice into the user-supplied dictionary typed array's backing store:

const dictionary = if (arguments[6].isUndefined()) null else arguments[6].asArrayBuffer(globalThis).?.byteSlice();

and Context.init() stores it long-term:

this.dictionary = dictionary orelse "";

This []const u8 is then dereferenced well after init() returns:

• setDictionary() (called from reset() on the JS thread): c.deflateSetDictionary(&this.state, dict.ptr, @intCast(dict.len)) / c.inflateSetDictionary(...).
• doWorkInflate() (called from doWork(), which for async write() runs on a WorkPool worker thread): when zlib returns Z_NEED_DICT, it calls c.inflateSetDictionary(&this.state, this.dictionary.ptr, @intCast(this.dictionary.len)).

This is exactly the pattern this PR's description identifies as unsafe for writeState, applied to the adjacent dictionary argument in the same init() function.

Why dictionarySetCached doesn't help

js.dictionarySetCached(this_value, globalThis, arguments[6]) keeps the JSValue GC-rooted, so the typed-array object won't be collected. But, as the PR description explains, that does not stabilize the backing store:

1. Accessing .buffer on a small typed array transitions it FastTypedArray → WastefulTypedArray and moves the vector to a new heap allocation.
2. ArrayBuffer.prototype.transfer() / structuredClone(..., {transfer}) / postMessage transfer detaches the buffer; the storage is now owned elsewhere (or freed once the new owner is collected).

After either, Context.dictionary is a dangling slice. The JS wrapper (src/js/node/zlib.ts) accepts any ArrayBufferView/ArrayBuffer for dictionary and passes it through without copying, so user code fully controls this buffer.

Step-by-step proof

1. const dict = Buffer.from("some-dictionary-bytes");
2. const z = zlib.createInflate({ dictionary: dict }); → NativeZlib.init() runs, captures dict.byteSlice() into Context.dictionary, and caches the JSValue via dictionarySetCached.
3. dict.buffer.transfer(); → the ArrayBuffer backing dict is detached. The cached JSValue is still alive (length 0), but Context.dictionary.ptr now points at freed/repurposed memory.
4. z.write(deflatedDataRequiringDict) → async path schedules AsyncJob.run on the WorkPool.
5. On the worker thread, doWork() → doWorkInflate() → c.inflate() returns Z_NEED_DICT → c.inflateSetDictionary(&this.state, this.dictionary.ptr, this.dictionary.len) reads dict.len bytes through a dangling pointer, off the JS thread.

The same dangling read also occurs on the JS thread via reset() → setDictionary().

Impact

Use-after-free read in zlib's inflateSetDictionary/deflateSetDictionary. Best case: garbage dictionary contents and bogus inflate output / Z_DATA_ERROR. Worst case: read of unmapped memory and a crash, potentially on a worker thread. Same severity class as the writeState bug this PR fixes (that one is a wild write; this one is a wild read).

Fix

The writeState-style fix (cache the JSValue, re-resolve at use time) is not sufficient here, because doWorkInflate() runs off the JS thread and cannot safely call asArrayBuffer(globalThis). Instead, the dictionary bytes should be copied into native-owned memory at init() time (e.g. bun.default_allocator.dupe(u8, ...)), stored in Context.dictionary, and freed in close()/deinit(). That makes the slice independent of the JS heap and removes the need for dictionarySetCached entirely.

This is pre-existing — the PR doesn't touch the dictionary line and doesn't make it worse. Flagging only because it's the same bug class, in the same init() function, with the same root cause the PR description articulates; a small follow-up would close the gap.

[robobun]

Agreed — noted this while working on the writeState fix. Leaving it out of this PR because the shape of the fix is different: doWorkInflate dereferences dictionary on a WorkPool thread, so re-resolving the JSValue at use time (the approach here) is not available. The dictionary bytes need to be copied into a native-owned buffer in init() and freed in close()/deinit(), which is its own change with its own test. Will follow up separately.

[claude]

🟣 Pre-existing, non-blocking: while you're here, there are two more unconditional .? unwraps in these same init() functions — arguments[0].asArrayBuffer(globalThis).?.asU32() for the brotli params (NativeBrotli.zig:92) and arguments[6].asArrayBuffer(globalThis).?.byteSlice() for the zlib dictionary (NativeZlib.zig:86, only undefined is guarded). Both panic on a non-buffer value. They're only reachable via direct _handle.init() misuse (the JS wrapper always passes the right types), so feel free to leave for a follow-up.

Extended reasoning...

What this is

This PR replaces the unconditional .? unwrap on writeResult with proper orelse validation, and the description notes "add the missing argument validation in init() (previously an unconditional .? unwrap)". A few lines below the diff hunk in the same functions, two more unconditional unwraps remain:

• NativeBrotli.zig:92 — const params_ = arguments[0].asArrayBuffer(globalThis).?.asU32();
• NativeZlib.zig:86 — const dictionary = if (arguments[6].isUndefined()) null else arguments[6].asArrayBuffer(globalThis).?.byteSlice(); (guards undefined but not null/number/string/etc.)

How it triggers

asArrayBuffer returns null when the value is not an ArrayBufferView, so .? panics on the optional unwrap. Concretely:

const z = require('zlib').createBrotliCompress();
z._handle.init(123, new Uint32Array(2), () => {});
// -> Zig panic: attempt to use null value (params unwrap)

const d = require('zlib').createDeflate();
d._handle.init(15, -1, 8, 0, new Uint32Array(2), () => {}, 123);
// -> Zig panic: attempt to use null value (dictionary unwrap)

In safe builds this is a process abort; in release-fast it's UB.

Why this is pre-existing and non-blocking

These lines are not in the diff — the PR neither introduces them nor adds new callers that could pass bad types. _handle.init() is an internal binding called only by Bun's own node:zlib JS wrapper, which always passes a Uint32Array for params and either undefined or a Buffer for dictionary. Triggering this requires deliberately reaching past the public API into _handle and passing garbage; Node's own native bindings also abort on that kind of misuse. So this is hardening of an internal surface, not a user-facing bug, and the PR's actual fix (the stale writeState pointer) is correct and complete on its own.

Addressing the "out of scope" objection

One reasonable view is that this is "while you're here" scope creep: the PR description's "an unconditional .? unwrap" is singular and clearly refers to the specific writeResult line being rewritten, not a general validation pass. That's fair, and is exactly why this is filed as pre-existing / non-blocking rather than a change request — it's just flagging that two siblings of the line you fixed have the same shape, since you've already got the orelse return globalThis.throwInvalidArgumentTypeValue(...) pattern at hand. Perfectly fine to defer to a separate cleanup PR.

Suggested fix (if you choose to take it)

Mirror the new writeResult handling:

// NativeBrotli.zig
const params_buf = arguments[0].asArrayBuffer(globalThis) orelse {
    return globalThis.throwInvalidArgumentTypeValue("params", "Uint32Array", arguments[0]);
};
const params_ = params_buf.asU32();

// NativeZlib.zig
const dictionary = if (arguments[6].isUndefinedOrNull()) null else (arguments[6].asArrayBuffer(globalThis) orelse {
    return globalThis.throwInvalidArgumentTypeValue("dictionary", "Buffer, TypedArray, or DataView", arguments[6]);
}).byteSlice();

NativeZstd.zig already validates its initParamsArray with orelse, so it's only these two.

[robobun]

Ack — both are pre-existing and only reachable via direct _handle.init() misuse (the JS wrapper always passes correct types). Leaving for a follow-up to keep this change scoped to the writeState stale-pointer fix; the zlib dictionary one will get proper validation as part of the copy-into-native fix mentioned in the other thread.

[robobun]

Status (build #57561 @ aec7f30, rebased onto the Rust runtime after #30412):

• The fix + 4 regression tests are green: zlib/brotli/zstd suites pass on every lane that ran, including the x64-asan shards that execute test/js/node/zlib/.
• The only red shard is test/js/third_party/mongodb/mongodb.test.ts on one x64-asan shard — an ASAN heap-use-after-free at process teardown in a TLS MongoClient test. That test configures no wire compression and never touches node:zlib, so it's unrelated to this diff (the zlib test in the same shard passed under ASAN). The other warning (update_interactive_install.test.ts on Windows) passed on retry.
• Local verification on the new base: all 4 new tests fail on an unfixed build (CORRUPTED 0 400) and pass with the fix; the rest of test/js/node/zlib/ has an identical pass/fail set with and without the change; cargo check --workspace is clean.

This needs a maintainer to review/merge — the remaining CI red is an unrelated flake on a lane this PR doesn't touch.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/runtime/node/zlib/NativeBrotli.zig (1)

92-92: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Unconditional .? unwrap on params can panic if argument is invalid.

The writeResult argument now has proper validation with orelse, but params_ on line 92 still uses .? which will panic if arguments[0] is not an ArrayBuffer. This is inconsistent with the improved validation pattern.

🛡️ Proposed fix to add validation

-    const params_ = arguments[0].asArrayBuffer(globalThis).?.asU32();
+    const params_ = (arguments[0].asArrayBuffer(globalThis) orelse {
+        return globalThis.throwInvalidArgumentTypeValue("params", "Uint32Array", arguments[0]);
+    }).asU32();

🤖 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/runtime/node/zlib/NativeBrotli.zig` at line 92, params_ is
unconditionally unwrapped with .? which can panic if arguments[0] is not an
ArrayBuffer; update the extraction of params_ to mirror the validated pattern
used for writeResult by using orelse to return or propagate a clear error (or
handle the invalid argument) instead of unwrapping—specifically change the call
chain involving arguments[0].asArrayBuffer(globalThis).?.asU32() to use
asArrayBuffer(...).orelse(...) and then asU32() (or validate the asU32 result)
so the code safely handles invalid or missing arguments without panicking.

🤖 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/zlib/zlib-writestate-detached.test.ts`:
- Around line 112-153: Add a parallel zstd test mirroring the brotli fixture:
create a zstdFixture that uses zlib.createZstdCompress(), captures z._handle,
calls handle.init(params, state, () => {}), performs handle.writeSync(...),
transfers state.buffer and mutates the stolen Uint32Array, then calls writeSync
again and asserts the transferred buffer was not written through; add a
test.concurrent titled like "zstd: writeSync does not write through stale
writeState pointer after detach" that spawns Bun with the zstdFixture and
asserts stdout/stderr/exitCode the same way the brotli test does. Ensure you
reference createZstdCompress, handle.init, handle.writeSync, and
state.buffer.transfer() in the new fixture so it covers NativeZstd the same way
the existing test covers NativeBrotli.

---

Outside diff comments:
In `@src/runtime/node/zlib/NativeBrotli.zig`:
- Line 92: params_ is unconditionally unwrapped with .? which can panic if
arguments[0] is not an ArrayBuffer; update the extraction of params_ to mirror
the validated pattern used for writeResult by using orelse to return or
propagate a clear error (or handle the invalid argument) instead of
unwrapping—specifically change the call chain involving
arguments[0].asArrayBuffer(globalThis).?.asU32() to use
asArrayBuffer(...).orelse(...) and then asU32() (or validate the asU32 result)
so the code safely handles invalid or missing arguments without panicking.

🪄 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: 5705aa2c-c542-40a3-ba33-e9fad654958a

📥 Commits

Reviewing files that changed from the base of the PR and between 0a7bed5 and b28fd36.

📒 Files selected for processing (6)

• src/runtime/api/zlib.classes.ts
• src/runtime/node/node_zlib_binding.zig
• src/runtime/node/zlib/NativeBrotli.zig
• src/runtime/node/zlib/NativeZlib.zig
• src/runtime/node/zlib/NativeZstd.zig
• test/js/node/zlib/zlib-writestate-detached.test.ts

[coderabbitai]

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/zlib/zlib-writestate-detached.test.ts`:
- Around line 124-129: Add the same pre-detach sanity check used in the zlib
sync fixture to the Brotli and Zstd tests: after calling handle.writeSync(0,
inBuf, 0, inBuf.length, outBuf, 0, outBuf.length) but before calling
state.buffer.transfer(), read the shared buffer (e.g., const sanity = new
Uint32Array(state.buffer);) and assert that the writeState was updated (e.g.,
sanity.some(v => v !== 0) or a specific expected value) so the test fails if
writeSync never modified the buffer; apply this insertion for both the Brotli
case around the inBuf/outBuf/writeSync block and the Zstd case at the
corresponding location mentioned in the comment.
- Around line 78-90: The onerror handler currently swallows native errors and
the console.log+process.exit pattern can lose buffered output; update the empty
handle.onerror to immediately write an error message and exit (e.g., write
details about the error and call process.exit(1)), and replace the success
console.log("OK") + process.exit(0) and any other console.log usages in the
async callbacks with process.stdout.write calls that use the callback to call
process.exit only after the write is flushed; keep the existing stolen[]
corruption check inside handle.init's callback and ensure both success and error
paths flush stdout/stderr before exiting.

🪄 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: 71f13417-d1f6-4af9-b1ab-2ab9ad5a5f0a

📥 Commits

Reviewing files that changed from the base of the PR and between b28fd36 and f661f94.

📒 Files selected for processing (1)

• test/js/node/zlib/zlib-writestate-detached.test.ts

[claude]

No issues found — the fix looks correct and follows the existing values[] codegen pattern, but since this is native memory-safety code at the JS/native boundary I'd like a human to sign off.

Extended reasoning...

Overview

This PR removes the raw write_result: ?[*]u32 pointer field from NativeZlib/NativeBrotli/NativeZstd and replaces it with a cached JSValue (via the codegen values[] mechanism, alongside the existing writeCallback/errorCallback/dictionary slots). A new updateWriteResult helper in CompressionStream re-resolves the typed array's backing store on each write completion (both runFromJSThread and writeSync, always on the JS thread) and length-checks before writing, turning a detached/migrated buffer into a safe no-op instead of a wild 8-byte write. Each init() gains orelse-based validation in place of the old unconditional .? unwrap. Four new subprocess tests cover zlib sync/async, brotli sync, and zstd sync.

Security risks

The PR fixes a memory-safety hazard (write through a stale pointer into freed/transferred memory). The new code only dereferences the typed-array vector after re-resolving it via asArrayBuffer and checking asU32().len >= 2, and only on the JS thread, so I don't see new UAF or OOB surface introduced. The added writeState entry in values[] is GC-visited by the same generated machinery that already handles writeCallback etc., so it shouldn't introduce a GC hazard.

Level of scrutiny

This is native Zig code at the JavaScriptCore interop boundary, changing how a hot path (every write/writeSync) accesses a JS-owned buffer. The pattern is established and the diff is small and mechanical, but correctness here depends on subtleties of JSC typed-array storage semantics and the codegen's GC-visiting of cached values — the kind of thing a maintainer familiar with Bun's native bindings should confirm.

Other factors

All prior review feedback has been addressed: the test.concurrent nit was applied, zstd test coverage was added, and the two pre-existing issues I flagged (dictionary stale-pointer, remaining .? unwraps) are explicitly out of scope with follow-ups acknowledged. No bugs were found by the bug-hunting system on the current revision. CI's only failure is a known-flaky bake/HMR test unrelated to zlib. None of the touched files have a CODEOWNER.

[claude]

I didn't find any issues — the fix is well-scoped, follows the existing values[]/*SetCached pattern, and has solid fail-before/pass-after coverage across all three backends. Deferring only because this is a native memory-safety change on the JSC↔Zig boundary in a hot path (every zlib/brotli/zstd write completion), which is worth a human sign-off.

Extended reasoning...

Overview

This PR fixes a use-after-free / wild-write in node:zlib's native bindings (NativeZlib/NativeBrotli/NativeZstd). Previously, init() captured a raw [*]u32 into the _writeState Uint32Array's backing store and wrote avail_in/avail_out through it after every write. Since a typed array's vector can move (FastTypedArray→WastefulTypedArray) or be freed (detach via transfer()/postMessage), this was a stale-pointer write. The fix drops the write_result: ?[*]u32 field, adds writeState to the codegen values[] list (GC-visited JSValue cache), and re-resolves the backing store via asArrayBuffer().asU32() at write-completion time, length-checked so a detached buffer becomes a safe no-op. It also replaces an unconditional .? unwrap with proper validation.

Files touched: zlib.classes.ts (codegen +1 entry), node_zlib_binding.zig (new updateWriteResult helper, 2 call sites), three Native*.zig files (drop field, validate + cache JSValue in init()), and a new 4-fixture regression test.

Security risks

This is itself a memory-safety hardening — it removes a wild 8-byte write reachable via _handle misuse. The replacement is strictly safer: re-resolution happens only on the JS thread (both runFromJSThread and writeSync), asU32() returns an empty slice for detached buffers, and the len < 2 guard prevents OOB. No new attack surface; the new throwInvalidArgumentTypeValue paths are only reachable via direct _handle.init() misuse. I see no injection, auth, or data-exposure concerns.

Level of scrutiny

Moderate-to-high. The diff is small and mechanical, and the new caching pattern exactly mirrors the existing writeCallback/errorCallback/dictionary handling in the same file, so the risk of regression is low. However, this is native Zig at the JSC boundary, on the hot path of every compression-stream write, and the correctness argument depends on JSC typed-array internals (vector stability, detach semantics). That domain knowledge is exactly where a maintainer's eye adds value over automated review.

Other factors

• All prior review feedback (mine and CodeRabbit's) has been addressed: test.concurrent, zstd coverage, and pre-detach sanity checks are in. The two open inline threads are explicitly pre-existing/non-blocking follow-ups (dictionary stale-pointer, remaining .? unwraps) that the author has acknowledged for a separate PR.
• Test coverage is strong: four subprocess fixtures (zlib sync/async, brotli sync, zstd sync) that demonstrably fail on the unfixed binary (CORRUPTED 0 400) and pass with the fix; existing zlib suites reportedly pass.
• CI: author reports full-matrix green except known unrelated flakes; latest build #51522 in flight at last check.
• No CODEOWNERS conflict apparent for these paths.

[claude]

No new issues from my side — earlier feedback is all addressed/resolved — but this rewrites how the native write-completion path touches JS-heap-backed memory across all three compression backends, so I'd like a human to sign off on the JSC/typed-array lifetime reasoning.

Extended reasoning...

Overview

This PR fixes a stale-pointer bug in NativeZlib/NativeBrotli/NativeZstd: instead of capturing a raw *mut u32 into the _writeState Uint32Array's backing store at init() time, it caches the JSValue on the wrapper (via the codegen values[] list) and re-resolves it to a slice on every write completion. The write_result field is removed from all three structs, flush_write_result now takes (global, this_value) and length-checks before writing, and the __impl_compression_stream! macro grows a write_state_get_cached accessor. A new test file with four subprocess fixtures (zlib sync/async, brotli sync, zstd sync) verifies that detaching the writeState backing store via .buffer.transfer() no longer results in the native handle scribbling over the transferred memory.

Security risks

The bug being fixed is itself a memory-safety hazard (8-byte wild write through a stale pointer into JS-heap-managed storage). The fix strictly reduces attack surface — the new path re-resolves the typed array on each call, checks byte_len >= 8, and turns a detached buffer into a no-op. I don't see new injection, auth, or data-exposure surface introduced; the only new behavior is that init() now throws ERR_INVALID_ARG_TYPE/ERR_INVALID_ARG_VALUE instead of panicking on a bad writeResult, which is a hardening improvement.

Level of scrutiny

This warrants careful human review: it's native Rust on the hot path of every node:zlib write, and correctness depends on JSC typed-array lifetime semantics (FastTypedArray→WastefulTypedArray vector migration, detach behavior of as_array_buffer/as_u32 returning empty for detached views). The reasoning in the PR description and code comments is thorough and matches my understanding, and the fail-before/pass-after tests are convincing, but I'd rather a human with JSC-heap expertise confirm the re-resolve approach is sound for both call sites (runFromJSThread and writeSync, both JS-thread-only).

Other factors

All prior review threads are resolved: my test.concurrent nit was applied, the zstd fixture and brotli/zstd pre-detach sanity checks were added per CodeRabbit, and my two pre-existing observations (the analogous dictionary stale-slice hazard and the remaining .? unwraps) were explicitly acknowledged as out-of-scope follow-ups (the dictionary one needs a different fix shape since it's dereferenced off-thread). The bug-hunting system found nothing on this revision, and the author reports green CI across the zlib/brotli/zstd suites plus the Node-compat test-zlib*.js set.

[robobun]

Superseded by 684f2af (#31606), which landed the same fix on the Rust runtime: the raw write_result: Cell<Option<*mut u32>> field is removed from all three handles, init() caches the typed array as the writeResult JSValue, and flush_write_result re-resolves it on every write completion with detach/length guards — plus a regression test for the detached-writeState case in test/js/node/zlib/zlib-handle-bounds-check.test.ts. Nothing left for this PR to add, so closing.