Repository navigation
Conversation
Both functions downcast to JSArrayBufferView, which a DataView also is, so a DataView was scanned instead of rejected, and the rejection used a hand-written message instead of Node's ERR_INVALID_ARG_TYPE text. The two functions also disagreed on a view over a detached buffer (isUtf8 threw a plain TypeError, isAscii ERR_INVALID_STATE) where Node returns true, and the detached ArrayBuffer message carried an "Invalid state: " prefix Node's C++-thrown error does not have. Route both through one helper that accepts a TypedArray (isTypedArrayType excludes DataView), an ArrayBuffer or a SharedArrayBuffer, throws ERR_INVALID_STATE with Node's message for a detached ArrayBuffer, and ERR_INVALID_ARG_TYPE through INVALID_ARG_INSTANCE for everything else.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. Reproduced on bun 1.4.0 with the released binary: Self-review of the diff turned up nothing that needed changing; the one useful finding (Node main has since dropped the detached- |
There was a problem hiding this comment.
LGTM — focused Node.js compat fix that consolidates two near-duplicate functions into one shared helper.
What was reviewed:
isTypedArrayType(view->type())correctly excludes DataView; matches usage inBunObject.cpp/napi.cpp.- Removed
impl()null check is safe —JSBuffer.cpp:827already dereferences it unconditionally after the same downcast. view->span()on a detached/out-of-bounds view is empty, so no memory is read; error helpers (INVALID_ARG_INSTANCE,throwErrorwithERR_INVALID_STATE) match existing call patterns.utf-8-validateshim reusesjsBufferConstructorFunction_isUtf8and now rejects DataView too — its test and documented API are Buffer-only, so unaffected.
Extended reasoning...
Overview
This PR fixes buffer.isUtf8() and buffer.isAscii() to match Node.js v26.3.0's input handling. It touches two files: src/jsc/modules/NodeBufferModule.h (~110 lines removed, ~50 added — two near-identical function bodies collapsed into one validateBytesOf template helper) and test/js/node/buffer.test.js (+86 lines of new describe.each coverage). The behavioral changes are: (1) reject DataView with ERR_INVALID_ARG_TYPE instead of scanning it, (2) use Node's exact error message via Bun::ERR::INVALID_ARG_INSTANCE for all rejected inputs, (3) return true for a TypedArray over a detached buffer instead of throwing, and (4) drop the Invalid state: prefix from the detached-ArrayBuffer error to match Node's C++-side wording.
Security risks
None. The only user-controlled input is the argument value, which is type-checked via JSC's dynamicDowncast before any bytes are read. Byte access goes through JSArrayBufferView::span() and ArrayBuffer::span(), both of which return an empty span for detached/out-of-bounds views, so there is no path to reading freed or out-of-range memory. No allocation, no size arithmetic, no user-callback reentrancy.
Level of scrutiny
Low-to-medium. This is a small, self-contained Node compat fix in a leaf function pair with no callers other than the module export table and the utf-8-validate third-party shim. The helper uses established in-tree idioms: isTypedArrayType() is the same predicate BunObject.cpp and napi.cpp use to distinguish TypedArray from DataView; INVALID_ARG_INSTANCE is the standard helper for class-instance error messages; the removed impl() null check matches JSBuffer.cpp:827 which already dereferences arrayBuffer->impl() unconditionally after the same downcast. The dropped RELEASE_AND_RETURN on the tail return is fine — neither jsBoolean nor simdutf::validate_* can throw, so there is no throwing tail call to release across.
Other factors
Test coverage is thorough: a describe.each over both functions asserting exact error class/code/message for DataView (three shapes), six rejected primitives/objects, detached ArrayBuffer, detached view (returns true), and every accepted input kind (all twelve TypedArrays, Buffer, ArrayBuffer, SharedArrayBuffer, sub-range view). The PR verified against Node v26.3.0 with a 49-input probe producing byte-identical output, and confirmed 22/24 new cases fail on bun 1.4.0. The ported Node tests test-buffer-isutf8.js / test-buffer-isascii.js and utf-8-validate.test.ts continue to pass. No outstanding reviewer comments; the only timeline entry is a CodeRabbit rate-limit notice.
|
Updated 12:13 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
…Ascii share one body
There was a problem hiding this comment.
LGTM — focused Node compat fix that collapses two duplicated argument-handling blocks into one helper and matches Node v26.3.0 exactly.
What was reviewed:
isTypedArrayType(view->type())correctly excludes DataView (same predicate asBunObject.cpp/napi.cpp);INVALID_ARG_INSTANCEandBun::throwErrorsignatures/return types match.- Dropping the
impl()null check is consistent withJSBuffer.cpp:827and other callers that dereference it unconditionally. view->span()on a detached view is safe (length 0, guarded bybytes.empty()) and is covered by the new detached-view test; theutf-8-validateshim sharing this function was checked — its documented arg is a Buffer.- Tests cover DataView (3 shapes), all TypedArray kinds, SAB, sub-range views, detached AB vs detached view, and exact error name/code/message for rejected inputs.
Extended reasoning...
Overview
Rewrites the argument handling for buffer.isUtf8() and buffer.isAscii() in src/jsc/modules/NodeBufferModule.h by replacing two ~50-line near-duplicate blocks with a single templated helper validateBytesOf. The behavioral changes: DataView is now rejected with ERR_INVALID_ARG_TYPE (was silently accepted because JSC's JSArrayBufferView includes DataView), rejected inputs get Node's exact The "input" argument must be an instance of ... message via Bun::ERR::INVALID_ARG_INSTANCE, a TypedArray over a detached buffer validates as empty (was throwing), and a detached ArrayBuffer throws ERR_INVALID_STATE without the Invalid state: prefix. Adds 86 lines of tests to test/js/node/buffer.test.js running both functions through the full input matrix via describe.each.
Security risks
None. This is input-type validation for a read-only byte scanner. No untrusted lengths drive allocation; span() returns JSC's bounds-checked view and the empty check runs before the simdutf call. No auth, crypto, or filesystem paths touched.
Level of scrutiny
Low-to-medium. Small, self-contained Node compat fix in a leaf function with no callers other than the module export and the utf-8-validate shim. The helper uses established in-tree primitives (isTypedArrayType, INVALID_ARG_INSTANCE, Bun::throwError) exactly as neighboring code does. I verified each helper's signature and return type in ErrorCode.h, and confirmed the impl() non-null assumption matches existing unconditional dereferences in JSBuffer.cpp, napi.cpp, and KeyObject.cpp. The dropped RELEASE_AND_RETURN is fine — nothing between scope declaration and the non-throwing return can raise an exception.
Other factors
Test coverage is thorough and asserts the strongest invariant (exact error name, code, and message; every TypedArray kind including Float16Array and BigInt arrays; SharedArrayBuffer; sub-range views; both detached forms). The PR description shows a 49-input probe with byte-identical output vs node v26.3.0, and reports the ported upstream test-buffer-isutf8.js/test-buffer-isascii.js and the utf-8-validate test all pass. The comment-cop bot's three inline notes were addressed in 2f7de0c (comments trimmed) and all threads are resolved. The bug hunting system found nothing.
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can this use the possibly shared impl thing? Can it avoid cloning bytes
Problem
isUtf8(new DataView(ab))andisAscii(new DataView(ab))fromnode:bufferscan the bytes and return a boolean. Node throwsTypeError [ERR_INVALID_ARG_TYPE]: The "input" argument must be an instance of ArrayBuffer, Buffer, or TypedArray. Received an instance of DataView.dynamicDowncast<JSArrayBufferView>(src/jsc/modules/NodeBufferModule.h:25and:79on main). In JSC aDataViewis aJSArrayBufferView, so it takes the "view" branch.First argument must be an ArrayBufferViewinstead of Node's message above; a TypedArray whose buffer was detached throws (a code-lessTypeErrorfromisUtf8,ERR_INVALID_STATEfromisAscii) where Node returnstrue; a detachedArrayBufferthrows with the messageInvalid state: Cannot validate on a detached bufferwhere Node's isCannot validate on a detached buffer.Fix
validateBytesOf, that each function calls with its simdutf validator. The helper is a port of Node's two layers: the JS checkisTypedArray(input) || isAnyArrayBuffer(input)(lib/buffer.js#L1415-L1429) and the binding's detached check (src/node_buffer.cc#L1305-L1333).view && isTypedArrayType(view->type()):isTypedArrayTypeis true for the twelve TypedArray kinds and false forDataViewType, which is exactly Node'sisTypedArray. It is the predicateBunObject.cppandnapi.cppalready use for this distinction, and the one node:buffer: reject DataView in Buffer.copyBytesFrom #35239 uses for the same bug inBuffer.copyBytesFrom.Bun::ERR::INVALID_ARG_INSTANCE("input", "ArrayBuffer, Buffer, or TypedArray", value), which keeps theERR_INVALID_ARG_TYPEcode and renders Node's wording including theReceived ...suffix. This is the helper the rest ofJSBuffer.cppuses for class-instance expectations.ArrayBufferform (util-inl.h#L599); a view over a detached buffer reportsbyteLength0 and validates as empty, so the helper readsview->span()(JSC zeroes the length when a view is detached) and only checksimpl()->isDetached()on theArrayBufferbranch. TheArrayBuffererror is thrown withBun::throwError(ERR_INVALID_STATE, ...)rather thanBun::ERR::INVALID_STATE, because the latter prependsInvalid state:(Node's JS-side format) and this error comes from C++ in Node;NodeSqlite.cppmakes the same distinction. Theimpl()null check the oldisUtf8copy had is gone:JSArrayBuffer::finishCreationdereferencesimpl(), so it is never null for a live wrapper, andBuffer.byteLengthinJSBuffer.cppalready relies on that.ArrayBufferthrow too, so there a detachedArrayBufferand a view over one are both treated as empty. This PR follows v26.3.0, which is what the ported upstream tests intest/js/node/test/parallel/test-buffer-isutf8.jsandtest-buffer-isascii.jsassert (ERR_INVALID_STATEfor theArrayBufferform). When those files are re-ported from a release that contains #64504, theisDetached()branch of the helper and the oneERR_INVALID_STATEassertion added here are the two things to drop; the detached-view behavior adopted here is the one #64504 codifies.utf-8-validate's default export is this sameisUtf8host function (UTF8ValidateModule.h), so it picks up the same argument handling. Its documented argument is aBuffer, andwsandcrosswsonly ever pass one; theArrayBufferacceptance andERR_INVALID_ARG_TYPErejections it had before were alreadynode:buffersemantics rather than the package's.test/js/node/buffer.test.js(buffer.isUtf8() input handling/buffer.isAscii() input handling): 22 of the 24 new cases fail on bun 1.4.0 (threeDataViewshapes, six rejected inputs with the exact message, the detachedArrayBuffermessage, and the detached view) and all pass with this build; the two that pass on both are the accepted-kinds controls covering every TypedArray kind,Buffer,ArrayBuffer,SharedArrayBuffer, a view over aSharedArrayBuffer, and a sub-range view, with bytes that makeisUtf8andisAsciianswer differently.bun bd test test/js/node/buffer.test.js: 642 pass, 1 skip, 0 fail.bun bd test/js/node/test/parallel/test-buffer-isutf8.jsandtest-buffer-isascii.js(Node's own tests, already ported): exit 0.bun bd test test/js/first_party/utf-8-validate/utf-8-validate.test.ts: pass.DataViewshapes, and 20 non-buffer values) prints byte-identical output on this build and on node v26.3.0; 50 of its 98 lines differ on bun 1.4.0. Output below.Background
JSArrayBufferViewis JSC's base class for everything that views anArrayBuffer: the typed arrays andDataView.dynamicDowncast<JSArrayBufferView>therefore accepts aDataView;JSC::isTypedArrayType(JSType)is the predicate that excludes it (isTypedArrayTypeIncludingDataViewis the other variant).JSArrayBufferView::span()is the view's bytes as{vector(), byteLength()}. When a buffer is detached JSC sets every view's length to 0 and clears its vector, and a view that fell out of bounds of a shrunk resizable buffer also reports length 0, so the span is empty in both cases and no memory is read.JSArrayBufferwraps bothArrayBufferandSharedArrayBuffer(they differ only inimpl()->isShared()), which is why one downcast covers Node'sisAnyArrayBuffer. ASharedArrayBuffercannot be detached.ERR_INVALID_ARG_TYPEpicks its wording from the expected names: capitalized class names render asmust be an instance of A, B, or C.Bun::ERR::INVALID_ARG_INSTANCEis the helper that produces that form;Bun::ERR::INVALID_ARG_TYPEwith a plain string rendersmust be of type ....ERR_INVALID_STATE: the JS one formats asInvalid state: %s, the C++THROW_ERR_INVALID_STATEuses the message verbatim.Bun::ERR::INVALID_STATEimplements the JS flavor;Bun::throwErrorwith the code gives the C++ one.Probe output (identical on this build and node v26.3.0)
The
isAsciihalf is the same except that the eight-byte inputs (which end in the UTF-8 encoding ofé) returnfalse; the ASCII slice, the out-of-bounds and length-tracking views, the empty inputs and the detached view returntrue.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/buffer.test.js