Skip to content

[JSC] Uint8Array.prototype.setFromBase64 into a zero-length target reads nothing - #459

Closed
robobun wants to merge 1 commit into
mainfrom
farm/190c9b83/set-from-base64-empty-target
Closed

[JSC] Uint8Array.prototype.setFromBase64 into a zero-length target reads nothing#459
robobun wants to merge 1 commit into
mainfrom
farm/190c9b83/set-from-base64-empty-target

Conversation

@robobun

@robobun robobun commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Uint8Array.prototype.setFromBase64 into 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 }; setFromBase64 passes the target's length as maxLength), and V8 (node 26) does that:

    new Uint8Array(0).setFromBase64('!!!')                                 // JSC: SyntaxError        spec / node: { read: 0, written: 0 }
    new Uint8Array(0).setFromBase64('Q', { lastChunkHandling: 'strict' })  // JSC: SyntaxError        spec / node: { read: 0, written: 0 }
    new Uint8Array(0).setFromBase64('  ')                                  // JSC: { read: 2, ... }   spec / node: { read: 0, written: 0 }
  • Cause: uint8ArrayPrototypeSetFromBase64 (JSGenericTypedArrayViewPrototype.cpp) hands the empty output span to WTF::fromBase64, and simdutf::base64_to_binary_safe scans 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, which JSTests/test262/expectations.yaml listed as an expected failure (upstream fails it too).

Fix

  • After the out-of-bounds (detached) check, which is where the spec invokes FromBase64, return { 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, the alphabet and lastChunkHandling getters and their validation, and the detached / out-of-bounds TypeError (including a detach performed by an options getter).
  • The two { read, written } result constructions in setFromBase64 and setFromHex move into one createSetFromResultObject helper, so the early return is one line; setFromHex is otherwise unchanged (FromHex checks the odd-length case before maxLength, and a zero-length target already returns { read: 0, written: 0 } there).
  • Uint8Array.fromBase64 is deliberately not touched: it has no maxLength, and it sizes its buffer with maxLengthFromBase64, which is 0 for inputs like "!" that must still throw. That is why the shortcut lives in setFromBase64 rather than in WTF::fromBase64.
  • Test: 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 against new Uint8Array(0), a zero-length subarray in 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, bad alphabet / lastChunkHandling values, 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 that Uint8Array.fromBase64 still rejects "#", "a", "=".
  • Verified: on the pinned c6cfe90c jsc (linux-amd64-debug-asan prebuilt) the new test fails at its first zero-length case with the SyntaxError above and test262's trailing-garbage-empty.js fails the same way; the existing uint8array-setFromBase64*.js / uint8array-base64-bad-char-decode.js stress tests and test262's target-size.js pass there (baseline). The new test passes as written on node 26 (with $.detachArrayBuffer shimmed), 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 existing uint8array-setFromBase64* / uint8array-base64-bad-char-decode stress tests, test262's trailing-garbage-empty.js and the ten other setFromBase64 test262 files all pass, and a 54-cell comparison of setFromBase64 results against node 26 (zero-length and exactly-full targets, every lastChunkHandling) has no differences (11 before, all zero-length).
  • Branch history: the patch was first built on c6cfe90c (the Bun pin at the time), then re-based unchanged onto eeab0404, 0f966e81, b7f217b4, aea1f010 and now c148a12d as Bun's pin moved; the current preview is autobuild-preview-pr-459-523b0cd7, and every preview gave the same results in Bun.
  • Bun side: Test Uint8Array.prototype.setFromBase64 into a zero-length target bun#39453 pins the current preview and adds 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.
  • The trailing-garbage-empty.js entries in JSTests/test262/expectations.yaml and expectations-linux.yaml are removed, since the test passes now (trailing-garbage.js stays listed, see below).
  • Not changed: the sibling 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

  • FromBase64(string, alphabet, lastChunkHandling, maxLength) is the spec operation behind both Uint8Array.fromBase64 (no maxLength) and setFromBase64 (maxLength = target length). It returns how many characters it consumed (read), the bytes, and an optional error; setFromBase64 reports read and the byte count as written. 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::fromBase64 is a thin wrapper over simdutf::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.
  • isIntegerIndexedObjectOutOfBounds is 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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — small, well-placed spec-compliance fix.

What was reviewed:

  • Placement of the maxLength == 0 early 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.
  • createSetFromResultObject is a pure extraction of the existing result-object construction (same property order, same putDirect calls); setFromHex is behaviorally unchanged.
  • Confirmed Uint8Array.fromBase64 is 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.

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Preview Builds

Commit Release Date
523b0cd7 autobuild-preview-pr-459-523b0cd7 2026-08-24 12:58:06 UTC
fb198d86 autobuild-preview-pr-459-fb198d86 2026-08-23 08:59:04 UTC
3794f8f5 autobuild-preview-pr-459-3794f8f5 2026-08-21 03:29:50 UTC
39d09bd8 autobuild-preview-pr-459-39d09bd8 2026-08-19 14:09:35 UTC
b5fc3025 autobuild-preview-pr-459-b5fc3025 2026-08-18 05:49:18 UTC
9203122d autobuild-preview-pr-459-9203122d 2026-08-17 18:40:37 UTC

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

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 details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f60fa47a-ce8c-4060-b1eb-07f104a2c6ee

📥 Commits

Reviewing files that changed from the base of the PR and between c148a12 and 523b0cd.

📒 Files selected for processing (4)
  • JSTests/stress/uint8array-setFromBase64-empty-target.js
  • JSTests/test262/expectations-linux.yaml
  • JSTests/test262/expectations.yaml
  • Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.cpp

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/lastChunkHandling getters, detach TypeError) is preserved — the new stress test covers each of these.
  • createSetFromResultObject is a pure extraction of the existing result-object construction; property order (read, written) and putDirect semantics are unchanged, and setFromHex behavior is identical.
  • Skipping jsString->view() on the zero-length path is safe: the value is already a primitive JSString, so resolving the view has no spec-observable side effects to preserve.
  • Uint8Array.fromBase64 is 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 createSetFromResultObject helper preserves the exact prior behavior (same constructEmptyObject + putDirect calls, same property order), so setFromHex is 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.

robobun added a commit to oven-sh/bun that referenced this pull request Aug 18, 2026
…{ 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.
@robobun
robobun force-pushed the farm/190c9b83/set-from-base64-empty-target branch from b5fc302 to 39d09bd Compare August 19, 2026 07:53

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 == 0 early 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.
  • createSetFromResultObject is a pure extraction of the existing constructEmptyObject + two putDirect calls; setFromHex behavior 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.fromBase64 is 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.yaml entry "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.js failure is intentionally left in place with a clear rationale (simdutf-level fix, node behaves the same).

robobun added a commit to oven-sh/bun that referenced this pull request Aug 19, 2026
…{ 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.
@robobun
robobun force-pushed the farm/190c9b83/set-from-base64-empty-target branch from 39d09bd to 3794f8f Compare August 21, 2026 02:42

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/lastChunkHandling getters, detached/OOB TypeError) still run. The new stress test explicitly asserts getter order, getter throws, and detach-from-getter on a zero-length target.
  • createSetFromResultObject is a byte-for-byte extraction: same constructEmptyObject + two putDirect calls in the same order with the same property names, so result-object shape is unchanged for both setFromBase64 and setFromHex.
  • Skipping jsString->view() on the zero-length path only skips rope resolution / potential OOM, which is not spec-observable.
  • Uint8Array.fromBase64 is 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.

robobun added a commit to oven-sh/bun that referenced this pull request Aug 21, 2026
…{ 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.
@robobun
robobun force-pushed the farm/190c9b83/set-from-base64-empty-target branch from 3794f8f to fb198d8 Compare August 23, 2026 07:45

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/lastChunkHandling getters, detach TypeError) is preserved — matches the new stress test's ordering assertions.
  • createSetFromResultObject is a pure extraction of the two identical {read, written} construction sites; setFromHex behavior is unchanged.
  • Caching uint8Array->length() into maxLength is safe — no user code runs between the read and its use in fromBase64 (only jsString->view(), which is rope resolution).
  • Uint8Array.fromBase64 is correctly left alone; the shortcut is caller-side, not in WTF::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 sibling trailing-garbage.js in 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.

robobun added a commit to oven-sh/bun that referenced this pull request Aug 23, 2026
…{ 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.
@robobun
robobun force-pushed the farm/190c9b83/set-from-base64-empty-target branch from fb198d8 to 523b0cd Compare August 24, 2026 11:17

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
  • createSetFromResultObject helper: pure refactor of the two identical result-object constructions; setFromHex behavior 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.

robobun added a commit to oven-sh/bun that referenced this pull request Aug 24, 2026
…{ 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.
@robobun

robobun commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as superseded. Upstream WebKit fixed this in b1701b348908 ("[JSC] Check maxLength == 0 when specified in fromBase64", 2026-08-17): WTF::fromBase64 takes an OutputSizeIsMaxLength argument and returns { read: 0, written: 0 } for an empty output when setFromBase64 passes Yes, with Uint8Array.fromBase64 passing No, so "!" still throws there. That commit is on main since the 8c4fd56347 upgrade (#503), and the trailing-garbage-empty.js expectations entries are already gone.

Same semantics as this PR, one layer lower, so there is nothing left to merge here. The stress test from this branch (JSTests/stress/uint8array-setFromBase64-empty-target.js) passes on main's engine as well; oven-sh/bun#39453 keeps the bun:test coverage and is now test-only.

@robobun robobun closed this Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant