Repository navigation
[JSC] Resizable ArrayBuffer: BigInt filter() over a shrunk buffer throws, and the constructor compares ToIndex(length) with maxByteLength - #605
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesThe PR updates Buffer semantics
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The buffer constructor ordering and BigInt typed-array filter changes are covered by targeted stress tests, with no unresolved merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed problem statement, fix explanation, and verification results. However, it does not follow the required template because it omits the Bugzilla title and link, the reviewer placeholder, and the required changed-file and function list. Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes observable spec-ordering behavior in JavaScriptCore runtime code that falls under the jsc-reviewers CODEOWNERS entry, a human look would still be worthwhile.
What was reviewed
constructImpl: verified the newToIndex(length)→maxByteLength→ compare →JSC_GET_DERIVED_STRUCTUREorder matches AllocateArrayBuffer step 3.a happening before OrdinaryCreateFromConstructor, and thattoIndexstill triggers uservalueOfexactly once (the old splittoNumber/toIndexpath is gone).genericTypedArrayViewProtoFuncFilter: checked thatfirstUndefinedInKeptrecords the index withinkept(not the source), sofrom.first(...)is always in-bounds; the prefix write +throwVMTypeErrormatches the spec's TypedArraySetElement loop, andspeciesConstructalready guarantees a BigInt-content-type result so the truncated store path is type-safe.- Confirmed
Adaptor::isBigIntexists on all adaptors; theif constexprguards keep the non-BigInt instantiations unchanged. - New stress tests follow JSTests/README.md rules (throw on failure, no logging).
Extended reasoning...
Overview
This PR fixes two ECMA-262 conformance bugs around resizable ArrayBuffers. In Source/JavaScriptCore/runtime/JSArrayBufferConstructor.cpp, the ArrayBuffer/SharedArrayBuffer constructor is reordered so ToIndex(length) runs before the maxByteLength option is read and before newTarget.prototype is accessed via JSC_GET_DERIVED_STRUCTURE, and the byteLength > maxByteLength check now compares the two size_t results of toIndex rather than a raw toNumber double. In Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h, %TypedArray%.prototype.filter on BigInt arrays now records the position of the first kept undefined element (produced when the callback shrinks/detaches the buffer), writes only the prefix into the species-constructed result, and then throws the ToBigInt(undefined) TypeError. Two new stress tests in JSTests/stress/ cover both fixes extensively, including side-effect ordering, species observability, and the non-BigInt (no-throw) baseline.
Security risks
None identified. The constructor change only reorders existing conversions and comparisons; it neither introduces new allocation logic nor bypasses any bounds check. The filter change stores strictly fewer elements than before in the error case (from.first(n) with n <= kept.size()), so there is no out-of-bounds write risk. firstUndefinedInKept is captured as kept.size() at append time, guaranteeing it is a valid prefix length. speciesConstruct already enforces content-type compatibility, so the BigInt-only path cannot reach a Number-typed destination. The std::optional<size_t> and [[unlikely]] annotations are compile-time/branch-hint only.
Level of scrutiny
Medium-high. While the diff is small and the logic checks out against the spec text cited in the PR, this is core JS engine runtime behavior with observable side-effect ordering, and both /Source/JavaScriptCore and /JSTests are listed in .github/CODEOWNERS under @ WebKit/jsc-reviewers. Per the approval guidelines, CODEOWNERS-covered runtime changes with behavior implications should get a human sign-off rather than an automated approve. The constructor reorder in particular changes when user getters (options.maxByteLength, newTarget.prototype) fire relative to the length RangeError, which is exactly the sort of subtlety a JSC reviewer will want to eyeball against test262 and the referenced 279181@ main history.
Other factors
The PR includes thorough regression tests that assert the exact ordering (via log arrays) and the partial-write-before-throw semantics (via a captured species result), and the description reports passing test262 and the existing typedarray/arraybuffer stress suite on a Debug build. The tests conform to JSTests/README.md requirements (throw-on-failure assertions, no printing, no long loops). No prior review activity exists on the timeline, so this is the first review posted.
|
Preview build of ed8b9ca: |
…er over a shrunk buffer throws, ArrayBuffer constructor compares ToIndex(length) with maxByteLength
61273cc to
c9b0a6b
Compare
… WebKit cf1b36ec8703
…ows, and the constructor compares ToIndex(length) with maxByteLength
Two conformance fixes for resizable ArrayBuffer / growable
SharedArrayBuffer. V8 and SpiderMonkey already behave this way.
1. %TypedArray%.prototype.filter on a BigInt64Array or BigUint64Array
whose callback shrinks or detaches the buffer.
filter reads each element with TypedArrayGetElement before it calls the
callback, so once the view is out of bounds the remaining elements are
undefined. The kept values are then stored into the new array with
TypedArraySetElement, which is ToBigInt(value) for a BigInt array, and
ToBigInt(undefined) is a TypeError. genericTypedArrayViewProtoFuncFilter
keeps native values, and for an undefined element it kept
BigInt64Adaptor::toNativeFromUndefined(), a stub that returns 0, so the
result had 0n where the other engines throw:
const b = new ArrayBuffer(40, { maxByteLength: 40 });
const v = new BigInt64Array(b).fill(7n);
v.filter((x, i) => { if (i === 2) b.resize(0); return true; })
// was 7,7,7,0,0, now TypeError (map() already threw)
Remember the position of the first undefined kept element. After
TypedArraySpeciesCreate, store the elements before it and then throw the
ToBigInt TypeError, which is the order the spec's store loop produces
and what a species constructor can observe. Number typed arrays are
unchanged: ToNumber(undefined) is NaN, which is what
toNativeFromUndefined() gives for them.
2. new ArrayBuffer(length, { maxByteLength }) and new
SharedArrayBuffer(length, { maxByteLength }) with a fractional length.
The constructor is ToIndex(length), then GetArrayBufferMaxByteLengthOption,
then AllocateArrayBuffer throws a RangeError if byteLength > maxByteLength.
constructImpl compared maxByteLength with ToNumber(length) before
truncation, so new ArrayBuffer(1.5, { maxByteLength: 1 }) threw
"ArrayBuffer length exceeds maxByteLength option" although ToIndex(1.5)
is 1. The split into an early toNumber() and a late toTypedArrayIndex()
dates from when the index conversion also threw for lengths above
MAX_ARRAY_BUFFER_SIZE, which has to happen after newTarget.prototype is
read. toIndex() now only range-checks against 2^53 - 1, which the spec
does in step 2, so do ToIndex(length) first and compare the two
integers. As a side effect a negative or too large length now throws
before options.maxByteLength and newTarget.prototype are read, as
specified.
* JSTests/stress/typedarray-filter-bigint-resizable-buffer-out-of-bounds.js: Added.
* JSTests/stress/arraybuffer-constructor-length-toindex-before-maxbytelength.js: Added.
* Source/JavaScriptCore/runtime/JSArrayBufferConstructor.cpp:
(JSC::JSGenericArrayBufferConstructor<sharingMode>::constructImpl):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:
(JSC::genericTypedArrayViewProtoFuncFilter):
c9b0a6b to
ed8b9ca
Compare
… WebKit 000c48997255
2 participants
Problem
Two places where JSC disagrees with the spec, V8 and SpiderMonkey on resizable
ArrayBuffer/ growableSharedArrayBuffer. Upstream WebKitmainhas the same code.1.
BigInt64Array/BigUint64Array.prototype.filter()when the callback shrinks or detaches the buffer%TypedArray%.prototype.filterreads each element with TypedArrayGetElement before it calls the callback, so once the view is out of bounds the remaining elements areundefined. It then stores the kept values into the new array with TypedArraySetElement, which isToBigInt(value)for a BigInt array, andToBigInt(undefined)throws.genericTypedArrayViewProtoFuncFilter(JSGenericTypedArrayViewPrototypeFunctions.h) keeps native values instead, and for anundefinedelement it keptBigInt64Adaptor::toNativeFromUndefined(), a stub that returns 0 ("since undefined->BigInt conversion throws an error"). So the result got0nin those slots.map()with the same callback already throws in JSC, and Number typed arrays are right as they are (ToNumber(undefined)isNaN, which is whattoNativeFromUndefined()gives them:NaNfor floats,0for integers).2.
new ArrayBuffer(length, { maxByteLength })/new SharedArrayBuffer(length, { maxByteLength })with a fractionallengthThe constructor is
ToIndex(length), thenGetArrayBufferMaxByteLengthOption(options), then AllocateArrayBuffer throws a RangeError ifbyteLength > maxByteLength.constructImpl(JSArrayBufferConstructor.cpp) comparedmaxByteLengthwithToNumber(length)before truncation. The split into an earlytoNumber()and a latetoTypedArrayIndex()afterJSC_GET_DERIVED_STRUCTUREcomes from 279181@main, when the index conversion also threw for lengths aboveMAX_ARRAY_BUFFER_SIZEand that had to stay afternewTarget.prototypeis read (data-allocation-after-object-creation.js). SincetoTypedArrayIndexbecametoIndexthe conversion only range-checks against 2^53 - 1, which is step 2 of the constructor, so nothing needs the split any more.Fix
filter: remember the position of the firstundefinedkept element (BigInt arrays only,if constexpr). After TypedArraySpeciesCreate, store the elements before it, then throw theToBigIntTypeError. That is the order of the spec's store loop, and it is what a species constructor that keeps the new array can observe (V8 does the same: the prefix is written, the rest is untouched).ToIndex(length)first, then readmaxByteLength, then compare the two integers, then get the structure. A side effect is that a negative or too largelengthnow throws beforeoptions.maxByteLengthandnewTarget.prototypeare read, which is the specified order. The allocation-failure RangeError still comes after the prototype read.Verification
JSTests/stress/typedarray-filter-bigint-resizable-buffer-out-of-bounds.js: both BigInt types with shrink to zero, partial shrink, a fixed-length view going out of bounds,transfer()on resizable and fixed buffers, shrink then grow back, the non-JSFunctioncallback path, a callback that drops theundefinedelements (no throw), the callback / species-create / store order with a subclass and with a foreign species result, and the Number types staying atNaN/0. Passes on node 26 apart from the error message text. Fails on the currentjscat the first case.JSTests/stress/arraybuffer-constructor-length-toindex-before-maxbytelength.js: fractional, string, boolean and object lengths againstmaxByteLengthfor both constructors, the cases that must still throw, and the order ofToIndex(length), themaxByteLengthgetter, thenewTarget.prototypegetter and the allocation failure. Passes on node 26 except that V8 readsmaxByteLengthbefore rejecting a length of 2^53. Fails on the currentjscat the first case.built-ins/ArrayBuffer,built-ins/SharedArrayBuffer,built-ins/TypedArray/prototype/{filter,map},built-ins/TypedArrayConstructors/ctors{,-bigint}: same results before and after (the only failures are the unimplemented immutable-ArrayBuffer tests).JSTests/stressfiles matching typedarray / arraybuffer / dataview / resizable / growable, plus the new files under no-LLInt, no-JIT, eager-JIT andcollectContinuouslyoptions, on a Debug build.