IntlCollator: throw OOM instead of crashing when upconverting a >=2^30 Latin-1 string#313
IntlCollator: throw OOM instead of crashing when upconverting a >=2^30 Latin-1 string#313robobun wants to merge 1 commit into
Conversation
…0 Latin-1 string
StringView::upconvertedCharacters() grows a Vector<char16_t>, and
WTF::Vector caps capacity at (UINT_MAX >> 1) / sizeof(T). For char16_t
that is 2^30 - 1 elements, so an 8-bit StringView of length >= 2^30
hits the FailureAction::Crash path in VectorBufferBase::allocateBuffer
and aborts the process.
compareStrings() reaches the ucol_strcoll fallback whenever the DUCET
fast path bails (control characters, non-ASCII Latin-1, or a collator
that cannot use the table). With a Latin-1 string at the 1 GiB boundary
that upconversion is the first allocation we make, so
'x'.localeCompare('\x00'.repeat(2**30)) and the equivalent
Intl.Collator().compare() SIGABRT.
Check the length before upconverting and throw an Out of memory
RangeError, matching the guard that already protects
String.prototype.normalize() in StringPrototype.cpp.
|
Warning Review limit reached
Next review available in: 26 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 (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small defensive guard that converts a fuzzer-found SIGABRT into a RangeError.
What was reviewed:
- Confirmed
upconvertedCharacters()callsVector<char16_t>::grow(length)only whenis8Bit(), so gating onis8Bit()is correct and 16-bit inputs are unaffected. - Verified the
>= (1u << 30)threshold matches the WTF::Vector capacity cap forchar16_tand mirrors the existing guard inString.prototype.normalize(StringPrototype.cpp:2223). - Checked that both fast paths (DUCET ASCII table,
ucol_strcollUTF8for all-ASCII 8-bit) are evaluated before this branch, so ordinary comparisons are untouched.
Extended reasoning...
Overview
This PR adds a 3-line length guard in IntlCollator::compareStrings (IntlCollator.cpp) just before the ucol_strcoll fallback that unconditionally calls StringView::upconvertedCharacters() on both operands. If either operand is an 8-bit string of length ≥ 2^30, it throws Out of memory instead of letting Vector<char16_t>::grow hit FailureAction::Crash in VectorBufferBase::allocateBuffer. The only other change is adding braces around the now-multi-statement if (!result) body.
Security risks
None introduced. This strictly reduces attack surface: a previously-reachable process abort (SIGABRT via CRASH()) triggered by user-controlled string length is now a catchable JavaScript RangeError. No new data flows, no relaxed checks. The threshold arithmetic (1u << 30) is well-defined for 32-bit unsigned and matches StringView::length()'s unsigned type.
Level of scrutiny
Low. The change is tiny, purely additive, and sits behind an [[unlikely]] branch that only fires for pathological ≥1 GiB Latin-1 inputs that reach the ICU fallback (i.e., contain control chars or non-ASCII Latin-1, or use a non-DUCET collator). It is a direct copy of the pattern already landed for String.prototype.normalize under rdar://160634825 — I confirmed the identical >= (1 << 30) check at StringPrototype.cpp:2223 guarding the same upconvertedCharacters() call. I also confirmed in StringView.h that UpconvertedCharactersWithSize only grows the Vector<char16_t> when the view is8Bit(), so the is8Bit() predicate in the guard is exactly right and 16-bit strings still pass through without allocation.
Other factors
The DUCET fast path and the ucol_strcollUTF8 all-ASCII path both run before this guard, so a 1 GiB plain-ASCII string continues to compare without allocating or throwing — the PR description's claim holds. The existing ThrowScope in the function is reused, and return { } on the throw path is consistent with the U_FAILURE branch immediately below. No callers need updating since the function already had a throwing path. Given the mechanical nature, the established precedent in the same codebase, and that this hardens a fuzzer-found crash, this is safe to approve without human review.
Preview Builds
|
1 participant
Problem
'x'.localeCompare('\x00'.repeat(2**30))(and the equivalentIntl.Collator().compare()) aborts the process with SIGABRT.Found by Fuzzilli against Bun.
Cause
StringView::upconvertedCharacters()grows aVector<char16_t>to hold the UTF-16 copy. WTF::Vector caps capacity at(UINT_MAX >> 1) / sizeof(T), which forchar16_tis2^30 - 1elements. An 8-bitStringViewof length>= 2^30therefore hits theFailureAction::Crashpath inVectorBufferBase::allocateBufferand callsCRASH().IntlCollator::compareStringsreaches theucol_strcollfallback whenever the DUCET ASCII fast path bails out (control characters with a zeroducetLevel1Weightsentry, non-ASCII Latin-1, or a collator that cannot use the table at all). At that point it upconverts both operands unconditionally.Fix
Check for an 8-bit operand at or above the
2^30threshold before callingupconvertedCharacters()and throw anOut of memoryRangeError, the same guard that already protects the upconversion inString.prototype.normalize()(StringPrototype.cpp, rdar://160634825). The DUCET fast path is untouched, so a 1 GiB string of ordinary ASCII still compares without allocating.Repro