IntlCollator: throw an OutOfMemoryError instead of crashing when a huge Latin-1 string needs the UTF-16 upconversion - #500
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesIntl collator OOM protection
Merge Risk: ⚪ Minimal · up to This localized change converts a crash into an OutOfMemoryError for oversized Latin-1 collation inputs and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Full details: Description checkExplanation The description explains the problem, root cause, fix, test coverage, fast-path behavior, and related Bun change. It does not provide the required WebKit Bugzilla URL or the template's Reviewed by line and changed-file/function list. Resolution Add the WebKit Bugzilla issue URL, include the required "Reviewed by NOBODY (OOPS!)." line or an actual reviewer, and list the changed paths and relevant functions in the required template format. Ensure the pull request is linked from Bugzilla and has the required labels and assignment if applicable.
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.
LGTM — small, targeted crash fix that turns a Vector capacity CRASH() into a catchable OutOfMemoryError.
What was reviewed:
- Verified
isValidCapacityForVector<char16_t>threshold is(UINT_MAX >> 1) / 2, so1 << 30correctly triggers the guard. - Confirmed
UpconvertedCharactersWithSizeusesVector<char16_t>::grow()which would CRASH() at that size — the guard is placed correctly. - Checked both callers (
IntlCollatorPrototype.cpp:107,StringPrototype.cpp:1797) useRELEASE_AND_RETURN, so the thrown exception propagates and thereturn { }value is never observed. - Matches the existing
rdar://160634825precedent inStringPrototype.cppand the existingU_FAILUREerror path in this same function.
Extended reasoning...
Overview
This PR adds a pre-flight capacity check in IntlCollator::compareStrings() before the ucol_strcoll() fallback path calls StringView::upconvertedCharacters(). When an 8-bit operand is ≥ 1 << 30 characters, the backing Vector<char16_t> cannot hold it (isValidCapacityForVector<char16_t> caps at 0x3FFFFFFF), and allocateBuffer<FailureAction::Crash> would hit CRASH(). The fix throws OutOfMemoryError instead. A stress test covers localeCompare and Intl.Collator().compare in both argument positions.
Security risks
None. This is a defensive bounds check that converts a process crash into a JS-level RangeError. No new data flows, no parsing of untrusted input beyond what already existed, and the check is purely a length comparison against a compile-time constant.
Level of scrutiny
Low-to-moderate. The change is ~10 lines in a single function, follows an established in-tree precedent (the rdar://160634825 check in StringPrototype.cpp:2188-2192), and reuses the exact error-handling pattern already present two lines below (throwException + return { }). The fast paths (ASCII UCA DUCET, ucol_strcollUTF8) are untouched, so there is no behavior change for normal-sized inputs.
Other factors
- Verified the math:
isValidCapacityForVector<char16_t>(n)=n <= (0x7FFFFFFF / 2)=n <= 0x3FFFFFFF, so the test's1 << 30(=0x40000000) is exactly the first failing size. - Verified
UpconvertedCharactersWithSizector callsm_upconvertedCharacters.grow(string.m_length)on the 8-bit path — the guard is correctly conditioned onis8Bit(). - Verified both call sites wrap the result in
RELEASE_AND_RETURN(scope, ...), so the pending exception propagates and the dummyUCOL_EQUALreturn value is never observed by JS — same as the pre-existingU_FAILUREbranch. - Test is appropriately gated with
//@ skip if $memoryLimitedsince it allocates a 1 GiB string.
Preview Builds
|
e173d7f to
59d0abd
Compare
There was a problem hiding this comment.
LGTM — small, defensive guard that turns a CRASH() into a catchable RangeError: Out of memory.
What was reviewed:
- Verified
UpconvertedCharactersWithSizebacks ontoVector<char16_t>::grow(), andisValidCapacityForVector<char16_t>caps at0x3FFFFFFF, so1 << 30is exactly the first failing length. - Confirmed both callers (
IntlCollatorPrototype.cpp:107,StringPrototype.cpp:1797) wrap the call inRELEASE_AND_RETURN(scope, …), so the thrown exception propagates and the{}return value is never observed. - Matches the existing
rdar://160634825precedent instringProtoFuncNormalize(StringPrototype.cpp:2188-2192). - Fast paths (ASCII DUCET, both-8-bit-ASCII
ucol_strcollUTF8) are untouched — the guard only runs on the fallback branch.
Extended reasoning...
Overview
This PR adds a capacity pre-check to IntlCollator::compareStrings() before it calls StringView::upconvertedCharacters() on the ucol_strcoll() fallback path. When an 8-bit operand is ≥ 1 << 30 characters, the backing Vector<char16_t> cannot grow to that size and allocateBuffer<FailureAction::Crash> calls CRASH(). The fix throws OutOfMemoryError instead. A stress test covers localeCompare in both argument orders and both the default and options-configured Intl.Collator.
Security risks
None introduced. The change strictly removes a process-crash denial-of-service vector reachable from untrusted JS ("a".repeat(1<<30).localeCompare("\u3042")). No new allocation, no new user-controlled data flow — just an early bounds check using the same predicate (WTF::isValidCapacityForVector<char16_t>) that Vector::allocateBuffer itself uses at Vector.h:226.
Level of scrutiny
Low-to-moderate. The diff is 9 source lines confined to one function's slow-path fallback. It follows an established pattern already in the codebase — stringProtoFuncNormalize in StringPrototype.cpp:2188-2192 does the same length-guard-then-throwOutOfMemoryError for the same underlying reason. The canUpconvert lambda correctly short-circuits for 16-bit views (which need no upconversion), and the [[unlikely]] annotation is appropriate. Both call sites use RELEASE_AND_RETURN, so the pending exception is handled correctly and the return { } sentinel is never consumed.
Other factors
- The preview build (
autobuild-preview-pr-500-e173d7f6) succeeded, so the explicitWTF::qualification and header availability are confirmed by CI. - The regression test is properly gated with
//@ skip if $memoryLimitedand forces the fallback path by pairing the huge Latin-1 string with a 16-bit operand, avoiding the ASCII fast paths. - No outstanding reviewer comments; CodeRabbit was rate-limited and did not review.
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
59d0abd to
dddf09e
Compare
There was a problem hiding this comment.
LGTM — small, targeted fix that converts a hard CRASH() into a JS RangeError: Out of memory.
What was reviewed:
- The
canUpconvertguard uses the sameisValidCapacityForVector<char16_t>predicate thatVectorBufferBase::allocateBufferchecks before crashing, and only applies to 8-bit views (16-bit views are passed through without copying inUpconvertedCharactersWithSize). - Error-path shape matches the existing
U_FAILUREbranch in the same function (throw...; return { };) and both callers wrap the result inRELEASE_AND_RETURN(scope, ...), so the pending exception propagates correctly. - Mirrors the existing
rdar://160634825precedent innormalize()(StringPrototype.cpp:2188-2192). - Fast paths (ASCII UCA DUCET, both-8-bit
ucol_strcollUTF8) are untouched — the check only runs on the fallback that would allocate.
Extended reasoning...
Overview
This PR modifies one function, IntlCollator::compareStrings() in Source/JavaScriptCore/runtime/IntlCollator.cpp, adding a 7-line capacity check before the ucol_strcoll() fallback path. It also adds a stress test at JSTests/stress/intl-collator-compare-huge-latin1-string.js.
The bug: when neither ASCII fast path applies, the function calls x.upconvertedCharacters() / y.upconvertedCharacters(). For an 8-bit StringView, UpconvertedCharactersWithSize grows a Vector<char16_t, 32> to view.length(). VectorBufferBase::allocateBuffer<FailureAction::Crash> calls CRASH() when !isValidCapacityForVector<char16_t>(newCapacity) (i.e., length > 0x3FFFFFFF). Since JSString::MaxLength is INT32_MAX, a valid Latin-1 JS string of 1 << 30 characters or more triggers this. The fix pre-checks the same predicate and throws OutOfMemoryError instead.
Security risks
None introduced. This is strictly defensive: it replaces a process crash with a catchable JS exception. No new allocation paths, no user-controlled data flowing anywhere new. The check is only on view.length(), an unsigned integer already bounded by JSString::MaxLength.
Level of scrutiny
Low-to-moderate. The change is 7 lines inside a single function, only reachable when the existing fast paths bail out, and only fires for strings ≥ 1 GiB in the 8-bit representation — a case that previously crashed unconditionally. The !view.is8Bit() short-circuit correctly skips 16-bit views (which UpconvertedCharactersWithSize handles without allocating, per StringView.h:683-685). The error-return shape (throwOutOfMemoryError(...); return { };) is identical to the pre-existing U_FAILURE branch three lines below, and both call sites (IntlCollatorPrototype.cpp:107, StringPrototype.cpp:1797) use RELEASE_AND_RETURN so the exception propagates. There is direct precedent for this exact pattern at StringPrototype.cpp:2188-2192 (rdar://160634825).
Other factors
- Regression test covers both argument positions of
localeCompare, the defaultIntl.Collator, and a collator with{ sensitivity: "base" }(which disables the UCA DUCET fast path). It is//@ skip if $memoryLimited, consistent with other 1 GiB stress tests. - The bug hunting system found no issues.
- No prior human review comments to address; only bot comments (coderabbit rate-limit notice, preview build links) on the timeline.
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
dddf09e to
b0d21d8
Compare
There was a problem hiding this comment.
LGTM — targeted crash fix that turns a Vector capacity CRASH() into a catchable OutOfMemoryError.
What was reviewed:
- Verified
isValidCapacityForVector<char16_t>is the exact predicateVector::allocateBufferchecks before CRASH()ing, so the guard matches the failure condition precisely. - Confirmed both callers (
IntlCollatorPrototype.cpp:107,StringPrototype.cpp:1797) useRELEASE_AND_RETURN, so the thrown exception propagates — same as the existingU_FAILUREthrow path just below. - The
!view.is8Bit()early-out is correct: 16-bit views pass throughupconvertedCharacters()without allocation. - Fast paths (ASCII DUCET, both-8-bit-ASCII
ucol_strcollUTF8) are untouched; the check only runs on the fallback path.
Extended reasoning...
Overview
The PR adds a 9-line guard in IntlCollator::compareStrings() (Source/JavaScriptCore/runtime/IntlCollator.cpp) plus a stress test. Before falling back to ucol_strcoll(), which requires UTF-16 and calls StringView::upconvertedCharacters() on both operands, it checks whether an 8-bit operand's length would exceed Vector<char16_t>'s maximum capacity. If so, it throws OutOfMemoryError instead of letting Vector::allocateBuffer<FailureAction::Crash> hit CRASH().
Security risks
None. This converts an unconditional process crash into a catchable JS RangeError. No new inputs are trusted, no new allocation paths are added, and the length check is purely arithmetic on an unsigned length. If anything this is a robustness/DoS improvement — a script can no longer kill the process via "a".repeat(1<<30).localeCompare("\u3042").
Level of scrutiny
Low-to-moderate. The change is confined to a single fallback branch inside one function, uses the same throw-and-return-{} pattern as the pre-existing U_FAILURE handler two lines below, and mirrors the precedent cited in the PR description (String.prototype.normalize at StringPrototype.cpp:2188-2192). I checked Vector.h:212 to confirm isValidCapacityForVector<T> is the exact predicate the Vector allocation path uses ((UINT_MAX >> 1) / sizeof(T), i.e. 0x3FFFFFFF for char16_t), so the guard is neither too tight nor too loose relative to the crash it prevents.
Other factors
- Both call sites wrap the result in
RELEASE_AND_RETURN(scope, ...), so the thrown exception propagates correctly even though a dummyUCOL_EQUALis returned — identical to how the existing ICU-failure branch already behaves. - The
[[unlikely]]annotation and the!view.is8Bit()short-circuit keep the hot path cost negligible. - A dedicated stress test exercises both argument positions, the default collator, and a collator with options that disables the DUCET fast path; it is gated on
$memoryLimited. - No prior human or bot review comments to address; no CODEOWNERS implicated for a localized runtime bugfix of this size.
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
b0d21d8 to
0a07044
Compare
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
0a07044 to
1a171c0
Compare
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
1a171c0 to
e30c7ff
Compare
|
The preview build for Until that job passes, no |
e30c7ff to
e92affa
Compare
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
e92affa to
de1149f
Compare
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
de1149f to
29a935c
Compare
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
29a935c to
5734434
Compare
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
5734434 to
53fc653
Compare
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
…ge Latin-1 string needs the UTF-16 upconversion
IntlCollator::compareStrings() falls back to ucol_strcoll() when none of
the ASCII fast paths apply. That path calls
StringView::upconvertedCharacters() on both operands. For a Latin-1
string of 1 << 30 or more characters, the Vector<char16_t> behind it is
past its maximum capacity (isValidCapacityForVector<char16_t>) and
CRASH()es.
"a".repeat(1 << 30).localeCompare("\u3042") killed the process. It now
throws RangeError: Out of memory, the same way String.prototype.normalize()
already handles an input of that size.
53fc653 to
345e73d
Compare
… crashing Points WEBKIT_VERSION at the preview build of oven-sh/WebKit#500. IntlCollator::compareStrings() falls back to ucol_strcoll() when none of the ASCII fast paths apply. That path upconverts each Latin-1 operand to UTF-16 through a Vector<char16_t>. A string of 2^30 or more characters is past the Vector's maximum capacity, and the allocation CRASH()ed the process. The WebKit change throws RangeError: Out of memory instead. Repro: "a".repeat(2 ** 30).localeCompare("\u3042").
1 participant
Problem
"a".repeat(1 << 30).localeCompare("\u3042")kills the process. The fuzzer found it in Bun with"DELETE".padEnd(1073741824).localeCompare(buffer), where the buffer's string form is 16-bit.IntlCollator::compareStrings()falls back toucol_strcoll()when none of the ASCII fast paths apply. That path callsStringView::upconvertedCharacters()on both operands. For a Latin-1 string of1 << 30or more characters theVector<char16_t>behind it is pastisValidCapacityForVector<char16_t>, andallocateBuffer<FailureAction::Crash>hitsCRASH()(Vector.h:228).JSString::MaxLengthisINT32_MAX), and a 16-bit operand of the same length compares fine. Only the Latin-1 upconversion has the lower limit.Fix
ucol_strcoll()fallback, check that each 8-bit operand fitsisValidCapacityForVector<char16_t>. If not, throwOutOfMemoryError(RangeError: Out of memory). This is whatString.prototype.normalize()already does for an input of that size (therdar://160634825check inStringPrototype.cpp).JSTests/stress/intl-collator-compare-huge-latin1-string.jscoverslocaleComparein both argument positions, the defaultIntl.Collator, and a collator with options (no UCA DUCET fast path). The second operand is 16-bit so the test does not scan the 1 GiB string. Skipped when$memoryLimited.The branch is rebased on
ceb9f90fb774, the WebKit Bun currently pins, so the preview build carries only this change.Bun side: oven-sh/bun#40253 pins the preview build and adds the regression test.