Repository navigation
node:zlib: convert compression-stream wrapper self-ref from StrongOptional to JsRef - #31843
Jarred-Sumner wants to merge 8 commits into
Conversation
Drain the Rust port's marker backlog: every audited TODO(port)/TODO(b2-*) marker and PORT NOTE comment was individually resolved - fixed, deleted as stale, untagged to a plain comment, or split into a tracked follow-up work order. ~495 real code fixes, each citing the Zig sibling for intended semantics; the 101 markers that remain are claimed by written work orders for changes too risky to land in a bulk branch (GC rooting, arena lifetime threading, cross-thread ownership restructures). Verified: full debug build + smoke tests on linux-x64, cross-target cargo check on windows/macos/linux x64/aarch64.
The codebase has moved from a port to a standalone Rust implementation; comments citing the prior Zig implementation (file/line cross-references, std.* stdlib mentions, quoted Zig syntax, divergence notes) no longer aid maintenance. Delete them, keeping SAFETY justifications rewritten in terms of the Rust code's own invariants. References to current identifiers (ZigString etc.) are unchanged.
NewServer::init moves the config into the server, so the hot-map registration that runs after init read an empty id from the stale local and registered every server under the same key, panicking on the second Bun.serve in --hot processes. Read allow_hot and id from the server's own config instead. This path was unreachable until hot_map() gained its lazy-init accessor. Also update parsePackedFeaturesList() for the new define_features! shape (internal @storage recursion and core = IDENT aliases).
- define_features!: drop the allow(non_upper_case_globals) from the pub use re-export arm where the lint cannot apply (useless_attribute x47) - HardcodedModule: alias maps use bun_collections::HashMap per the disallowed-types policy (wyhash instead of SipHash) - shell_parser, js_printer: byte paths/text Display via bstr::BStr instead of String::from_utf8_lossy - shell_parser: drop_in_place(&raw mut ...) per borrow_as_ptr - Loader: derive Default instead of the manual impl - Yarn printer: JSON debug env check via bun_core::getenv_z - pnpm: remove empty is_prod() placeholder branch - WebSocketUpgradeClient: SslCtxOwned::into_raw via ManuallyDrop instead of mem::forget - PostgresSQLConnection: remove redundant AnyPostgresError::from - streams: return result instead of Ok(result?)
…ional to JsRef The native NativeZlib/NativeBrotli/NativeZstd compression streams held their JS wrapper back-reference as a bare jsc Strong.Optional, the pattern flagged as a leak hazard for self-refs to a class's own wrapper. This converts the shared this_value slot to JsRef across the CompressionStream mixin trait and all three implementations. The lifecycle is preserved exactly: the ref is upgraded to strong only while an async write is in flight on the work pool (the wrapper has no pending-activity hook, so this strong ref is the sole GC liveness source for the wrapper and its cached writeCallback/pendingInput/pendingOutput values mid-write), taken and cleared when the write completes on the JS thread, cleared on close, and marked Finalized at native teardown as advisory terminal state (try_get/set_weak treat it as dead; the actual protection against late access is that the struct is freed immediately after refcount-zero deinit). All three implementations finalize the slot identically in teardown. NativeZstd shares the same mixin macro, so its field is converted in the same mechanical way. Tested with new subprocess tests in test/js/node/zlib/zlib.test.js that drive async deflate/gzip/brotli/zstd round-trips with Bun.gc(true) forced between scheduling and completion (would crash or corrupt output if in-flight rooting were lost), plus destroy-with-in-flight-write loops under GC pressure covering the pending_close path. The new tests are regression-only guards: the conversion intentionally preserves GC behavior, so they also pass on the pre-change build; the verify phase should run the full test/js/node/zlib/ suite (and ideally ASAN) as the actual verification.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing both nits. No bugs found, but the core StrongOptional → JsRef conversion changes GC-rooting semantics across the work-pool thread boundary, so I'd like a human to sign off on the lifecycle (set_strong → try_get/clear → finalize) rather than auto-approving.
Extended reasoning...
Overview
The substantive change is confined to four files: node_zlib_binding.rs (the CompressionStream mixin trait) and the three Native{Zlib,Brotli,Zstd}.rs implementations, plus two new regression tests in zlib.test.js. The remaining ~200 files in the diff are pure autofix.ci formatting noise — trailing-newline removal, import reordering, derive consolidation, blank-line trimming — with no semantic impact.
The core change swaps the this_value slot type from StrongOptional to JsRef and adjusts the four lifecycle touchpoints accordingly: set_strong() on async-write start, try_get() + reset-to-empty() on completion, reset on close_internal, and finalize() in each struct's teardown path. The PR description states behavior is preserved exactly, and the new subprocess tests force GC between scheduling and completion to guard against regressions.
Security risks
No injection, auth, or data-exposure surface. The risk class here is memory safety: this is the GC-rooting mechanism that keeps the JS wrapper (and its cached pending input/output buffers) alive while a worker thread holds raw pointers into them. A mistake in the strong/weak transition would manifest as use-after-free under GC pressure, not as a security boundary bypass — but UAF in native code is still a correctness/stability concern that merits careful review.
Level of scrutiny
High. Even though the conversion is described as mechanical and behavior-preserving, it sits at the intersection of (a) JSC GC rooting, (b) cross-thread lifetime management via the work pool, and (c) intrusive refcounting with explicit finalize() calls in three slightly different teardown paths (destroy_on_zero for Brotli, deinit for Zlib, Drop for Zstd). The JsRef API surface (set_strong, try_get, empty, finalize) has subtly different semantics from StrongOptional (set, try_swap, deinit), and verifying the mapping is correct for each path is exactly the kind of thing a human reviewer familiar with the JsRef contract should confirm.
Other factors
- Both prior review nits (assert on stderr, drop explicit timeouts) were addressed in 3cbecef.
- The bug-hunting pass found nothing.
- ASAN + debug-build verification is documented in the PR description.
- The autofix formatting churn is large but inert; it does make the diff harder to skim, which is another reason a human should look at the four substantive files directly rather than trusting the file count.
2 participants
The native NativeZlib/NativeBrotli/NativeZstd compression streams held their JS wrapper back-reference as a bare jsc Strong.Optional, the pattern flagged as a leak hazard for self-refs to a class's own wrapper. This converts the shared this_value slot to JsRef across the CompressionStream mixin trait and all three implementations. The lifecycle is preserved exactly: the ref is upgraded to strong only while an async write is in flight on the work pool (the wrapper has no pending-activity hook, so this strong ref is the sole GC liveness source for the wrapper and its cached writeCallback/pendingInput/pendingOutput values mid-write), taken and cleared when the write completes on the JS thread, cleared on close, and marked Finalized at native teardown as advisory terminal state (try_get/set_weak treat it as dead; the actual protection against late access is that the struct is freed immediately after refcount-zero deinit). All three implementations finalize the slot identically in teardown. NativeZstd shares the same mixin macro, so its field is converted in the same mechanical way. Tested with new subprocess tests in test/js/node/zlib/zlib.test.js that drive async deflate/gzip/brotli/zstd round-trips with Bun.gc(true) forced between scheduling and completion (would crash or corrupt output if in-flight rooting were lost), plus destroy-with-in-flight-write loops under GC pressure covering the pending_close path. The new tests are regression-only guards: the conversion intentionally preserves GC behavior, so they also pass on the pre-change build; the verify phase should run the full test/js/node/zlib/ suite (and ideally ASAN) as the actual verification.
Verification
Implemented and verified on a unified integration branch: full debug build (linux-x64, ASAN), cargo check across the workspace, and the affected test files run against the debug build (failures cross-checked against main's build to exclude pre-existing issues). Each change was reviewed twice (compile/API correctness and GC/concurrency/semantics lenses) with findings repaired before landing.