Repository navigation
Conversation
zlib.ts checked buffers with a local `instanceof ArrayBuffer ||
instanceof SharedArrayBuffer`. That check is false for a buffer created
in a node:vm context, so the sync and async convenience methods and the
`dictionary` option rejected it with ERR_INVALID_ARG_TYPE ("Received an
instance of ArrayBuffer"). Node uses util.types.isAnyArrayBuffer, which
checks the internal class and not the realm's prototype.
Take isArrayBufferView and isAnyArrayBuffer from node:util/types, like
node:crypto and the internal stream modules already do.
|
Status: ready for review. Reproduced on bun 1.4.1 with an Verified with a debug build: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughChangesCross-realm zlib buffer handling
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized change broadens zlib support for cross-realm ArrayBuffer inputs and adds targeted coverage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, root cause, fix, affected APIs, tests, and verification results. It does not use the template headings exactly, but it provides the required information and is complete. Comment |
|
The automated review raised no findings on c094249. No open review threads. CI build 109027 is in progress; the diff is a two-line change in zlib.ts plus tests, so no further changes are planned unless CI shows a failure in the zlib tests. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — swaps instanceof ArrayBuffer/SharedArrayBuffer for the realm-independent util.types brand checks, matching Node's own lib/zlib.js and 27 other built-in modules in this repo.
What was reviewed:
- All 12 call sites of
isArrayBufferView/isAnyArrayBufferinzlib.ts— the new tests exercise every path (sync/async convenience,ZlibandBrotlidictionary validation). isArrayBufferView:ArrayBuffer.isViewwas already realm-safe, so this half is a no-op behavior-wise;isAnyArrayBufferis the actual fix.- Negative case preserved: cross-realm plain object still throws
ERR_INVALID_ARG_TYPE.
Extended reasoning...
Overview
This PR touches two files: a two-line change in src/js/node/zlib.ts replacing local isArrayBufferView (was ArrayBuffer.isView) and isAnyArrayBuffer (was instanceof ArrayBuffer || instanceof SharedArrayBuffer) with the same-named functions from require("node:util/types"), and a new describe block in test/js/node/zlib/zlib.test.js that verifies cross-realm ArrayBuffer/SharedArrayBuffer inputs are accepted by every convenience method and the dictionary option.
Security risks
None. The change tightens type detection to a native brand check (JSArrayBuffer cell type via dynamicDowncast) rather than a prototype-chain walk. This is strictly less spoofable than instanceof, and it does not widen the accepted input space beyond what Node accepts — the negative test confirms non-buffer objects from the vm realm still throw ERR_INVALID_ARG_TYPE.
Level of scrutiny
Low. The fix is a well-understood Node-compat pattern: instanceof fails cross-realm, util.types.is* does not. The exact same import is already used by 27 other built-in modules in src/js/ (crypto, fs, tls, streams, etc.), so this follows the established in-tree convention that REVIEW.md calls for. Node's own lib/zlib.js uses internal/util/types for the same two functions, so this brings Bun's zlib in line with the reference implementation.
Other factors
Test coverage is thorough for the size of the fix: it covers all five sync compress/decompress pairs, four async pairs, both ArrayBuffer and SharedArrayBuffer, both dictionary code paths (Zlib at line 562 and Brotli at line 720), and the rejection path. The precondition assertion (instanceof ArrayBuffer is false) ensures the test would fail on the old code for the right reason. No CODEOWNERS entries cover these paths, no prior reviewer objections exist in the timeline, and the bug hunt exited on dry_streak with no findings.
|
Both automated reviews are in with no requested changes. No open review threads. CI build 109027 is at 27 of 181 jobs passed, the rest are running. I will report back when it finishes. |
|
Nothing new to address from the updated automated summary (no findings). Build 109027 finished with 180 of 181 jobs green. The one failed job ran |
This branch has not been deployed
2 participants
Problem
node:zlibrejects anArrayBufferorSharedArrayBufferthat was created in another realm (vm.runInContext). Every convenience method (gzipSync,gzip,deflateSync,brotliCompressSync,zstdCompressSync, ...) and thedictionaryoption throwERR_INVALID_ARG_TYPE. The message contradicts itself:The "buffer" argument must be of type string, Buffer, TypedArray, DataView, or ArrayBuffer. Received an instance of ArrayBuffer. Node 26.3.0 accepts the same input.src/js/node/zlib.ts:21:b instanceof ArrayBuffer || b instanceof SharedArrayBuffer.instanceofwalks the prototype chain, and a buffer from another realm has that realm'sArrayBuffer.prototype. TypedArray and DataView inputs were not affected becauseArrayBuffer.isViewreads the internal type.Fix
isArrayBufferViewandisAnyArrayBufferfromnode:util/types. Both are native and check the cell type (JSArrayBuffer, typed array or DataView), so the realm does not matter. This is what node'slib/zlib.jsdoes, and whatnode:cryptoand the internal stream modules in bun already do.test/js/node/zlib/zlib.test.js(newArrayBuffer from another realmblock, 15 of 17 cases fail on bun 1.4.1). Also the rest oftest/js/node/zlib/and the vendoredtest-zlib-convenience-methods.js,test-zlib-dictionary.js,test-zlib-brotli-dictionary.js,test-zlib-zstd-dictionary.js,test-zlib-not-string-or-buffer.js,test-zlib-invalid-input.js.Background
vm.createContext()creates a new realm with its ownArrayBuffer,Objectand so on. A value created there and passed to the main realm has a different[[Prototype]], soinstanceofagainst a main-realm constructor is false.util.types.isAnyArrayBuffer(value)is realm-independent. Bun implements it insrc/jsc/modules/NodeUtilTypesModule.cppasdynamicDowncast<JSArrayBuffer>.Notes
Repro on bun 1.4.1 (node 26.3.0 prints the decompressed bytes for every line):
test/js/node/zlib/leak.test.tstimes out in itsbeforeAllon this debug ASAN build with and without this change (10,000deflateSynccalls in one hook). It is not related to this diff.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file