Repository navigation
Conversation
…s: an out-of-range BigInt needle matches nothing The three prototype functions convert the needle to the element type once and then compare raw elements. For the BigInt adaptors, toNativeFromValueWithoutCoercion() converted with JSBigInt::toBigInt64 / toBigUInt64, which are ToBigInt64 / ToBigUint64: the value modulo 2^64. So new BigInt64Array([3n, -1n]).indexOf(2n ** 64n - 1n) returned 1 and new BigUint64Array([0n]).includes(-(2n ** 64n)) returned true. The spec compares with IsStrictlyEqual / SameValueZero against each element's BigInt value, so a needle outside the element type's range matches no element. V8 and SpiderMonkey return -1 / false. Add JSBigInt::tryGetAsInt64 / tryGetAsUint64, which return nullopt instead of wrapping when the BigInt is not representable, and use them in toNativeFromValueWithoutCoercion(). The Number adaptors already reject a needle the element type cannot hold.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that toNativeFromValueWithoutCoercion has no other callers than the three %TypedArray%.prototype search functions (so the semantic change is scoped to indexOf/lastIndexOf/includes), that tryGetAsInt64 handles the INT64_MIN boundary correctly (magnitude 2^63 with sign → wrapped value is negative, sign matches), and that the includes nullopt path with a shrunk resizable buffer still returns false for an out-of-range BigInt since it is not undefined.
Extended reasoning...
The fix itself looks correct and narrowly scoped. I traced toNativeFromValueWithoutCoercion to its only call site (JSGenericTypedArrayView::toAdaptorNativeFromValueWithoutCoercion), which in turn is used only by genericTypedArrayViewProtoFuncIncludes, genericTypedArrayViewProtoFuncIndexOf, and genericTypedArrayViewProtoFuncLastIndexOf in JSGenericTypedArrayViewPrototypeFunctions.h, so switching from wrapping to exact-representation semantics cannot leak into element writes or other conversions. I walked the sign/magnitude cases of tryGetAsInt64 against toBigUInt64Heap's two's-complement output and confirmed the (value < 0) != sign() test admits exactly [-2^63, 2^63 - 1], including the -2^63 corner where the wrapped value is 0x8000… and thus negative. The sizeof(uint64_t) / sizeof(Digit) bound is correct for both 32- and 64-bit Digit given JSBigInt's no-leading-zero-digit invariant. The includes early-return at JSGenericTypedArrayViewPrototypeFunctions.h:526 was checked because it now fires for out-of-range BigInts too; it keys off valueToFind.isUndefined(), which a BigInt never is, so behavior is correct. Since the changed paths are CODEOWNERS-covered (Source/JavaScriptCore and JSTests → WebKit/jsc-reviewers) and there is an unposted verified finding alongside the inline nit, deferring rather than approving.
|
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 (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe change adds checked BigInt-to-64-bit conversions, uses them for typed-array matching, and adds stress coverage for representable, out-of-range, wrapped, boundary, and mismatched typed-array search values. ChangesBigInt typed-array matching
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change rejects out-of-range BigInt typed-array search needles rather than matching modulo-wrapped values, with coverage for boundaries, views, repeated elements, and search indices. No merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
Preview Builds
|
There was a problem hiding this comment.
LGTM — the testLoopCount nit from the previous review is addressed, and the added repeated-element cases are a nice extra.
What was reviewed:
- Verified
tryGetAsInt64/tryGetAsUint64range logic against the heap-BigInt digit representation on both 32- and 64-bitDigitsizes, including the-2^63and zero edge cases. - Confirmed
toNativeFromValueWithoutCoercionreaches only the three typed-array search prototypes (includes/indexOf/lastIndexOfin JSGenericTypedArrayViewPrototypeFunctions.h), so no store or coercion path picks up the non-wrapping behavior. - Checked the new stress test against JSTests/README.md conventions: throws on failure, no logging, now driven by
testLoopCount.
Extended reasoning...
Overview
This PR fixes a spec-compliance bug where BigInt64Array/BigUint64Array indexOf/lastIndexOf/includes would wrap an out-of-range BigInt needle modulo 2^64 and spuriously match a congruent element. It adds two non-wrapping helpers (JSBigInt::tryGetAsUint64, JSBigInt::tryGetAsInt64) in JSBigIntInlines.h that return std::nullopt when the value does not fit, and switches toNativeFromValueWithoutCoercion in ToNativeFromValue.h to call them for the BigInt adaptors. A new stress test covers wrapped needles, boundaries, multiple view kinds (plain, subarray, resizable length-tracking and fixed-window, SharedArrayBuffer), repeated elements, and cross-type Number/BigInt mismatches. Since the previous review, commit 85d30e9 replaced the hard-coded 200-iteration loop with Math.max(testLoopCount, rows.length) and added repeated-element first/last-match cases.
Security risks
None identified. The change narrows behavior (returns nullopt in more cases) rather than widening it, and the affected function is used only to convert a search needle for equality comparison — not for indexing, bounds checks, or memory writes. No allocator, pointer arithmetic, or GC interaction is touched. The static_cast<int64_t> of a uint64_t is implementation-defined wrapping that JSC already relies on in the adjacent toBigInt64.
Level of scrutiny
Moderate. This is JSC runtime code, so I walked the range logic by hand: for tryGetAsUint64, negative sign or length() > 64/sizeof(Digit) (1 on 64-bit, 2 on 32-bit) correctly rejects anything ≥ 2^64 or < 0. For tryGetAsInt64, once the magnitude is < 2^64, comparing the sign of the wrapped int64_t against heapBigInt->sign() accepts exactly [-2^63, 2^63 - 1]; the -2^63 case (magnitude 2^63 → wrapped uint64 2^63 → int64 INT64_MIN, negative, matches sign()==true) and the zero case (length()==0, wrapped 0, sign()==false) both pass. I grepped all callers of toNativeFromValueWithoutCoercion / toAdaptorNativeFromValueWithoutCoercion and confirmed only the three search prototypes reach it, so set, fill, DataView, and Atomics paths keep their spec-required wrapping semantics.
Other factors
The new helpers follow the exact structure of the existing toBigUInt64/toBigInt64 immediately above them, and the DFG's DataViewSet int64 path uses the same sign-mismatch test the PR notes. The stress test now conforms to JSTests/README.md (throws on failure, no output, testLoopCount-driven), addressing the only point from the prior review. The change is small, self-contained, spec-aligned, and well-tested; approving.
|
Superseded by #609. That change replaces the modular |
2 participants
Problem
BigInt64Array/BigUint64Array.indexOf,.lastIndexOfand.includesmatch a BigInt needle that no element equals when it is congruent to an element modulo 2^64.new BigInt64Array([3n, -1n]).indexOf(2n ** 64n - 1n)is1andnew BigUint64Array([0n]).includes(-(2n ** 64n))istrue. V8, SpiderMonkey and JSC's ownArray.prototype.indexOf.call(array, needle)give-1/false.toNativeFromValueWithoutCoercion()(Source/JavaScriptCore/runtime/ToNativeFromValue.h:61). The three functions convert the needle to the element type once, then compare raw elements. The BigInt adaptors converted withJSBigInt::toBigInt64/toBigUInt64, which are ToBigInt64 / ToBigUint64: the value modulo 2^64.Fix
JSBigInt::tryGetAsInt64(JSValue)/tryGetAsUint64(JSValue)next to the wrapping conversions. They returnnulloptwhen the BigInt is not representable.toNativeFromValueWithoutCoercion()calls them, so such a needle gives-1/false, like a needle that is not a BigInt.JSTests/stress/bigint-typed-array-index-of-out-of-range.js(new) fails on the unpatchedjsc(needle 18446744073709551615n indexOf: expected -1 but got 1) and passes with the change, also with--useJIT=0and eager tier-up. Related stress tests and test262 directories pass (see Notes).Background
JSBigIntis a sign bit plus little-endianDigits with no leading zero digit. More digits than fit in 64 bits means a magnitude of at least 2^64. WithUSE(BIGINT32)a small BigInt is an immediateint32_tinstead.m < 2^64the wrapped value ism(positive) or2^64 - m(negative). Asint64_tit has the BigInt's sign exactly when the BigInt is in [-2^63, 2^63 - 1]. The DFG'sDataViewSetpath forsetBigInt64makes the same test.mainhas the same code.Notes
indexOf/includesonly forJSArrayshapes (ArrayIndexOfIntrinsic), so these C++ prototype functions are the only path for typed arrays. That matches the report: the result is the same with every JIT tier off.SharedArrayBufferview, with and withoutfromIndex, for needles that wrap to an element (2^64 - 1,2^64 + 3,±2^64,2^128,2^63,-2^63 - 1,±(2^200 + 5),-5) and for the representable boundaries (0n,-1n,±2^32,2^63 - 1,-2^63,2^63,2^64 - 1), and checks thatArray.prototype.indexOf/includesagree.jsc:typedarray-indexOf,typedarray-lastIndexOf,typedarray-includes,typedarray-resize-includes,non-suitable-typed-array-index-of,typed-array-lastIndexOf-exception-check,bigint-typed-array-*,bigint-*(with theirrequireOptions),array-indexof-*,array-includes-*.built-ins/TypedArray/prototype/{indexOf,lastIndexOf,includes}andbuilt-ins/Array/prototype/{indexOf,lastIndexOf,includes}: 559 pass, 0 fail.built-ins/BigInt,built-ins/TypedArrayConstructors/Big{Int,Uint}64Array,built-ins/DataView/prototype/{get,set}BigInt64,built-ins/Atomics: the same 458 pass / 12 fail as the unpatched build (immutableArrayBufferandAtomics.waitagent tests).JSBigInt::compare(value, int64). That helper'sInt64BigIntImplreportslength() == 2for any non-zero value on 32-bit targets, so it would compare5nunequal to5there. The dedicated helpers avoid it and are cheaper.