Repository navigation
Conversation
Make bun_zlib a fully safe public API by encoding zlib's preconditions
as type invariants on z_stream instead of scattering unsafe blocks at
every call site.
bun_zlib_sys:
- Privatize z_stream.{internal_state, next_in, next_out, avail_in,
avail_out, alloc_func, free_func, err_msg, user_data}. The single
type invariant (internal_state is null or zlib-owned; buffer windows
are null or valid) is documented once on the struct.
- Declare inflateInit2_/deflateInit2_/inflateEnd/deflateEnd/
inflateReset/deflateReset/deflateBound as safe fn(&mut z_stream) now
that the invariant discharges their precondition (verified against
vendor/zlib's inflateStateCheck/deflateStateCheck).
- Keep inflate/deflate/deflateParams/crc32/compress2/setDictionary as
unsafe in the raw module; front each with an #[inline(always)] safe
method (z_stream::inflate/deflate/..., z_stream::input()).
- Drop the posix/win32 extern duplication (shared.rs is the single
source of truth; the per-platform modules just re-export).
bun_zlib:
- Remove the four duplicate extern "C" blocks (now imported from
bun_zlib_sys::raw) and the 16 raw externs from the public surface.
- Add compress2_into(&mut [u8], &[u8], level) as the safe compress2.
- ZlibReaderArrayList/ZlibCompressorArrayList/DeflateEncoder/
InflateDecoder route every zlib call through the safe z_stream
methods; 28 unsafe blocks down to 12, each establishing the buffer
invariant locally against a borrowed slice or owned Vec tail.
- Delete the unused ZlibReader<W> type and its bun_io dependency.
Consumers:
- NativeZlib::Context: 19 zlib unsafe blocks down to 1 (set_buffers,
which vouches for the pinned-ArrayBuffer contract across the worker
thread; the inflate/deflate/init/end/reset/params/setDictionary
calls themselves are now safe method calls). The UNZIP header sniff
reads z_stream::input() instead of raw-dereffing next_in.
- crash_handler: compress2 call goes through compress2_into.
- All other consumers were already behind the safe wrapper types and
are untouched.
Zero behavior change; #[inline(always)] on every wrapper keeps the hot
paths at the same codegen. bun run rust:check-all passes on all ten
targets.
|
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 zlib bindings now encapsulate stream state behind Rust methods and accessors. Compression, decompression, native Node integration, buffer safety contracts, crash trace compression, and regression tests were updated to use the new API. Safe zlib bindings
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:14 PM PT - Jul 23rd, 2026
❌ @robobun, your commit e9abb3f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35319That installs a local version of the PR into your bun-35319 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #31996, though they share some mechanical cleanup. #31996 consolidates helper functions in the node zlib binding layer ( The overlap is incidental dead-code removal both PRs needed (the unused |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…Long in compress2_into, drop dead Options.method, assert Z_NEED_DICT code
…ases, drop unused zError, assert output in reset() test
|
CI on 8bbc492: the zlib test suites (node/zlib, fetch-gzip, websocket-permessage-deflate, the new Ready for review. |
- with_allocator: now unsafe; (S2) requires a matched alloc/free pair since zlib defaults zalloc and zfree independently, so half-null is a cross-allocator free in *_end(). - data_type: c_int not a 3-variant enum; inflate() writes a bitfield (unused bits + 64/128/256) on every return, so the old DataType field held an invalid discriminant after every safe inflate() call. Nothing read it, but it was the one pub field whose invariant zlib itself violates. - input() -> input_at(i): a raw byte read instead of from_raw_parts, so (B)'s "readable" contract suffices and no &[u8] aliasing assertion is introduced. Both callers only ever peek bytes 0/1.
…ract to cover allocator behavior
…ia disjoint mode/status ranges
- Zlib*ArrayList.list_ptr: privatized; a caller reallocating the Vec through the pub field dangled the z_stream's next_out. - CompressionContext::set_buffers: now unsafe fn (trait + 3 inherent impls). The safe-signature version stored raw pointers that outlive the slice borrows, so a safe caller could pass stack slices and then call safe do_work() on freed memory. The pinning contract is now documented on the trait and discharged at the two call sites in node_zlib_binding.rs where the ArrayBuffers are actually pinned.
…y/finally d.close() in reset test
There was a problem hiding this comment.
No findings remain — every soundness nit from the earlier rounds (raw::*Init2_ version deref, zError, with_allocator contract, data_type enum validity, cross-kind StateCheck, list_ptr/set_buffers pub launderers, the write_sync SAFETY wording, and the test try/finally) is resolved as of e9abb3f. Deferring to a human because this is the template-setting *_sys safe-binding pattern dylan-conway requested; the invariant design (S1/S2/B) and the safe fn extern choices deserve a maintainer sign-off before other crates copy it.
What was reviewed:
- The (S1)/(S2)/(B) invariant set against vendored zlib-ng's
inflateStateCheck/deflateStateCheckanddeflateParams— eachsafe fninraw::is now total given the struct invariants plus the documented cross-kind mode/status disjointness. - Every remaining
unsafeblock inbun_zlib/NativeZlibestablishes (B) against a locally-owned buffer or forwards a documented caller contract; no laundering via pub fields remains. - The UNZIP header-sniff and GUNZIP trailing-member rewrites preserve the exact byte-index reads and
avail_in == 1early-return of the original raw-pointer code. compress2_intohandles the LLP64uLongtruncation case;crash_handlerbehavior is preserved.
Extended reasoning...
Overview
This PR restructures bun_zlib_sys and bun_zlib so product code no longer writes unsafe to call zlib. It privatizes the z_stream fields that carry FFI preconditions, documents three type invariants (S1: internal_state null-or-zlib-owned; S2: allocator behavioral contract; B: buffer validity), and fronts the extern block with #[inline(always)] safe methods. posix.rs/win32.rs/shared.rs collapse into a single lib.rs; the dead ZlibReader<W>, Options.method, and bun_io dependency are removed. Consumers (ZlibReaderArrayList, ZlibCompressorArrayList, DeflateEncoder/InflateDecoder, NativeZlib::Context, crash_handler) are rewired to the safe surface. CompressionContext::set_buffers becomes unsafe fn across all three backends (zlib/brotli/zstd) with the pinning/call-frame-rooting contract lifted into a # Safety doc. Net −533 LOC across 13 files.
Security risks
None user-facing. This is an internal FFI refactor with no new attack surface — the same zlib calls happen with the same arguments. The soundness-relevant changes (which raw:: externs are safe fn, which struct fields stay pub) were the focus of the prior review rounds; each hole raised (garbage version pointer, data_type enum out-of-range discriminant, allocator misbehavior via safe with_allocator, cross-kind inflate_end on a deflate state, list_ptr/set_buffers launderers) has been closed by making the entry point unsafe, privatizing the field, or documenting the vendored zlib-ng defense that discharges it.
Level of scrutiny
High. This is memory-safety-adjacent FFI code on a hot path (every node:zlib operation, Bun.gzipSync, fetch decompression, WebSocket permessage-deflate), and — per the PR description — the explicit template for the other *_sys crates. The invariant design and the safe fn boundary choices will be copied, so a maintainer should ratify the pattern before it propagates. The five-round back-and-forth on this PR already tightened several of those boundaries; the current state looks sound to me, but the architectural sign-off belongs to a human.
Other factors
CI on the latest push is green on all zlib-adjacent suites (zlib.test.js 383 tests, fetch-gzip, websocket-permessage-deflate, the new behavior-preservation suite); the only red is unrelated spawn.test.ts flake also failing on main. The behavior-preservation test file is well-constructed (known-answer vectors, split-header UNZIP, concatenated gzip members, params() mid-stream, reset(), dictionary Adler-32 mismatch) and the earlier bare-.toThrow() / cleanup-after-assert nits are fixed. Every prior inline finding is marked resolved with a corresponding commit.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-24, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
Requested by @dylan-conway as the proving ground for the
*_syssafe-binding pattern.This is a zero-behavior-change refactor by design. The property being changed, "callers don't write
unsafeto reach zlib", is a compile-time type-system property with no runtime observable.test/js/node/zlib/zlib-safe-bindings.test.tsis a behavior-preservation suite: it passes identically on main and on this branch (11/11, 23 expect() calls), which is the proof that the refactor preserved every touched code path. A fail-before test is structurally impossible here; merge on the strength of the diff review and the preservation suite.What this does
Makes
bun_zliba fully safe public API so no product code writesunsafeto call zlib. The pattern: encode zlib's actual preconditions as type invariants onz_stream(private fields + documented invariants), then the FFI calls that only depend on those invariants become honestlysafe fn. This is the template for the other*_syscrates.The invariant
z_streamnow privately ownsinternal_state,next_in/avail_in,next_out/avail_out,alloc_func/free_func,err_msg,user_data. Three invariants, stated once on the struct:internal_stateis null or points to a live state allocated by zlib for this stream. Enforced by field privacy: only zlib ever writes it.alloc_func/free_funcare null or valid fn ptrs. Already enforced byOption<extern fn>.next_inis null or readable foravail_inbytes;next_outis null or writable foravail_outbytes. Established byunsafe fn set_input/set_output, the two places a caller vouches for a buffer.Verified against
vendor/zlib'sinflateStateCheck/deflateStateCheck(which dereferencestatewhen non-null, so (S1) is load-bearing) anddeflateParams(which callsdeflate(Z_BLOCK)internally, so it needs (B) not just (S1)).bun_zlib_sysraw::{inflateEnd, deflateEnd, inflateReset, deflateReset, deflateBound}declared assafe fn(&mut z_stream, ...): (S1)+(S2) discharge every precondition, and&mut z_streamis ABI-identical to a non-nullz_streamp.raw::{inflate, deflate, deflateParams, inflateInit2_, deflateInit2_, inflateSetDictionary, deflateSetDictionary, crc32, compress2}stayunsafe fn; fronted by#[inline(always)]safe methods onz_stream(inflate,deflate,inflate_init2,deflate_init2,deflate_params,inflate_set_dictionary(&[u8]),input() -> &[u8], etc.) and free wrappers (crc32_bytes,compress2_into).posix.rs/win32.rsdeleted;shared.rsis the single extern block (the 76-fnwin32.rsbindgen dump was dead; nothing imported it).bun_zlibunsafe extern "C"blocks (16 raw fns) removed from the public surface.ZlibReaderArrayList/ZlibCompressorArrayList/DeflateEncoder/InflateDecoderroute every call through the safez_streammethods. Control flow and allocator choices preserved exactly.ZlibReader<W>type and itsbun_iodependency deleted. DeadOptions.methodfield removed.compress2_into(&mut [u8], &[u8], level) -> Result<usize, ReturnCode>.Consumers
unsafeblocks beforesrc/zlib/lib.rssrc/runtime/node/zlib/NativeZlib.rssrc/crash_handler/lib.rsThe one remaining
unsafeinNativeZlib::Context::set_buffersis the irreducible one: it vouches that the pinned JSArrayBufferbacking stores stay valid across the worker-threaddo_work. That's a JS-pinning contract, not a zlib call. The UNZIP header sniff and the GUNZIP trailing-member loop now readz_stream::input()instead of raw-dereffingnext_in.Verification
test/js/node/zlib/zlib-safe-bindings.test.ts(new): known-answer vectors for every touched path (Bun.gzipSync/gunzipSync/deflateSync/inflateSync,Bun.hash.crc32, node:zlib dictionary inflate/deflate,createUnzipauto-detect with header split across two writes, concatenated gzip members with zero padding,params()mid-stream,reset(),err_msgsurfaces). Passes identically on the released bun and this branch.test/js/node/zlib/zlib.test.js(383 tests),zlib-handle-bounds-check.test.ts,fetch-gzip.test.ts,websocket-permessage-deflate.test.ts, the18413*/17793fetch-decompress regressions, and the nodetest-zlib*.jsparallel suite all pass onbun bd.bun run rust:check-allclean on all ten targets.cargo clippyclean onbun_zlib_sys/bun_zlib/bun_runtime/bun_crash_handler.Zero behavior change;
#[inline(always)]on every wrapper method keeps the hot paths at the same codegen as the previous direct externs.