jsc: TypedArray indexed access at 4294967295 on a 2**32-length view - #35876
jsc: TypedArray indexed access at 4294967295 on a 2**32-length view#35876robobun wants to merge 5 commits into
Conversation
Bump WEBKIT_VERSION to the oven-sh/WebKit#348 preview build and add a regression test. parseIndex() is capped at MAX_ARRAY_INDEX (0xFFFFFFFE) for regular Arrays, but a 1-byte TypedArray can have length 2**32, so index 0xFFFFFFFF is valid. The PropertyName overloads of getOwnPropertySlot/put/defineOwnProperty/deleteProperty now accept it.
|
Status: build #82196 finished 195 passed / 1 failed. The new test oven-sh/WebKit#348 preview build Once oven-sh/WebKit#348 merges, |
|
Warning Review limit reached
Next review available in: 12 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 |
Object.hasOwn / get_by_id / in_by_id can memoise "4294967295 is absent" on the shared TypedArray structure and then return that stale answer for a 2**32-length view. Repoint WEBKIT_VERSION at the c106f67d preview (oven-sh/WebKit#348 revised) which widens HasOwnPropertyCache::tryAdd and the three TypedArray canonical-numeric-string short-circuits in generateConditions / prepareChainForCaching / PolyProtoAccessChain to refuse caching when parseTypedArrayIndex() accepts the key. Cover the hasOwn(small) -> hasOwn(big) and warmed-IC(small) -> probe(big) orderings in the test.
|
Updated 3:58 AM PT - Jul 26th, 2026
❌ @robobun, your commit e83092d has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35876That installs a local version of the PR into your bun-35876 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
No issues found in the test (the earlier ASAN_OPTIONS nit is now applied), but this is a JSC engine bump — it points WEBKIT_VERSION at a preview build of an unmerged WebKit PR that changes TypedArray property lookup and the structure-keyed IC/HasOwnPropertyCache guards, and the range also pulls in five unrelated oven-sh/WebKit PRs. That's engine-critical and the actual code lives in oven-sh/WebKit#348, so a maintainer should sign off (and the preview tag needs repointing once #348 lands).
What was reviewed
- The new test: subprocess pipe draining, assertion ordering, JSON round-trip of
undefinedintoEqual, ASAN skip path — all check out. - Spec expectations at index 4294967295 (
gopd.configurable: true,deleteInBounds: false,smallDelete: true) match ECMA-262 IntegerIndexedElement semantics. prebuiltDestDir/prebuiltUrlhandling of theautobuild-prefix — the existing branch already covers it.
Extended reasoning...
Overview
Two files: a one-line WEBKIT_VERSION bump in scripts/build/deps/webkit.ts (from a merged-main sha to the preview tag autobuild-preview-pr-348-c106f67d) and a new 146-line test at test/js/bun/jsc/typedarray-uint32max-index.test.ts. The behavioural change itself — making index 4294967295 reachable on a 2**32-length TypedArray via parseTypedArrayIndex and widening the HasOwnPropertyCache / IC miss-condition guards — lives entirely in oven-sh/WebKit#348, not in this diff.
Security risks
None introduced by the files in this repo. The engine-side change widens a bounds check by exactly one value (UINT32_MAX) on the property-name path only; the unsigned-index path already handled it via inBounds(). No new untrusted-input parsing here. The test allocates 4 GiB in a child process with a graceful skip; no network, no filesystem writes outside the temp allocation.
Level of scrutiny
High. WebKit bumps are the highest-blast-radius change in this repo: this one alters property lookup on a core JS type and simultaneously touches four IC/cache sites (HasOwnPropertyCache::tryAdd, generateConditions, prepareChainForCaching, PolyProtoAccessChain::tryCreate) whose correctness I cannot verify from this diff. The bump also carries five other WebKit PRs (#317, #328, #331, #332, #347) in the same range. The version is pinned to a preview release that the description says will be deleted once the WebKit PR merges — so merging this as-is would leave main pointing at an ephemeral artifact. All of that is squarely maintainer territory.
Other factors
- My earlier inline nit (ASAN
allocator_may_return_null=1so the 4 GiB skip path works) was applied in a901bb3 and the thread is resolved. - The test is well-constructed against REVIEW.md: pipes drained concurrently via
Promise.all, stderr asserted before parsing stdout, exit code asserted last, exact-valuetoEqualon the whole result object, variant matrix covered (numeric vs string key,.at,in,hasOwn, gOPD, defineProperty, Reflect.set/deleteProperty, short-view negative case, adjacent canonical numeric strings, Int8/Uint8Clamped views, and the small→big cache-ordering probes). - I checked that
JSON.stringifydropping theundefined-valued keys (oneBeyond,minusZero,smallGet) is fine becausetoEqualtreats missing andundefinedas equivalent. webkit.tsalready special-casesautobuild--prefixed versions in both the release-tag URL andprebuiltDestDir, so the preview tag resolves without further changes.
Given the engine-critical scope, the bundled unrelated WebKit changes, and the temporary preview pin, I'm deferring rather than approving.
There was a problem hiding this comment.
No issues found in the test (the ASAN allocator_may_return_null nit from the earlier pass is applied). Deferring to a human for the WebKit bump itself: it points at a preview-build tag that will be deleted once oven-sh/WebKit#348 merges, and the range pulls in five other JSC changes (#317/#328/#331/#332/#347) beyond the TypedArray fix — that's an engine-level dependency bump touching property-access ICs and cache guards, which warrants maintainer sign-off.
What was reviewed
- Test fixture: subprocess pipe draining,
ASAN_OPTIONSskip path, expected values againstIsValidIntegerIndex(incl.deleteInBounds: false,smallDelete: true,gopd.configurable: true). webkit.ts: theautobuild-tag is handled by the existingprebuiltUrl/prebuiltDestDirbranches, so no build-script breakage.- IC/HasOwnPropertyCache ordering assertions match the widened guards described for c106f67d.
Extended reasoning...
Overview
Two files: a one-line WEBKIT_VERSION bump in scripts/build/deps/webkit.ts from a merged-main sha to autobuild-preview-pr-348-c106f67d, and a new 146-line test at test/js/bun/jsc/typedarray-uint32max-index.test.ts. The behavioural change lives entirely in oven-sh/WebKit#348 (adds parseTypedArrayIndex, routes four JSGenericTypedArrayView PropertyName overloads through it, and widens the HasOwnPropertyCache::tryAdd / generateConditions / prepareChainForCaching / PolyProtoAccessChain::tryCreate guards so a length-dependent answer at key "4294967295" isn't memoised on the shared TypedArray structure). The bump also carries five unrelated WebKit PRs in the 549170099226..c106f67d range.
Security risks
None identified in the bun-side diff. The engine change tightens spec conformance for a boundary index rather than relaxing any check; no untrusted-input parsing or auth surface is touched here. The bundled WebKit changes (LiteralParser OOM → RangeError, SignalsWin VEH ordering, inspector throw-scope release, a Heap option, async-stack unwrapping) are not reviewable from this diff and are part of why a human should look.
Level of scrutiny
High. A WebKit version bump is a JS-engine dependency change: the modified paths (getOwnPropertySlot/put/defineOwnProperty/deleteProperty, IC miss-condition generation, HasOwnPropertyCache) are core to every property access. The author already found one correctness bug in their own first revision (stale structure-keyed miss cache), which underlines how subtle this area is. Additionally, WEBKIT_VERSION is being pointed at a preview release tag that the description says will be deleted once the upstream PR merges — a maintainer should decide whether to land against the preview or wait for the merged sha.
Other factors
- My earlier inline nit (ASAN
allocator_may_return_null=1so the 4 GiB skip path works) was applied in a901bb3 and the thread is resolved; the current diff carries the fix. - The test is well-constructed per REVIEW.md conventions: spawned subprocess with concurrent stdout/stderr/exited drain,
bunEnvspread, exact-valuetoEqualon a JSON payload, negative cases ("4294967296","-0", short view), sibling element types, and the small→big cache-ordering probes.JSON.stringifydropsundefinedkeys, buttoEqualtreats missing andundefinedas equivalent, so theoneBeyond/minusZero/smallGetassertions hold. scripts/build/deps/webkit.tsalready special-casesautobuild-prefixes in bothprebuiltUrl(tag passthrough) andprebuiltDestDir(uses the full slug instead of a 16-char slice), so the preview tag doesn't break the build machinery.- No CODEOWNERS or outstanding human review comments; CI was retriggered after the preview tarballs published but the build result isn't visible in the timeline yet.
Problem
IsValidIntegerIndex(O, index)only requires0 <= index < O.[[ArrayLength]]. AUint8Arraycan have length up toMAX_ARRAY_BUFFER_SIZE(2**32on 64-bit), so4294967295is a valid index there, and V8 treats it as one.Cause
JSGenericTypedArrayView::{getOwnPropertySlot, put, defineOwnProperty, deleteProperty}(PropertyName)callparseIndex(), which is capped atMAX_ARRAY_INDEX = 0xFFFFFFFEbecause a regular JS Array's length is a uint32 and the largest index islength - 1. For"4294967295"parseIndexreturnsnullopt, and theisCanonicalNumericIndexStringfallback treats it as always out of bounds. Theunsigned-index overloads (getOwnPropertySlotByIndexetc.) already compare against the fullsize_tlength viainBounds(), which is why4294967295 in u(routed throughgetUInt32→hasProperty(unsigned)) returnstruewhileu[4294967295]returnsundefined.Fix
oven-sh/WebKit#348 adds
parseTypedArrayIndex(PropertyName)inPropertyName.hthat extendsparseIndex()to the full uint32 range (UINT32_MAXis the only extra value, handled as a literal compare afterparseIndexreturnsnullopt) and routes the fourJSGenericTypedArrayViewPropertyName overloads through it.TypedArray structures are shared per type, not per length, so once
getOwnPropertySlot("4294967295")becomes length-dependent the structure-keyed caches that previously memoised "always absent" must be told not to:HasOwnPropertyCache::tryAddwidens its index guard toparseTypedArrayIndex, and theisTypedArrayType && isCanonicalNumericIndexStringshort-circuits ingenerateConditions/prepareChainForCaching/PolyProtoAccessChain::tryCreatereturn invalid instead of a valid empty condition set whenparseTypedArrayIndexaccepts the key, so noMiss/InMissIC is installed for it on a TypedArray. Without those guards,Object.hasOwn(short, "4294967295"); Object.hasOwn(long, "4294967295")tripsASSERT(*result == hasOwnProperty(...))inObjectPrototype.cppon assert-enabled builds, and a warmed IC returns the stale miss in release builds.The DFG/FTL inline fast paths speculate Int32 for the index and OSR-exit or fall to the C++ slow path for
0xFFFFFFFF, so no JIT change is needed.This PR bumps
WEBKIT_VERSIONto theautobuild-preview-pr-348-c106f67dpreview build and addstest/js/bun/jsc/typedarray-uint32max-index.test.ts, which spawns a child that allocates a2**32-byteUint8Arrayand exercises[[Get]],[[Set]],.at,in,Object.hasOwn,Object.getOwnPropertyDescriptor,Object.defineProperty,Reflect.setandReflect.deletePropertyat index4294967295, the same key on a short view (still out of bounds), the adjacent canonical numeric strings"4294967296"/"-0"(stillundefined),Int8Array/Uint8ClampedArrayviews over the same buffer, and thehasOwn(small)→hasOwn(big)/ warmed-ICprobe(small)→probe(big)orderings that exercise the cache guards. The child skips cleanly if the allocation is refused.Also in this range (549170099226..c106f67d)
minEdenToOldGenerationRatioa JSC option (same default; behaviour unchanged)Verification
Node 26.3.0 on the same fixture prints the direct-access values exactly. For the warmed-IC ordering Node returns
undefined/false(V8 shares the stale-miss behaviour), which is a V8 bug againstIsValidIntegerIndex; this change gives the spec answer there.Found while fixing
Buffer.writeUInt8rejecting offset4294967295(#35867 works around it at the Buffer layer; this closes the engine-level gap).Once oven-sh/WebKit#348 merges,
WEBKIT_VERSIONshould be repointed at the merged main sha (the preview release is deleted at that point).[decide:webkit] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file