Repository navigation
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 (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 2 remain after this review. WalkthroughThe change updates JavaScriptCore Set-like size conversion to clamp values at or above ChangesSet-like size handling
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The size clamping change and its boundary coverage do not leave a concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed, relevant explanation of the bug, fix, scope, and validation. However, it does not include the required Bugzilla URL, review status, or the template-formatted changed-file and function list. Resolution Add the associated Bugzilla link, include the required review line such as "Reviewed by NOBODY (OOPS!).", and list each changed path with the affected function or class in the repository template format. Ensure the pull request is linked from Bugzilla and has the required labels and assignment.
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 and didn't find any bugs — the clamp is correct and the test is well-formed. Since it changes JSC runtime semantics and both paths are routed to jsc-reviewers in CODEOWNERS, a human sign-off is still worthwhile.
What was reviewed:
getSetSizeAsInt:>=compares the double against4294967295.0(exactly representable), so +Infinity and every finite value ≥ 2^32-1 clamp; the remainingstatic_cast<uint32_t>only sees integers in [0, 2^32-2], so the prior UB is gone.- Callers only use the result in
thisSet->size() <= size/< sizecomparisons, and a realJSSetcannot reach UINT32_MAX elements, so clamping preserves every branch outcome. - New stress test throws on failure, prints nothing on success, has no tier-up loop (pure slow-path C++), and is small enough to stay well under the 200ms budget.
Extended reasoning...
Overview
The PR fixes a spec-compliance bug in the ES2025 Set methods (intersection, difference, union, symmetricDifference, isSubsetOf, isSupersetOf, isDisjointFrom) when the set-like argument reports size >= 2^32. The one-line functional change in Source/JavaScriptCore/runtime/SetPrototype.cpp replaces std::isinf(intOrInfSize) with intOrInfSize >= std::numeric_limits<uint32_t>::max() so that any size at or beyond UINT32_MAX (including +Infinity) is clamped rather than passed to static_cast<uint32_t>, which was undefined behavior for out-of-range doubles and wrapped in practice. A three-line comment documents why clamping is sound. A new 120-line stress test in JSTests/stress/ exercises all seven methods across 13 large sizes plus small-size and error paths, asserting both results and the observable size/has/keys/next call sequence.
Security risks
None identified. This is a pure spec-conformance fix on a slow path that only runs for user-supplied set-like objects (real JSSet arguments take the fast path). No memory management, no allocation sizing, no pointer arithmetic — the returned uint32_t is used solely in <= / < comparisons against JSSet::size() to pick which observable algorithm branch to take. Removing the out-of-range static_cast<uint32_t>(double) actually eliminates a small piece of UB.
Level of scrutiny
Low-to-moderate on the code itself: the diff is a single guard change whose correctness is easy to verify by inspection (the uint32_t limit promotes to an exactly-representable double for the comparison; toIntegerOrInfinity guarantees integral values so the residual cast is well-defined; NaN and negatives are handled earlier). However, .github/CODEOWNERS routes both /Source/JavaScriptCore and /JSTests to WebKit/jsc-reviewers, and per the review guidelines CODEOWNER-covered paths should get a human look even when the change is small.
Other factors
The new test conforms to JSTests/README.md rules: assertions throw, nothing is printed on success, there is no hot loop needing testLoopCount (the fix is in a C++ slow-path helper, not JIT-tiered code), and the total work is a few hundred trivial operations — comfortably under 200ms. The PR description states it fails on an unpatched build and that existing set-* stress tests and test262 built-ins/Set pass; I did not independently run those but the claim is consistent with the diff. No other reviewers have weighed in yet.
Preview Builds
|
…runcating it
GetSetRecord keeps the set-like's size as an unbounded integer or +Infinity,
and intersection, difference, isSubsetOf, isSupersetOf and isDisjointFrom
compare it with the receiver's size to decide whether to call other.has()
or to iterate other.keys(). getSetSizeAsInt() special-cased Infinity but
passed every other double through static_cast<uint32_t>, which is undefined
for values of 2^32 and more and wraps in practice. A set-like reporting
size 2^32 compared as size 0, so these methods took the wrong branch:
new Set([1]).isSubsetOf({ size: 2 ** 32, has: () => true, keys }) returned
false without calling has(), and isSupersetOf iterated keys() where the
spec returns false after reading size.
Clamp instead. JSSet::size() can never reach UINT32_MAX (the backing
storage is capped at IndexingHeader::maximumLength slots), so a clamped
size compares the same way the exact one would. This also covers Infinity,
so the isinf() branch goes away.
* JSTests/stress/set-methods-set-like-size-beyond-uint32.js: Added.
* Source/JavaScriptCore/runtime/SetPrototype.cpp:
(JSC::getSetSizeAsInt):
a54ec5a to
50c11bf
Compare
…r more (WebKit bump for oven-sh/WebKit#596) GetSetRecord keeps a set-like's size as an unbounded integer. JSC truncated it to uint32 before comparing it with the receiver's size, so intersection, difference, isSubsetOf, isSupersetOf and isDisjointFrom consulted the wrong side (other.keys() instead of other.has(), or the reverse) once the reported size reached 2^32. Pin WebKit to the preview build of oven-sh/WebKit#596, which clamps the size at UINT32_MAX instead, and add coverage for all seven methods across the size boundary.
1 participant
Problem
sizeis 2^32 or more.new Set([1]).isSubsetOf({ size: 2 ** 32, has: v => v === 1, keys })returnsfalsewithout callinghas().isDisjointFromreturnstrue,intersectionreturns an empty Set,differencekeeps every element, andisSupersetOfiterateskeys()where the spec returnsfalseafter readingsize. Sizes2^32 - 1andInfinitybehave correctly. V8 and SpiderMonkey follow the spec for every size.getSetSizeAsInt()inSource/JavaScriptCore/runtime/SetPrototype.cpp:215. It special-casesInfinityand passes every other double throughstatic_cast<uint32_t>. That cast is undefined for values of 2^32 and more and wraps in practice, so2^32compares as0and2^53 + 2as2. The callers then take the wrong branch of "thisSize <= otherSize: callother.has(), else iterateother.keys()".Fix
UINT32_MAXinstead of casting:if (intOrInfSize >= UINT32_MAX) return UINT32_MAX;. This coversInfinitytoo, so theisinf()branch goes away.JSSet::size(), auint32_tthat can never reachUINT32_MAX: the backing storage is capped atIndexingHeader::maximumLength(2^28) slots. A clamped size compares the same way the spec's unbounded integer would.JSTests/stress/set-methods-set-like-size-beyond-uint32.js(new) checks the result and the observablesize/has/keys/nextcall log of all seven methods for sizes from 2^31 through 2^64, 1e300,Number.MAX_VALUEandInfinity, plus the small-size branches and theRangeError/TypeErrorpaths. It fails on the unpatched engine atisSubsetOf size=4294967296and passes with this change. The 162 existingJSTests/stress/set-*tests and the 764 test262built-ins/Setcases (strict and sloppy) pass under the builtjsc.Background
obj.size, appliesToNumberthenToIntegerOrInfinity, throws on NaN or a negative value, and stores the result as[[Size]]with no upper bound.intersection,differenceandisDisjointFrombranch onSetDataSize(O) <= otherRec.[[Size]],isSubsetOfreturnsfalsewhenSetDataSize(O) > [[Size]], andisSupersetOfreturnsfalsewhenSetDataSize(O) < [[Size]].unionandsymmetricDifferencereadsizebut do not use it.Setargument takes thefastSet*paths and never reachesgetSetSizeAsInt(). The function only runs for user-supplied set-likes, where a virtual or predicate-backed set ({ size: Number.MAX_SAFE_INTEGER, has }) is the usual reason to report a huge size.mainhas the same code.