TypedArray: accept index 4294967295 in PropertyName get/put/define/delete - #348
TypedArray: accept index 4294967295 in PropertyName get/put/define/delete#348robobun wants to merge 1 commit into
Conversation
|
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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
…lete parseIndex() is capped at MAX_ARRAY_INDEX (0xFFFFFFFE) because a regular Array's length is a uint32 and the largest index is length - 1. A TypedArray's length is a size_t bounded by MAX_ARRAY_BUFFER_SIZE / elementSize, so for 1-byte element types the last valid index of a maximum-length view is 0xFFFFFFFF. The PropertyName overloads of getOwnPropertySlot / put / defineOwnProperty / deleteProperty on JSGenericTypedArrayView called parseIndex(), which returned nullopt for "4294967295", and then treated the key as any other canonical numeric index string (always out of bounds). The unsigned-index overloads already accepted 0xFFFFFFFF, so [[HasProperty]] via getUInt32() reported the element as present while [[Get]] returned undefined. Add parseTypedArrayIndex() that extends parseIndex() to the full uint32 range (the only extra value is UINT32_MAX) and route the four PropertyName overloads through it. Because TypedArray structures are shared per type and not per length, the structure-keyed caches that previously memoised "4294967295 is absent" must be told the answer is now length-dependent: - HasOwnPropertyCache::tryAdd widens its index guard from parseIndex to parseTypedArrayIndex so "4294967295" is never cached. - generateConditions / prepareChainForCaching / PolyProtoAccessChain return invalid instead of a valid empty condition set when the TypedArray canonical-numeric-string short-circuit sees a key that parseTypedArrayIndex accepts, so no Miss / InMiss IC is installed. IsValidIntegerIndex only requires 0 <= index < [[ArrayLength]] and imposes no MAX_ARRAY_INDEX cap.
1535a8d to
c106f67
Compare
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.
There was a problem hiding this comment.
The caching concerns from my earlier review are now addressed — HasOwnPropertyCache::tryAdd, generateConditions, prepareChainForCaching, and PolyProtoAccessChain::tryCreate all reject "4294967295" on TypedArrays. I didn't find any remaining issues, but this threads through the property-access slow path and three IC/caching subsystems, so a human familiar with JSC's inline-cache invariants should sign off.
Checked: parseTypedArrayIndex only widens parseIndex by the single literal "4294967295"; equal(uid, span8) handles 16-bit impls; inBounds(uint64_t) compares against size_t length so UINT32_MAX doesn't wrap. The three cache-refusal sites return invalid()/nullopt/nullptr (not a valid-empty set), so no Miss IC is installed. The HasOwnPropertyCache guard change is strictly wider than before — non-TypedArray callers only lose caching for the one extra key.
Extended reasoning...
Overview
Adds parseTypedArrayIndex() in PropertyName.h (identical to parseIndex() plus a literal match for "4294967295") and swaps the four JSGenericTypedArrayView PropertyName overloads (getOwnPropertySlot, put, defineOwnProperty, deleteProperty) to use it, so index 0xFFFFFFFF on a 4 GiB one-byte-element TypedArray routes to the ByIndex path instead of the always-absent canonical-numeric fallback. Because that makes absence of "4294967295" length-dependent rather than structure-determined, the revision also patches HasOwnPropertyCache::tryAdd, generateConditions, prepareChainForCaching, and PolyProtoAccessChain::tryCreate to refuse to cache/install a Miss for that key on a TypedArray structure.
Security risks
TypedArray indexed access is memory-safety-relevant. The new path funnels into the existing getOwnPropertySlotByIndex/putByIndex/deletePropertyByIndex, all of which already accept an unsigned and gate on inBounds(uint64_t) against the size_t length — no new arithmetic is introduced, and UINT32_MAX was already reachable on those overloads via hasProperty(unsigned). The IC-side changes are strictly reductive (refuse to cache where we previously cached), so they cannot introduce a stale-hit; the risk direction would be a missed refusal, and the three known chokepoints plus HasOwnPropertyCache are covered. I don't see a memory-safety or type-confusion vector here, but IC correctness in JSC is subtle enough that I'd want a maintainer to confirm no other structure-keyed absence path exists.
Level of scrutiny
High. This is core JSC runtime + inline-cache machinery. The individual edits are small and mechanical, but the reasoning behind them (structure-shared TypedArrays, which caches key on Structure alone, which slot states are cacheable) is exactly the kind of thing that benefits from a second pair of eyes from someone who owns this code. The earlier revision missed the caching interactions entirely, which is a signal that the surface is non-obvious.
Other factors
- My prior review's two findings are both fixed in this revision, using the suggested approach (widen
tryAdd's guard; return invalid/nullopt from the condition-set short-circuits rather than a valid empty set).PolyProtoAccessChain::tryCreatewas additionally covered. - The bug-hunting system found nothing on this revision.
- Regression tests live in the paired oven-sh/bun PR (per description), covering direct access,
hasOwnordering, and warmed-IC ordering — I can't verify them from here. - The PR description's claim that DFG/FTL fast paths speculate Int32 and OSR-exit for
0xFFFFFFFFis plausible but I did not independently trace every JIT tier; that's another reason for a JSC maintainer to look.
1 participant
Problem
IsValidIntegerIndex(O, index)only requires0 <= index < O.[[ArrayLength]]; there is noMAX_ARRAY_INDEXcap. For aUint8Arrayof length2**32(MAX_ARRAY_BUFFER_SIZE), index4294967295is valid.Cause
JSGenericTypedArrayView::{getOwnPropertySlot, put, defineOwnProperty, deleteProperty}(PropertyName)callparseIndex(propertyName), which is capped atMAX_ARRAY_INDEX = 0xFFFFFFFE(the largest valid Array index, since an Array's length is a uint32). For"4294967295"parseIndexreturnsnullopt, and the fallbackisCanonicalNumericIndexStringbranch treats every such key as out of bounds.The
unsigned-index overloads (getOwnPropertySlotByIndexetc.) already compare against the fullsize_tlength viainBounds(), which is why4294967295 in u(routed throughgetUInt32→hasProperty(unsigned)) returnstrue.Fix
Add
parseTypedArrayIndex(PropertyName)alongsideparseIndex(PropertyName)that covers the full uint32 range.UINT32_MAXis the only value that fits in a uint32 and is rejected byparseIndex, so it is the only extra case to cover; the helper falls through to the existing parser first. The fourJSGenericTypedArrayViewPropertyName overloads now route through it.TypedArray structures are shared per type, not per length, so once
getOwnPropertySlot("4294967295")becomes length-dependent the structure-keyed caches that memoised "always absent" must be told not to:HasOwnPropertyCache::tryAddwidens its index guard fromparseIndextoparseTypedArrayIndexso"4294967295"is never cached (theASSERT(*result == hasOwnProperty(...))inObjectPrototype.cppfires otherwise on a short-then-long probe).isTypedArrayType && isCanonicalNumericIndexStringshort-circuits ingenerateConditions,prepareChainForCachingandPolyProtoAccessChain::tryCreatereturn invalid /nullopt/nullptrinstead of a valid empty condition set whenparseTypedArrayIndexaccepts the key, so noMiss/InMissIC is installed for it on a TypedArray.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.Regression test is in the paired oven-sh/bun PR and covers the direct accesses, the
hasOwn(small) → hasOwn(big)ordering, and a warmed-ICprobe(small) → probe(big)ordering.