[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
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
|
9203122 to
b5fc302
Compare
|
Warning Review limit reached
On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file. Or wait 13 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ 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.
b5fc302 to
39d09bd
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it modifies JSC runtime behavior in Source/JavaScriptCore/runtime/ (CODEOWNERS-covered), a human look is still worthwhile.
What was reviewed:
- Spec ordering: the
maxLength == 0early return sits after the string/options/alphabet/lastChunkHandling validation and the out-of-bounds check, so all observable pre-FromBase64 steps still run — matches the stress test's getter-order and detach-in-getter cases. createSetFromResultObjectis a pure extraction of the existingconstructEmptyObject+ twoputDirectcalls;setFromHexbehavior is unchanged.- Skipping
jsString->view()on the zero-length path only elides rope resolution — the argument was already type-checked as a primitive string, so no observable step is dropped. Uint8Array.fromBase64is untouched and the new test confirms it still throws on invalid short inputs.
Extended reasoning...
Overview
This PR fixes a spec-conformance bug in Uint8Array.prototype.setFromBase64: when the target typed array has length 0, the spec's FromBase64 operation (step 3) returns { Read: 0, Bytes: «», Error: none } without inspecting the input string, but JSC was still passing the string to WTF::fromBase64 / simdutf, which scanned it and reported syntax errors. The fix adds a two-line early return in uint8ArrayPrototypeSetFromBase64 after the detached/OOB check, factors the { read, written } result construction into a shared createSetFromResultObject helper (also used by setFromHex), removes the now-passing trailing-garbage-empty.js entry from both test262 expectations files, and adds a very thorough stress test.
Security risks
None identified. The change narrows behavior (returns early instead of decoding) and only when the output buffer is empty, so no new memory is written and no new input is parsed. The std::span passed to fromBase64 in the non-zero path uses the same uint8Array->length() value as before, just via a local. No auth, crypto, or permissions code is touched.
Level of scrutiny
Moderate-to-high. The functional change is tiny and mechanically obvious — an if (!maxLength) return {0,0} guard placed at exactly the point the spec places it, plus a helper extraction that is byte-for-byte equivalent to the inlined code it replaces. The reasoning in the PR description is precise, the new stress test is comprehensive (30 inputs × 26 option combos × 4 zero-length target shapes, plus ordering/throw/detach checks and 1-byte regression checks), and the author cross-validated against node 26 and test262. However, this is a change to Source/JavaScriptCore/runtime/, which is production-critical for every Bun user and is under @WebKit/jsc-reviewers in CODEOWNERS. Per policy, JSC runtime changes should get a human sign-off even when the automated review is clean.
Other factors
- No prior human reviews or unresolved comments on the PR.
- All 42 preview build variants built successfully per the PR description.
- Minor: the PR description says the
expectations.yamlentry "is not removed here", but the diff does remove it from both expectations files — the description appears to predate the second commit. This is cosmetic and doesn't affect the code. - The sibling
trailing-garbage.jsfailure is intentionally left in place with a clear rationale (simdutf-level fix, node behaves the same).
…{ 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.
39d09bd to
3794f8f
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The change is small and matches the spec, but since it alters observable JS engine semantics in a runtime host function, a human look would still be worthwhile.
Checked: the early return sits after all spec-ordered validation (this/string/options/alphabet/lastChunkHandling/detached), so getter order and TypeErrors are preserved on zero-length targets.
Checked: createSetFromResultObject produces the same {read, written} shape and property order as the code it replaces; setFromHex is behaviorally unchanged.
Checked: skipping jsString->view() on the zero-length path has no observable side effects (JSString is already a primitive; only rope resolution is skipped).
Checked: Uint8Array.fromBase64 is untouched, so single-char garbage still throws — covered by the new stress test.
Extended reasoning...
Overview
The PR fixes a spec-conformance bug in Uint8Array.prototype.setFromBase64: when the target has length 0, FromBase64 step 3 says nothing is read and no error can occur, but JSC was still handing the string to simdutf and reporting SyntaxErrors / non-zero read counts. The fix is a 3-line early return in uint8ArrayPrototypeSetFromBase64 (JSGenericTypedArrayViewPrototype.cpp) placed immediately after the detached/out-of-bounds check, plus extraction of the {read, written} result construction into a shared createSetFromResultObject helper (also used by setFromHex, unchanged in behavior). A comprehensive stress test is added, and the now-passing trailing-garbage-empty.js entry is removed from both test262 expectations files.
Security risks
None identified. The change short-circuits before any buffer write and only when the output span is empty; no new memory is touched. The hoisted maxLength is read after the OOB check, matching the previous uint8Array->length() call site. There is no parsing of untrusted input added — strictly less input processing than before.
Level of scrutiny
Moderate-to-high. The diff itself is tiny and mechanically obvious — an early return guarded by !maxLength, plus a pure helper extraction — and the PR description documents exhaustive verification against the spec, node 26, test262, and ASAN builds. However, this is a JSC runtime host function whose behavior is directly observable from JavaScript, and it changes error-vs-success semantics for a builtin. Engine-semantics changes in this repo conventionally get a human sign-off even when small, so I'm deferring rather than approving.
Other factors
- The early return is correctly ordered: all spec steps before FromBase64 (receiver check, string check, options coercion with
alphabet/lastChunkHandlinggetters, detached/OOB TypeError) still run. The new stress test explicitly asserts getter order, getter throws, and detach-from-getter on a zero-length target. createSetFromResultObjectis a byte-for-byte extraction: sameconstructEmptyObject+ twoputDirectcalls in the same order with the same property names, so result-object shape is unchanged for bothsetFromBase64andsetFromHex.- Skipping
jsString->view()on the zero-length path only skips rope resolution / potential OOM, which is not spec-observable. Uint8Array.fromBase64is deliberately untouched (it has no maxLength), and the stress test pins that it still rejects invalid short inputs.- No prior human or bot reviews with substantive feedback; no outstanding comments to address.
…{ 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.
3794f8f to
fb198d8
Compare
There was a problem hiding this comment.
LGTM — small, well-scoped spec-compliance fix with thorough test coverage.
What was reviewed:
- The early return is placed after the detached/out-of-bounds check and after all options-getter validation, so spec step ordering (type checks,
alphabet/lastChunkHandlinggetters, detach TypeError) is preserved — matches the new stress test's ordering assertions. createSetFromResultObjectis a pure extraction of the two identical{read, written}construction sites;setFromHexbehavior is unchanged.- Caching
uint8Array->length()intomaxLengthis safe — no user code runs between the read and its use infromBase64(onlyjsString->view(), which is rope resolution). Uint8Array.fromBase64is correctly left alone; the shortcut is caller-side, not inWTF::fromBase64.
Extended reasoning...
Overview
This PR fixes a spec-compliance bug in Uint8Array.prototype.setFromBase64: when the target array has length 0, the spec's FromBase64 step 3 says to return { Read: 0, Bytes: «», Error: none } immediately without scanning the input string. JSC was instead handing the empty output span to simdutf::base64_to_binary_safe, which scans and validates the whole input regardless, causing spurious SyntaxErrors and non-zero read counts on whitespace.
The C++ change in JSGenericTypedArrayViewPrototype.cpp is ~15 net lines: a new createSetFromResultObject helper extracted from two identical construction sites, and a 3-line early return in uint8ArrayPrototypeSetFromBase64 when uint8Array->length() is 0. The remaining changes are a comprehensive 182-line stress test and removal of the now-passing trailing-garbage-empty.js entry from both test262 expectations files.
Security risks
None. The change adds an early return that skips work (string resolution and base64 decoding) in a case where the output buffer is empty. No new buffer writes, no new user-controlled sizing, no new code paths that touch memory. The createSetFromResultObject refactor is a verbatim extraction of existing object-construction code.
Level of scrutiny
Low-to-moderate. This is runtime code in JavaScriptCore, but the change is a narrow, additive early return guarded by a simple length check, placed at exactly the spec-mandated point (after the detached check, before FromBase64 would begin). The only subtlety is step ordering — and the PR gets it right: all observable side effects (this-check, string-check, options-object check, alphabet and lastChunkHandling getters and their validation, the detached/out-of-bounds TypeError) run before the shortcut. The stress test explicitly asserts each of these, and the author cross-checked the test against node 26 to confirm the expectations are the spec's rather than the patch's.
I also verified that caching length() into maxLength and reusing it for the fromBase64 span is safe: between the length read and the span construction, only jsString->view() runs, which is rope resolution and cannot run user code that would resize or detach the buffer.
Other factors
- The PR removes a known test262 failure (
trailing-garbage-empty.js) and explicitly leaves the siblingtrailing-garbage.jsin place with a clear rationale (that one requires a simdutf-level change and node behaves the same). - The author verified pass-before/pass-after on debug-ASAN builds and against a companion Bun PR with CI-pinned tests.
- No prior human or bot reviews with substantive feedback; no outstanding comments to address.
- The bug-hunting system found no issues.
…{ 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.
…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.
fb198d8 to
523b0cd
Compare
There was a problem hiding this comment.
LGTM — small, well-justified spec-conformance fix with thorough test coverage.
What was reviewed:
- Early-return placement: sits after the Uint8Array/string/options/detached checks and before string resolution, matching FromBase64 step 3 ordering.
createSetFromResultObjecthelper: pure refactor of the two identical result-object constructions;setFromHexbehavior is unchanged.- Skipping
jsString->view()on the zero-length path has no observable side effects (already type-checked as a primitive string). - test262 expectations removal matches the now-passing
trailing-garbage-empty.js.
Extended reasoning...
Overview
This PR fixes Uint8Array.prototype.setFromBase64 to short-circuit with {read: 0, written: 0} when the target has zero length, per FromBase64 step 3 of the spec. The C++ change in JSGenericTypedArrayViewPrototype.cpp is ~15 net lines: a new createSetFromResultObject helper (deduplicating the result-object construction shared with setFromHex), a length-zero early return placed immediately after the out-of-bounds check, and reuse of the cached maxLength in the existing fromBase64 call. The rest is a comprehensive new stress test and removal of the now-passing trailing-garbage-empty.js from both test262 expectations files.
Security risks
None. The change adds an early return that avoids parsing input in a case where no output could be written anyway. No new buffer writes, no relaxed bounds checks, no untrusted-size arithmetic. The setFromHex change is a pure refactor to call the shared helper.
Level of scrutiny
Moderate — this is JSC runtime code that ships in Bun, but the change is minimal, mechanical, and directly implements a specific spec step. I verified the early return is positioned after all spec-mandated pre-checks (this-value type, string argument type, options coercion and getter invocation, detached/OOB TypeError), so observable ordering is preserved; the new stress test explicitly covers each of these. Skipping jsString->view() is safe because the argument is already confirmed to be a primitive string — resolving a rope has no user-observable side effects. The helper's re-fetch of vm from globalObject is equivalent to the previously-in-scope vm.
Other factors
The PR description documents extensive verification: the new test fails on the pre-fix build and passes on the preview build, passes on node 26 (independent oracle), and the fix resolves a known test262 failure. All 42 preview build variants built successfully across five rebases with identical results. There are no outstanding reviewer comments. The author correctly scoped the fix to setFromBase64 only, explaining why Uint8Array.fromBase64 and setFromHex do not need the same treatment.
…{ 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.
|
Closing as superseded. Upstream WebKit fixed this in Same semantics as this PR, one layer lower, so there is nothing left to merge here. The stress test from this branch ( |
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.yamllisted 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).c6cfe90c(the Bun pin at the time), then re-based unchanged ontoeeab0404,0f966e81,b7f217b4,aea1f010and nowc148a12das Bun's pin moved; the current preview isautobuild-preview-pr-459-523b0cd7, and every preview gave the same results in Bun.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.jsentries inJSTests/test262/expectations.yamlandexpectations-linux.yamlare removed, since the test passes now (trailing-garbage.jsstays listed, see below).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.