Conversation
…e size in bytes
310668@main ("Support CanBorrow in Vector<T>", webkit.org/b/311221) put the
borrow bit in the high bit of m_capacity, which counts elements, and says that
the maximum capacity is now 2^31 - 1. isValidCapacityForVector() halved the
limit on the size in bytes instead: (UINT_MAX >> 1) / sizeof(T). That halves
the capacity of every Vector whose T is wider than a byte. A Vector<char16_t>
went from 2^31 - 1 to 2^30 - 1 elements, so String::fromUTF8() and the other
converters that size a Vector<char16_t> by the length of their input call
CRASH() for an input that they converted before.
Keep the limit on the size in bytes from before that change, and add the limit
on the count: min(UINT_MAX / sizeof(T), UINT_MAX >> 1). Nothing changes for a
T of one byte.
* Source/WTF/wtf/Vector.h:
(WTF::isValidCapacityForVector):
* Source/WTF/wtf/text/StringImpl.h:
a585d76 to
f63021b
Compare
|
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; 1 remains after this review. WalkthroughThe change adds Bun-specific compile-time capacity checks. ChangesBun capacity validation
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is established by the available evidence. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue
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.
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: the non-Bun #else branch and every sizeof(T) == 1 limit are unchanged (the char8_t sites at StringImpl.h:1500-1537 keep the 31-bit byte ceiling, which the description states); allocateBuffer stores sizeToAllocate / sizeof(T), so m_capacity never receives a count above 2^31-1 on the checked path, and reallocateBuffer is only reached with a capacity that already passed that check; the new static_assert in StringImpl.h fails under the upstream formula (INT32_MAX <= (UINT_MAX >> 1) / 2 is false) and holds under the new one at the exact boundary. The Bun-side VectorSizeLimit.h asserts still encode the halved formula, as the description says, so the pin bump must land with the matching Bun change.
Extended reasoning...
The diff is 15 added lines, header-only, gated on USE(BUN_JSC_ADDITIONS), and the new constexpr is simple enough to check by hand: min(UINT_MAX / sizeof(T), UINT_MAX >> 1) is the pre-310668 byte limit intersected with the 31-bit element limit of m_capacity, and both static_asserts exercise the exact boundaries they claim. The one inline finding (append growth via nextCapacity overshooting the limit before reaching 2^31-1) is the substantive open point; the items above were examined and found not to be affected by this change or already disclosed by the description, so a human reviewer can concentrate on the growth path and on coordinating the Bun-side header.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
Source/WTF/wtf/Vector.h— Bun users appending to aVector<char16_t>still abort at roughly 1.43 G elements, one third below the 2^31-1 limit the new static_asserts claim is reachable.expandCapacityat Vector.h:1285 asks fornextCapacity(capacity()), which is 1.5x the current capacity (FastMalloc.h:279), and never clamps it to the limit.allocateBufferat Vector.h:235 then rejects that request and calls CRASH() at line 237 even though the element the caller wanted fits. Fix: clamp the growth request inexpandCapacityto the largest valid capacity for T before callingreserveCapacity, so any append below the limit succeeds on every Vector type.Extended reasoning...
The PR raises the ceiling and adds static_asserts saying a Vector<char16_t> holds StringImpl::MaxLength code units, but that is only true for sized construction. Trace: a Vector<char16_t> built by append reaches capacity c with c*1.5 > 2^31-1, that is c above about 1431655765. The next append calls expandCapacity (Vector.h:1285), which computes max(newMinCapacity, nextCapacity(c)) = c + c/2. reserveCapacity calls Base::allocateBuffer, and isValidCapacityForVector<char16_t>(c + c/2) at Vector.h:235 is false, so line 237 CRASHes the process. The catch-all fallible path returns false at line 239 only for tryAppend callers. Population: any Bun script that builds a large UTF-16 buffer incrementally (StringBuilder-style joins, URL and query builders, Bun's own C++ appending to Vector<char16_t>). The dismissing finder called it pre-existing, and the base does crash earlier (about 0.71 G), but this PR advertises and static_asserts a 2^31-1 ceiling that its own growth path cannot reach, so users hit an abort below the documented limit. Remedy: clamp nextCapacity to the maximum valid capacity…
Verification: pre-existing. Trigger: any Vector<char16_t> (or other T) grown by append/appendSlowCase reaching capacity c with c + c/2 > 2^31-1, i.e. c >= 1431655766, then appending one more element. Mechanism verified:
appendSlowCase(Source/WTF/wtf/Vector.h:1595) callsexpandCapacity<action>(size() + 1, ptr);expandCapacity(Vector.h:1285) computes `std::max(newMinCapacity, std::max(minCapacity,…
|
Preview build of f63021b: |
…ity from before WebKit 310668@main The abort is a regression in Bun 1.3.13. WebKit 310668@main took the high bit of Vector's element count for a borrow bit, but halved the limit on the size in bytes, so every Vector whose element is wider than a byte lost half of its capacity. oven-sh/WebKit#691 limits the count instead. fileURLToPath() returns the 2^30 byte path again, as Bun 1.3.12 does, and the test now expects that path and not "". VectorSizeLimit.h states the WebKit formula in two static_asserts, so it moves with the pin.
1 participant
Problem
Vector<T>callsCRASH()at half its former capacity, for everyTwider than a byte. Bun 1.3.13 took that change. Five scripts that Bun 1.3.12 runs abort on 1.3.13 and on main, also insidetry/catch(Notes). One isrequire("node:url").fileURLToPath("file:///%E4%B8%80" + "q".repeat(2 ** 30 - 1)).m_capacity, which counts elements. Its message says the maximum capacity is now 2^31 - 1.isValidCapacityForVector(Vector.h:212) halved the limit on the size in bytes instead, so aVector<char16_t>went from 2^31 - 1 to 2^30 - 1 elements.Fix
isValidCapacityForVectoriscapacity <= min(UINT_MAX / sizeof(T), UINT_MAX >> 1), underUSE(BUN_JSC_ADDITIONS): the limit from before 310668@main, and the 31 bits ofm_capacity.m_capacitycannot hold. Nothing changes for aTof one byte.static_asserts: a valid capacity fits in 31 bits (Vector.h), and aVector<char16_t>holdsStringImpl::MaxLengthcode units (StringImpl.h). A merge that takes the upstream line again does not compile.Background
m_capacityisunsigned : 31next tom_isBorrowed : 1inVectorBufferBase.isValidCapacityForVector<T>(n)guards each buffer allocation: the crashing path callsCRASH(), the fallible path returns false.Vector<char16_t>by the byte count of their input.Notes
Origin. A fuzzing run against Bun found the aborts one by one. No user reported them.
Regression table. Linux x64 release builds. 1.3.12 is from 2026-04-09 and 1.3.13 from 2026-04-19. 310668@main is from 2026-04-06.
url.fileURLToPath("file:///%E4%B8%80" + "q".repeat(2 ** 30 - 1)).length("\ud800" + "q".repeat(2 ** 30)).toWellFormed().lengthfor (let i = 0; i < 33554431; i++) queueMicrotask(f)new URLSearchParams("a=" + "\u4e00".repeat(2 ** 29)).get("a").lengthu = new URL("http://a/"); u.host = "q".repeat(2 ** 30); u.host.lengthThe stack of the first one on main ends in
allocateBuffer<(WTF::FailureAction)0>(Vector.h:228), from the sized constructor of theVector<char16_t, 1024>inStringImpl::create(std::span<const char8_t>)(StringImpl.cpp:289).Why it is safe.
allocateBuffercomputes the size in bytes insize_tand storesm_capacity = sizeToAllocate / sizeof(T)(Vector.h:233-242), so the count it stores is the count it checked.What I ran. Bun built against
autobuild-preview-pr-691-f63021bf(oven-sh/bun#42982), linux x64.queueMicrotaskscript runs in 215 s and thetoWellFormedscript in 12 s, both with exit 0 and nothing on stderr. The other three take several minutes each there and I did not run them.VectorSizeLimit.hstates this formula in twostatic_asserts. Bun compiles with the new formula there, and does not compile with the old one.What this does not give back. A
Vectorof bytes had a limit of 2^32 - 1 elements before 310668@main. The borrow bit leaves 31 bits for the count, so 2^31 - 1 stays the limit for a one-byteT. In Bun,console.count("q".repeat(2 ** 30))prints on 1.3.12 and aborts on 1.3.13, on main and with this change:StringImpl::tryGetUTF8ForCharactersasks for two bytes for each Latin-1 character, which is 2^31. oven-sh/bun#42868 covers that one.Other open pull requests that this touches.
String::fromUTF8for the first script. With this change no input reaches that path, and the script gets its correct result instead of"". I close [WTF] String::fromUTF8 returns a null string when its UTF-16 buffer cannot be allocated #683 when this lands.QueuedTaskonly (the third script).Upstream. The same line is in WebKit main. A report against webkit.org/b/311221 needs someone with an account there.
Bun side.
src/jsc/bindings/VectorSizeLimit.hhas twostatic_asserts that state the halved formula, so a pin bump to this change has to update that file in the same pull request. oven-sh/bun#42982 does that.