[JSC] Uint8Array.prototype.setFromBase64 into a zero-length target reads nothing - #459
[JSC] Uint8Array.prototype.setFromBase64 into a zero-length target reads nothing#459robobun wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-placed spec-compliance fix.
What was reviewed:
- Placement of the
maxLength == 0early return: it sits after the |this|/string type checks, options getters + validation, and the detached/OOB check, so all observable pre-FromBase64 steps still run in spec order. createSetFromResultObjectis a pure extraction of the existing result-object construction (same property order, sameputDirectcalls);setFromHexis behaviorally unchanged.- Confirmed
Uint8Array.fromBase64is untouched so short invalid inputs still throw, and the new stress test covers zero-length subarrays, length-tracking/resized views, getter side effects, and 1-byte targets that must still scan.
Extended reasoning...
Overview
The PR fixes a spec-compliance edge case in Uint8Array.prototype.setFromBase64: when the target has length 0, FromBase64 step 3 says nothing is read and no error can be raised, but JSC was still handing the empty output span to WTF::fromBase64 / simdutf, which scanned the input and threw on invalid characters or reported whitespace as read. The fix adds a if (!maxLength) return {read: 0, written: 0} guard immediately after the detached/out-of-bounds check in uint8ArrayPrototypeSetFromBase64, and factors the two identical {read, written} result-object constructions (in setFromBase64 and setFromHex) into a static createSetFromResultObject helper. A thorough new stress test exercises ~30 inputs × 26 option combinations across four zero-length target shapes plus all the validation that must still run.
Security risks
None. This is a JS-visible behavior change that makes an edge case more permissive (no longer throwing on garbage input when the target is empty), which is exactly what the spec and V8 do. No new parsing, no buffer writes, no pointer arithmetic — the new path skips work rather than adding it. The refactored helper is a byte-for-byte extraction of existing code.
Level of scrutiny
Low-to-moderate. The C++ change is ~15 lines net in a single host function, mechanically simple, and directly maps to a specific spec step with a test262 case (trailing-garbage-empty.js) that this fixes. The only subtlety — ordering of the early return relative to observable side effects (options getters, detach checks) — is handled correctly and explicitly covered by the new stress test (getter-order assertion, getter-throws, detach-before-call, detach-in-getter, OOB zero-length view). The setFromHex change is a pure refactor with no behavioral delta.
Other factors
The PR description is unusually detailed: it explains why the guard lives at the call site rather than in WTF::fromBase64 (so Uint8Array.fromBase64 still rejects short garbage), why setFromHex needs no equivalent guard (odd-length check precedes maxLength in FromHex), and why the stale expectations.yaml entry is left for a follow-up. The new test was cross-validated against node 26. No prior reviewer comments on the timeline. This addresses a known upstream test262 failure with a minimal, well-understood change.
Preview Builds
|
…ads nothing
FromBase64 step 3 returns { read: 0, written: 0 } when maxLength (the
target's length) is 0, before looking at the string. setFromBase64 handed
the empty span to WTF::fromBase64 anyway, so simdutf scanned the input and
a zero-length target still rejected invalid strings ("!!!", "Q" with
lastChunkHandling "strict") and reported whitespace as read (" " gave
read: 2). Return the empty result right after the out-of-bounds check,
which is where the spec invokes FromBase64; argument and option validation
and the detached check still run first.
This is test262's setFromBase64/trailing-garbage-empty.js, which now
passes, so its entries leave expectations.yaml and expectations-linux.yaml.
9203122 to
b5fc302
Compare
|
Warning Review limit reached
Next review available in: 9 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The change is small and well-scoped, but since it alters spec-observable behavior in a JSC runtime built-in, a human look would still be worthwhile.
What was reviewed:
- The zero-length early return is placed after the detached/out-of-bounds check and all options getters, so spec-mandated observable ordering (type checks,
alphabet/lastChunkHandlinggetters, detach TypeError) is preserved — the new stress test covers each of these. createSetFromResultObjectis a pure extraction of the existing result-object construction; property order (read,written) andputDirectsemantics are unchanged, andsetFromHexbehavior is identical.- Skipping
jsString->view()on the zero-length path is safe: the value is already a primitiveJSString, so resolving the view has no spec-observable side effects to preserve. Uint8Array.fromBase64is untouched and still rejects invalid input (covered by the new test).
Extended reasoning...
Overview
This PR fixes Uint8Array.prototype.setFromBase64 to comply with FromBase64 step 3: when the target typed array has length 0, return { read: 0, written: 0 } without scanning the input string. The change touches four files: a ~20-line edit to JSGenericTypedArrayViewPrototype.cpp (the early return plus extracting a createSetFromResultObject helper reused in setFromBase64 and setFromHex), a new 182-line stress test, and removal of the now-passing trailing-garbage-empty.js entry from both test262 expectations files.
Security risks
None identified. The change adds an early return that reduces the amount of input processing on a code path where no output buffer exists; there is no new parsing, no new buffer arithmetic, and no user-controlled indexing. The extracted helper is a pure refactor of existing object construction.
Level of scrutiny
Moderate-to-high. This is JSC runtime code implementing a TC39-specified built-in, so any change affects spec-observable behavior across all Bun users. The change itself is small and mechanical — a guarded early return with a spec citation — and is backed by a comprehensive stress test cross-validated against Node 26. Still, runtime built-in changes in a JS engine are the kind of thing a maintainer should sign off on rather than a bot.
Other factors
- The early return is correctly placed after
isIntegerIndexedObjectOutOfBounds, so detached buffers and out-of-bounds resizable views still throw TypeError before the shortcut applies (verified by test cases in the new stress test). - The PR description states the expectations.yaml entry "is not removed here" to avoid a merge conflict, but the diff does remove it from both files — the description appears to predate a rebase. This is a documentation inconsistency, not a code issue, but worth the author confirming the expectations files are in the intended state.
- The
createSetFromResultObjecthelper preserves the exact prior behavior (sameconstructEmptyObject+putDirectcalls, same property order), sosetFromHexis behaviorally unchanged. - Test coverage is thorough: 30 inputs × 26 option combinations × 4 zero-length target shapes, plus explicit ordering/error-path checks and a regression guard that
Uint8Array.fromBase64(which has no maxLength) still rejects invalid input.
…{ read: 0, written: 0 } (WebKit bump)
Picks up oven-sh/WebKit#459. FromBase64 step 3 returns before looking at
the string when maxLength (the target's length) is 0; JSC handed the empty
span to simdutf anyway, so a zero-length target still threw on invalid
input and reported whitespace as read, unlike node.
WEBKIT_VERSION points at that PR's preview build until it is merged.
1 participant
Problem
Uint8Array.prototype.setFromBase64into a zero-length target still parses the string. The spec returns before looking at it (FromBase64 step 3: if maxLength is 0, return{ Read: 0, Bytes: « », Error: none };setFromBase64passes the target's length as maxLength), and V8 (node 26) does that:Cause:
uint8ArrayPrototypeSetFromBase64(JSGenericTypedArrayViewPrototype.cpp) hands the empty output span toWTF::fromBase64, andsimdutf::base64_to_binary_safescans the whole input even when it has nowhere to write, so invalid characters, a partial last chunk and whitespace are all still reported. Targets of length 1 and up are unaffected (the length 0 shortcut is the only thing simdutf does not implement; this came out of a fuzz differential against node where all 183 mismatches had a zero-length target).This is test262's
built-ins/Uint8Array/prototype/setFromBase64/trailing-garbage-empty.js, whichJSTests/test262/expectations.yamllists as an expected failure (upstream fails it too).Fix
{ read: 0, written: 0 }when the target's length is 0, without resolving the string's view. Everything the spec runs before that still runs in the same order: the Uint8Array and string type checks, the options object check, thealphabetandlastChunkHandlinggetters and their validation, and the detached / out-of-bounds TypeError (including a detach performed by an options getter).{ read, written }result constructions insetFromBase64andsetFromHexmove into onecreateSetFromResultObjecthelper, so the early return is one line;setFromHexis otherwise unchanged (FromHex checks the odd-length case before maxLength, and a zero-length target already returns{ read: 0, written: 0 }there).Uint8Array.fromBase64is deliberately not touched: it has no maxLength, and it sizes its buffer withmaxLengthFromBase64, which is 0 for inputs like"!"that must still throw. That is why the shortcut lives insetFromBase64rather than inWTF::fromBase64.JSTests/stress/uint8array-setFromBase64-empty-target.js. 30 inputs (garbage, partial chunks, bad and excess padding, whitespace, the other alphabet's characters, non-Latin1 characters) x 26 option combinations againstnew Uint8Array(0), a zero-lengthsubarrayin the middle of a buffer (contents unchanged), a length-tracking view whose buffer was resized to 0 and a fixed zero-length view at the end of a grown buffer; the validation that still has to happen on a zero-length target (non-string argument, non-object options, badalphabet/lastChunkHandlingvalues, getter order, getters throwing, detached before the call and from inside a getter, a zero-length view that went out of bounds); that a 1-byte target and a length-tracking view grown to 1 byte are scanned again; and thatUint8Array.fromBase64still rejects"#","a","=".c6cfe90cjsc(linux-amd64-debug-asan prebuilt) the new test fails at its first zero-length case with the SyntaxError above and test262'strailing-garbage-empty.jsfails the same way; the existinguint8array-setFromBase64*.js/uint8array-base64-bad-char-decode.jsstress tests and test262'starget-size.jspass there (baseline). The new test passes as written on node 26 (with$.detachArrayBuffershimmed), so its expectations are the spec's, not this patch's. Pass-after, with this PR's preview build (autobuild-preview-pr-459-9203122d, all 42 variants built) pinned in a debug ASAN Bun: the new stress test, the three existinguint8array-setFromBase64*/uint8array-base64-bad-char-decodestress tests, test262'strailing-garbage-empty.jsand the ten othersetFromBase64test262 files all pass, and a 54-cell comparison ofsetFromBase64results against node 26 (zero-length and exactly-full targets, everylastChunkHandling) has no differences (11 before, all zero-length).test/js/bun/jsc/uint8array-base64.test.ts, which fails 21 of 29 tests on the current engine and passes on this one. Neither repo's CI runs JSTests, so that file is the CI pin for the behavior; the Bun PR gets re-pinned to the merged commit once this lands.trailing-garbage-empty.jsentry inJSTests/test262/expectations.yamlbecomes stale with this change. It is not removed here because this branch sits onc6cfe90c(the commit Bun pins, so the preview build is exactly pin plus this change) andmainhas since regenerated that file in a different format; deleting the entry here would only produce a conflict. It can be dropped with the nexttest262-runner --save.trailing-garbage.js(a 3-byte target and"aaaa#": the spec returns after the chunk that fills the target, simdutf keeps scanning and reports the#). node 26 behaves like JSC there, so it stays as is; fixing it means changing simdutf's full-output handling, not this call site.Background
Uint8Array.fromBase64(no maxLength) andsetFromBase64(maxLength = target length). It returns how many characters it consumed (read), the bytes, and an optional error;setFromBase64reportsreadand the byte count aswritten. Step 3 is an explicit shortcut for maxLength 0, which is what makes a full target observably different from a too-small one: a too-small target decodes until it runs out of room, a zero-length one never starts.WTF::fromBase64is a thin wrapper oversimdutf::base64_to_binary_safe, which decodes as much as fits and otherwise validates the remaining input; it has no notion of the spec's step 3, so the caller that has a maxLength has to apply it.isIntegerIndexedObjectOutOfBoundsis the detached / shrunk-resizable-buffer check (spec steps 11 and 12);JSArrayBufferView::length()after it is the spec's TypedArrayLength, which is also 0 for a length-tracking view over a buffer that was resized to 0.