Deduplicate zlib/brotli/zstd binding helpers - #31996
Conversation
|
Updated 7:16 PM PT - Aug 10th, 2026
✅ @robobun, your commit 87a9140c0f33a3da8a64f170a0959caf9069be96 passed in 🧪 To try this PR locally: bunx bun-pr 31996That installs a local version of the PR into your bun-31996 --bun |
|
@robobun adopt |
WalkthroughThis PR centralizes zlib argument validation, updates native Brotli, zlib, and Zstd streams, unifies zlib initialization and output growth, narrows FFI exports, removes an unused dependency, and adds validation and lifecycle tests. ChangesZlib validation and compression stream unification
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Adopted, brought up to date with main, and mergeable at 87a9140. Scope: three-way dedup of the binding validation helpers, a shared write/writeSync argument parser, removal of the dead zlib_sys posix/win32 split (now also guarded by the orphaned-files source lint), zstd validate-before-allocate (regression test fails on main), and 32-bit guards on both avail_in and avail_out in the zlib crate. All review threads resolved. Verified on this head: handle tests 48/48, zlib.test.js, zstd.test.ts, fetch decompression regressions, source lints 75/75, cargo check + clippy including the Windows target. Buildkite build 91826 running. |
![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
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/NativeZstd.rs (1)
135-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
initParamsArraybefore initializing Zstd state.
validate_uint32_array(...)can still throw afterwriteState/callback caching and afters.init()allocates a CCtx/DCtx. That leaves the instance half-initialized, and a later retry can leak the first context becauseContext::init()just replacesself.state.Suggested fix
let init_params_array_value = arguments[0]; let pledged_src_size_value = arguments[1]; let write_state_value = arguments[2]; let process_callback_value = arguments[3]; validate_write_result_array(global, write_state_value, "writeState")?; + let mut params_ = + validate_uint32_array(global, init_params_array_value, "initParamsArray")?; js::write_result_set_cached(this_value, global, write_state_value); let write_js_callback = validators::validate_function(global, "processCallback", process_callback_value)?; @@ if err.is_error() { CompressionStream::<Self>::emit_error(self, global, this_value, err); return Ok(JSValue::FALSE); } - let mut params_ = - validate_uint32_array(global, init_params_array_value, "initParamsArray")?; for (i, &x) in params_.as_u32().iter().enumerate() {As per coding guidelines, untrusted input must be validated before any processing, allocation, or side effect.
🤖 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/NativeZstd.rs` around lines 135 - 164, The code validates initParamsArray after caching writeState/callback and after calling self.stream.with_mut(|s| s.init(...)), which can allocate and leave the instance half-initialized if validate_uint32_array throws; move the validate_uint32_array(global, init_params_array_value, "initParamsArray") call to before any side-effects or allocations (i.e., before calling self.stream.with_mut(|s| s.init(pledged_src_size)) and preferably before js::write_callback_set_cached/js::write_result_set_cached) so all untrusted input is validated first; update references to init_params_array_value and params_ accordingly and keep CompressionStream::emit_error handling for init failures.Source: Coding guidelines
🤖 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 402-467: In parse_write_args, don't cast JS values to u32 using
the shim jsv_to_u32 before validating them; instead call the existing
validate_uint32 on the flush, in_off, in_len, out_off, and out_len argument
values (the same argument indices used around jsv_to_u32) to ensure they are
finite unsigned 32-bit integers, then convert to u32 for further checks; keep
the existing flush_value_is_valid(flush) and the buffer bound checks for
in_buf.byte_len and out_buf.byte_len but use the validated/converted values
(refer to the parse_write_args function, the jsv_to_u32 shim, and
validate_uint32 helper).
In `@src/zlib/lib.rs`:
- Around line 773-778: The call in
ZlibCompressorArrayList::init_with_list_allocator uses
uLong::try_from(input.len()).expect("int cast") before calling deflateBound,
which can panic on platforms where uLong is 32-bit; replace the expect with a
checked conversion that returns Err(ZlibError::InvalidArgument) (or the
appropriate existing ZlibError variant) when input.len() does not fit into
zlib's uLong, and only call deflateBound with the safely converted value
(reference: deflateBound, zlib_reader.zlib).
---
Outside diff comments:
In `@src/runtime/node/zlib/NativeZstd.rs`:
- Around line 135-164: The code validates initParamsArray after caching
writeState/callback and after calling self.stream.with_mut(|s| s.init(...)),
which can allocate and leave the instance half-initialized if
validate_uint32_array throws; move the validate_uint32_array(global,
init_params_array_value, "initParamsArray") call to before any side-effects or
allocations (i.e., before calling self.stream.with_mut(|s|
s.init(pledged_src_size)) and preferably before
js::write_callback_set_cached/js::write_result_set_cached) so all untrusted
input is validated first; update references to init_params_array_value and
params_ accordingly and keep CompressionStream::emit_error handling for init
failures.
🪄 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: 6c206be3-e6b4-44e6-a30e-82332eb40519
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
src/runtime/node/node_zlib_binding.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rssrc/zlib/Cargo.tomlsrc/zlib/lib.rssrc/zlib_sys/lib.rssrc/zlib_sys/posix.rssrc/zlib_sys/win32.rs
💤 Files with no reviewable changes (4)
- src/zlib_sys/lib.rs
- src/zlib_sys/posix.rs
- src/zlib/Cargo.toml
- src/zlib_sys/win32.rs
…test binding validation Context::close() for NativeZlib/NativeBrotli/NativeZstd now tolerates a handle whose init() never ran (failed argument validation, or init was never called): Brotli unwrapped a None state pointer, Zstd passed NULL to ZSTD_*Ctx_reset, and Zlib tripped its debug assert on deflateEnd's StreamError. GC finalization of such a handle crashed the process. ZlibReaderArrayList/ZlibCompressorArrayList init now rejects inputs whose length does not fit zlib's 32-bit avail_in instead of silently truncating (and panicking on the Windows deflateBound cast, where uLong is 32-bit). Adds native-handle tests pinning the argument validation contract of the consolidated helpers and the never-initialized lifecycle.
There was a problem hiding this comment.
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/NativeZstd.rs (1)
157-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
initParamsArraybefore creating the ZSTD context.
self.stream.with_mut(|s| s.init(pledged_src_size))runs beforevalidate_uint32_array(...). IfinitParamsArrayis the wrong typed array, this method throws afterZSTD_create{C,D}Ctxhas already succeeded, so the handle stays partially initialized until an explicitclose()or later GC finalization.♻️ Suggested fix
- let err = self.stream.with_mut(|s| s.init(pledged_src_size)); + let mut params_ = + validate_uint32_array(global, init_params_array_value, "initParamsArray")?; + + let err = self.stream.with_mut(|s| s.init(pledged_src_size)); if err.is_error() { CompressionStream::<Self>::emit_error(self, global, this_value, err); return Ok(JSValue::FALSE); } - - let mut params_ = - validate_uint32_array(global, init_params_array_value, "initParamsArray")?; for (i, &x) in params_.as_u32().iter().enumerate() {As per coding guidelines, "Validate untrusted input BEFORE any processing, allocation, or side effect."
🤖 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/NativeZstd.rs` around lines 157 - 165, Validate the incoming initParamsArray before allocating or initializing the ZSTD context: call validate_uint32_array(global, init_params_array_value, "initParamsArray") and handle/return its Err result before invoking self.stream.with_mut(|s| s.init(pledged_src_size)) (and before any ZSTD_create*Ctx side-effects); then use the validated params_ in the later loop. This ensures the ZSTD context (created via s.init / ZSTD_createCCtx/ZSTD_createDCtx) is only created after untrusted input is checked and avoids leaving a partially initialized handle when validation fails.Source: Coding guidelines
🤖 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-handle-bounds-check.test.ts`:
- Around line 286-287: Add an assertion that stderr is empty before checking
stdout/exitCode: after the destructuring of [stdout, stderr, exitCode] (the
Promise.all call), insert expect(stderr.trim()).toBe("") and then assert stdout
and exitCode as currently done (or replace the single object equality with
ordered assertions: expect(stdout.trim()).toBe("survived");
expect(exitCode).toBe(0)). This ensures any unexpected stderr output is caught;
reference the variables stdout, stderr, exitCode from the test's Promise.all
destructuring.
---
Outside diff comments:
In `@src/runtime/node/zlib/NativeZstd.rs`:
- Around line 157-165: Validate the incoming initParamsArray before allocating
or initializing the ZSTD context: call validate_uint32_array(global,
init_params_array_value, "initParamsArray") and handle/return its Err result
before invoking self.stream.with_mut(|s| s.init(pledged_src_size)) (and before
any ZSTD_create*Ctx side-effects); then use the validated params_ in the later
loop. This ensures the ZSTD context (created via s.init /
ZSTD_createCCtx/ZSTD_createDCtx) is only created after untrusted input is
checked and avoids leaving a partially initialized handle when validation fails.
🪄 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: a98753f7-bb5e-4505-b91d-a7c03867b1ac
📒 Files selected for processing (5)
src/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rssrc/zlib/lib.rstest/js/node/zlib/zlib-handle-bounds-check.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… subprocess stderr Moves the initParamsArray type check ahead of ZSTD context creation in NativeZstd::init so a type error cannot leave a half-initialized handle, and folds stderr into the lifecycle test's combined assertion so a crash regression shows the panic text in the failure diff.
# Conflicts: # src/zlib/lib.rs
There was a problem hiding this comment.
Both earlier nits are addressed and the bug-hunting pass is clean; deferring only because this is a sizeable refactor of unsafe native compression bindings (zlib/brotli/zstd close/init paths) that's worth a human look before merge.
Extended reasoning...
Overview
Consolidates duplicated argument-validation and write-arg-parsing helpers across NativeZlib/NativeBrotli/NativeZstd, removes the dead zlib_sys posix/win32 split and the unused ZlibReader (+bun_io dep), adds map_init_return_code/regrow_output_tail helpers in src/zlib/lib.rs, and fixes pre-existing crashes when close()/GC-finalize runs on a handle whose init() never succeeded. New tests pin the validation contract and the lifecycle fix.
Since my last review
Both prior inline comments are resolved: the lifecycle subprocess test now asserts {stdout, stderr, exitCode} together, and the PR description's final paragraph was rewritten to accurately describe the c67f705 initParamsArray reorder. The only new commit is the main merge (3cafa81). No bugs surfaced in the current bug-hunting pass.
Security risks
None identified. The changes tighten validation (reject >4 GiB inputs instead of truncating, guard null-state close()) rather than relaxing it. No auth/crypto/permission surface.
Level of scrutiny
Medium-high. This is production runtime code with unsafe FFI (zlib/brotli/zstd state lifecycle, raw buffer pointers into JS-owned ArrayBuffers), and the diff includes behavioral changes beyond pure deduplication: close() null-state guards in three backends, the zstd initParamsArray validation reorder, and the 32-bit avail_in rejection. The consolidation itself reads as message-for-message faithful and the new tests cover it well, but the scope (~11 files, several hundred net lines across unsafe native bindings) puts it outside what I'd auto-approve.
Other factors
robobun adopted/verified on a debug build with the full zlib suite + Windows cross-target check; all CodeRabbit threads are resolved. No outstanding reviewer asks.
# Conflicts: # src/runtime/node/zlib/NativeBrotli.rs # src/runtime/node/zlib/NativeZstd.rs
|
Merged main and resolved the conflicts. Two of the three were real overlaps: #32762 landed the same never-initialized close() guards this PR carried, so I took main's versions and dropped the duplicates here; #32637 added crc32_bytes and the DeflateEncoder/InflateDecoder wrappers next to code this PR deletes, and those are preserved. Details are in the updated PR description. All 39 tests in the zlib handle suite (this PR's plus #32762's) and the rest of the compression suites pass on the merged head, and the Windows cross-target check compiles clean. |
…ed handle After init() throws on an invalid initParamsArray, a write on the handle must move no bytes. Validating after ZSTD context creation left a usable encoder configured with default parameters behind the thrown error.
# Conflicts: # src/zlib/lib.rs
|
Merged main again (5310acc). The conflict was in src/zlib/lib.rs: #33909 switched error machinery to per-crate thiserror enums (dropped the named_error_set macro call accordingly) and updated the ZlibReader impl this PR deletes; the deletion stays. Verified: all 40 zlib handle tests and the rest of the compression suites pass on the merged head, the gate's fail-before test still fails on main's src and passes here, and the Windows cross-target check compiles clean. |
|
@robobun get this mergeable — or close it if the changes are already covered on main / too stale to salvage. |
|
Checked against current main before deciding: none of this is covered there yet. Main still has the three copies of the constructor mode check and the writeResult checks, both copies of the write/writeSync argument parsing, the zlib_sys posix/win32 split, the zstd init ordering, and the truncating input-length casts. So it is worth salvaging rather than closing. Bringing it up to date now: four conflicts, mostly from #34427 (dictionaries) and #36555 (growable SAB) adding new validation to the write path that the consolidated parser has to absorb. Will post when it is green. |
# Conflicts: # src/runtime/node/node_zlib_binding.rs # src/runtime/node/zlib/NativeZstd.rs # src/zlib/lib.rs # src/zlib_sys/win32.rs
|
Up to date with main as of 926eba0 and mergeable again; CI is running on build 91663. What the merge changed: main had moved flush validation onto the per-codec trait (#34427) and added the resizable-ArrayBuffer rejection to the async write path only (#36555), so parse_write_args is now generic over the stream type and takes a reject_resizable flag that write passes as true and writeSync as false, keeping that deliberate difference visible. Main's dead-code sweeps had already removed ZlibReader, so that part of the PR dropped out; the rest (the three-way helper dedup, the write/writeSync parser, the zlib_sys posix/win32 removal, the zstd init ordering, and the input-length guards) is still not on main. Description rewritten to describe only what remains. Verified on the merged head: 48/48 in the handle test file (the zstd test still fails with src reverted to main), zlib.test.js 387 pass, all 61 upstream test-zlib-* files, zstd.test.ts, and the fetch decompression regressions; cargo check and clippy clean including the x86_64-pc-windows-msvc target. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it consolidates the buffer-bounds validation on the native write/writeSync path across three codecs, reorders zstd's init sequence, and deletes cross-platform FFI surface, a maintainer look would still be worthwhile.
What was reviewed:
parse_write_argstraced against both originalwrite/writeSyncbodies — validation order, error codes/messages, and thereject_resizablesplit match the pre-refactor code;in_validated.is_none()is equivalent to the oldarguments[1].is_null()gate on the pin/unpin paths.validate_mode/validate_uint32_array/validate_write_result_arraycompared against all three inline originals — equivalent (the BrotliRangeErrorOptions::default()vs.msg: b""difference is a no-op).map_init_return_code/regrow_output_tail: the implicit drop on the?error path runs the sameinflateEnd/deflateEndthe explicitdrop(zlib_reader)did;regrow_output_tail(.., usize::MAX)reduces exactly to the old unbounded compressor path.- Nothing outside
src/zlib/lib.rsreferencedbun_zlib_sys::{posix,win32}; the deleted externs are all re-declared locally insrc/zlib/lib.rs.
Extended reasoning...
Overview
Net -93 line refactor across the node zlib native bindings and the bun_zlib / bun_zlib_sys crates. Four shared validation helpers (validate_mode, validate_uint32_array, validate_write_result_array, unset_task_callback) and one shared 7-argument parser (parse_write_args) replace three-way duplicated code in NativeZlib/NativeBrotli/NativeZstd. In src/zlib/lib.rs, map_init_return_code and regrow_output_tail collapse repeated init/regrow blocks, and uInt::try_from(input.len()) guards replace truncating as casts. src/zlib_sys/{posix,win32}.rs are deleted (dead re-exports). One deliberate behavior change: NativeZstd::init now validates initParamsArray before s.init() allocates the ZSTD context. ~220 lines of new tests pin the exact error contract.
Security risks
The consolidated code sits directly on the path that validates user-controlled in_off/in_len/out_off/out_len against JS ArrayBuffer lengths before slicing into their backing stores — a classic OOB-write surface. I traced parse_write_args against both originals and the order/semantics are preserved (bounds compared in usize before any slice is formed; the reject_resizable flag correctly reproduces the async-only rejection from #36555). The uInt::try_from guards close a >4 GiB truncation on avail_in and a Windows-only deflateBound panic. No new attack surface is introduced.
Level of scrutiny
High. This is native FFI code that hands raw pointers into JS-owned memory to C compression libraries, across three codecs and both platforms. REVIEW.md's memory-safety and refactor-is-guilty-until-proven-behavior-preserving sections apply directly. The refactor is careful and the new tests pin every error message, but a subtle divergence here would be an OOB write, not a wrong error string — the class of bug the repo's guidelines flag as most-blocked.
Other factors
All prior review threads (CodeRabbit's jsv_to_u32 note, my two inline nits on the stderr assertion and the stale description paragraph) are resolved, and the description now accurately reflects the zstd reorder. The PR was just brought up to date with main at a maintainer's request and CI is running on the merged head. The deduplication itself is mechanically verifiable and I found no divergence, but the combination of a memory-safety-critical path, one intentional behavior change, and cross-platform FFI deletion puts this outside what I'd auto-approve.
|
@robobun fix the Source lints failure. |
The sweep lint read src/zlib_sys/win32.rs to check three dead externs had not come back; this branch removes the file, so the entries have nothing to read.
|
Fixed in 69ddbb7. The failing lint was test/internal/source-lints/dead-symbols-pub-exports-sweep.test.ts: one of main's dead-code sweeps added three entries that read src/zlib_sys/win32.rs to make sure gzprintf/inflateUndermine/deflateTune had not come back, and this branch deletes that file, so the read threw ENOENT. Removed those three entries; deleting the file covers what they were guarding. The full source-lints suite passes locally with the same command the workflow runs (75/75), and the check is re-running along with the rest of CI on build 91697. |
They came back in 69ddbb7 by way of a stale index; lib.rs only declares shared, so nothing referenced them.
|
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. |
|
One more commit on top of your merge: c072c5e. While checking that the merge left the PR intact I found that my lint fix (69ddbb7) had accidentally re-added src/zlib_sys/posix.rs and win32.rs; a stale index from an earlier verification step got swept into that commit. They were orphaned (lib.rs only declares shared, so nothing compiled them, which is why every check stayed green), but they are the files this PR exists to delete. Removed again, and the diff against main is back to the intended shape: helper dedup, shared write parser, zlib_sys split removed, zstd init ordering, length guards, lint entry, tests. Re-verified on the pushed tree: native and Windows cargo check, source lints 75/75, handle tests 48/48 (zstd test still fails with main's src), zlib.test.js and zstd.test.ts green. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/zlib/lib.rs`:
- Around line 180-185: Clamp every Vec-derived avail_out value to uInt::MAX as
usize before casting to uInt, including the assignments in regrow_output_tail
and the corresponding sites around lines 271, 756, and 794. Preserve any
existing budget or capacity limits, applying the uInt maximum as an additional
upper bound to prevent truncation.
In `@test/js/node/zlib/zlib-handle-bounds-check.test.ts`:
- Around line 119-140: Extend the mode validation matrix in the
test.each(modeCases) constructor test to cover undefined, zero, NaN, Infinity,
and -Infinity for every native constructor, asserting the shared validator’s
expected errors. Keep the existing string, fractional, and range checks, and
ensure zero is tested explicitly rather than only through min - 1.
- Around line 290-293: Update the invalid-initialization cases in the zlib
bounds-check test to assert that each NativeZlib, NativeBrotli, and NativeZstd
init() call throws instead of silently catching exceptions. Preserve the
existing invalid arguments and callback setup, and ensure an unexpected
successful init fails the test before the lifecycle continuation.
🪄 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: 75bea08a-b8e6-42a6-a3a4-94356da6b4e4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
src/runtime/node/node_zlib_binding.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rssrc/zlib/Cargo.tomlsrc/zlib/lib.rssrc/zlib_sys/lib.rssrc/zlib_sys/posix.rssrc/zlib_sys/win32.rstest/internal/source-lints/dead-symbols-pub-exports-sweep.test.tstest/js/node/zlib/zlib-handle-bounds-check.test.ts
💤 Files with no reviewable changes (5)
- src/zlib/Cargo.toml
- src/zlib_sys/posix.rs
- test/internal/source-lints/dead-symbols-pub-exports-sweep.test.ts
- src/zlib_sys/lib.rs
- src/zlib_sys/win32.rs
reserve_expand_tail returns the Vec's whole slack, so a buffer with 4 GiB or more spare truncated avail_out and could make a valid compress fail with ShortRead. Clamp at the two live sites (the regrow helper and the compressor's post-deflateBound setup). Tests: cover undefined, 0, NaN and the infinities for every constructor, and make the lifecycle test assert each invalid init() really threw.
The orphaned-files lint reads the committed tree, so it catches the stale-index resurrection that happened once already on this branch.
There was a problem hiding this comment.
I reviewed the latest revision (87a9140) and didn't find any bugs. The prior concerns from this thread — the accidentally-resurrected zlib_sys/{posix,win32}.rs, the missing gone[] guard, the unclamped avail_out casts, the mode/NaN test coverage, and asserting init() actually throws in the lifecycle test — are all addressed by c072c5e, 4562fd9, and 87a9140. Because this reworks the shared write/writeSync argument parsing that hands ArrayBuffer slices across the threadpool boundary and reorders NativeZstd::init, a human look before merge is still worthwhile.
What was reviewed:
parse_write_argspreserves the original per-path validation order (arg parsing →throw_unless_idle→ pin) and error class/code/message for bothwriteandwriteSync; thereject_resizableflag correctly scopes the async-only resizable-ArrayBuffer rejection.validate_mode/validate_uint32_array/validate_write_result_arraymatch the three inlined originals byte-for-byte on error text;RangeErrorOptionsnow usesmsg: b""uniformly (NativeBrotli previously used..Default::default(), same effect).map_init_return_code/regrow_output_tail/avail_out_forare equivalent to the blocks they replace, plus the newuInt::MAXclamps andavail_inoverflow rejection; the dropped explicitdrop(zlib_reader)calls are covered by?unwinding.zlib_sys/posix.rsandwin32.rsare deleted at HEAD and now listed in thegone[]orphaned-files guard.
Extended reasoning...
Overview
This PR deduplicates argument-validation helpers shared by NativeZlib/NativeBrotli/NativeZstd (constructor mode, writeResult/writeState, Uint32Array params, and the 7-argument write/writeSync parser) into node_zlib_binding.rs; extracts map_init_return_code/regrow_output_tail/avail_out_for in src/zlib/lib.rs; deletes the unused bun_zlib_sys::{posix,win32} modules and the bun_io dep from bun_zlib; and adds two hardenings (zstd initParamsArray validated before context allocation, and 32-bit avail_in/avail_out overflow guards). 12 files, ~575+/655−, with 234 lines of new tests pinning error class/code/message.
Security risks
The touched code parses user-controlled offsets/lengths into JS ArrayBuffer backing stores and hands raw slices to a threadpool worker. The refactor does not weaken any existing bounds check — the in_off + in_len / out_off + out_len checks and the resizable-ArrayBuffer rejection are preserved verbatim in parse_write_args, and the new tests assert them. The avail_in/avail_out 32-bit clamps close a truncation edge case rather than opening one. No new unsafe blocks are introduced in the binding layer; the SAFETY comments in src/zlib/lib.rs are carried over unchanged. I see no injection, auth, or data-exposure surface here.
Level of scrutiny
High. This is native Rust that manipulates raw pointers into GC-managed ArrayBuffers across a threadpool boundary, and REVIEW.md flags memory safety as the most-blocked category. The refactor is described as behavior-preserving, but it also carries two intentional behavior changes (zstd init ordering, >4 GiB input rejection), and the shared parser now serves both the sync and async paths with a boolean flag distinguishing them. That is exactly the shape where a subtle divergence between the two original copies could be silently unified the wrong way — the PR handles this correctly (reject_resizable), but it warrants a maintainer's eyes.
Other factors
The PR has been through 22 iterations over two months with four merge-from-main resolutions absorbing #32762, #33909, #34427, and #36555. A maintainer is actively driving it toward merge. Every prior inline comment on this thread (mine and CodeRabbit's) is resolved by the last three commits, and the multi-agent bug hunter found nothing on this revision. The new test file pins error messages for all three codecs and includes a regression test that fails on main. Given the scope and the memory-safety-adjacent code paths, deferring rather than auto-approving.
2 participants
What this does
Deduplicates the node
zlibnative bindings and removes dead FFI surface. Net -93 lines against current main.Binding helpers (
node_zlib_binding.rs, used byNativeZlib/NativeBrotli/NativeZstd): the constructormodecheck, thewriteResult/writeStatecheck, and theUint32Arrayparams check each existed three times;writeandwriteSynceach carried their own copy of the 7-argument parsing. They becomevalidate_mode,validate_write_result_array,validate_uint32_array, andparse_write_args. The extracted code is order-for-order and message-for-message identical to the inlined originals, which the new tests pin.parse_write_argsis generic over the stream type so it calls the per-codecflush_value_is_validadded by #34427, and takes areject_resizableflag because the two copies had diverged on main: the asyncwriterejects non-shared resizable ArrayBuffers (it captures the buffer for the threadpool, #36555) whilewriteSyncdoes not. The flag keeps that difference explicit instead of hiding it in two parsers.zlib crate:
map_init_return_codeandregrow_output_tailreplace repeated init/regrow blocks in the two ArrayList types.bun_iowas an unused dependency.zlib_sys:
posix.rsandwin32.rsonly re-exportedsharedtypes plus extern declarations nothing referenced;src/zlib/lib.rsdeclares the externs it actually calls itself. Both files go and the cfg-selected re-export inmod internalbecomes a plainsharedimport. Both paths are added to the source-lintgone[]list, which checks the committed tree, so a stray index cannot quietly bring them back (that happened once during this PR's history).Two small hardenings found while writing the tests, still unique to this PR:
NativeZstd::initvalidatesinitParamsArraybefores.init()allocates the context. Previously a type error there threw after the context existed, leaving a live encoder configured with default parameters behind the thrown error; a follow-up write on that handle compressed data. The new test fails on main and passes here.ZlibReaderArrayList/ZlibCompressorArrayListreject inputs longer than zlib's 32-bitavail_ininstead of truncating to a prefix (and, on Windows whereuLongis 32-bit, panicking at thedeflateBoundcast). On the output side,avail_outis clamped to 32 bits at the two live sites:reserve_expand_tailhands back the Vec's whole slack, and a buffer with exactly 4 GiB spare used to truncate to 0 and fail a valid compress withShortRead. Both need >4 GiB buffers to exercise, so neither has a CI test.History
Opened as a split of #31912. Earlier revisions also fixed crashes when finalizing or closing a never-initialized handle; #32762 landed the complete fix for that class on main and this PR now uses those versions. Main's dead-code sweeps also removed the unused
ZlibReadertype this PR originally deleted. What remains above is everything main has not absorbed, re-verified against main at the latest merge.Verification
test/js/node/zlib/zlib-handle-bounds-check.test.ts: contract tests for the constructors,init,write,writeSync(error class, code, and message), the zstd half-init regression test, and a lifecycle test covering failed-validationinit()followed byclose()and GC across all three classes. 48/48 pass with this PR; withsrc/reverted to main exactly the zstd test fails.test/js/node/zlib/*(includingzlib.test.js, 387 pass), the 61 upstreamtest-zlib-*node tests,test/js/bun/util/zstd.test.ts, and the fetch decompression regression tests pass on a debug build of the merged head.cargo checkandcargo clippyclean forbun_zlib_sys,bun_zlib,bun_runtime, natively and forx86_64-pc-windows-msvc(relevant becausewin32.rsis deleted).[review] gate passed · iteration 22 · 12 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 22
evidence per changed file