URLParser: do not re-parse a String::MaxLength input under ASSERT_ENABLED - #621
URLParser: do not re-parse a String::MaxLength input under ASSERT_ENABLED#621robobun wants to merge 1 commit into
Conversation
…BLED
After a parse that saw no syntax violation, the URLParser constructor
re-parses makeString(' ', inputString) and compares the two results. For
an input that is already String::MaxLength (2147483647) characters long
the length sum overflows int32_t, tryMakeString returns a null String,
and makeString calls CRASH(). Every WTF::URL built from a maximum length
string then aborts the process in a build with assertions on.
Build the string with tryMakeString and skip the check when it returns
a null String.
There was a problem hiding this comment.
LGTM — small, debug-only fix that swaps makeString for tryMakeString and skips the self-check when allocation fails.
What was reviewed:
- Confirmed the change is entirely inside the
#if ASSERT_ENABLEDblock; release behavior is unchanged. - Verified
tryMakeStringis in scope (<wtf/text/MakeString.h>is already included and it is exported into the global namespace) and thatWTF::moveis defined inStdLibExtras.hand accepts the lvalue here. - Checked that all shorter inputs still take the identical re-parse-and-compare path as before; only the null-result case is newly skipped.
Extended reasoning...
Overview
The PR touches a single file, Source/WTF/wtf/URLParser.cpp, and only the #if ASSERT_ENABLED self-check block inside the URLParser constructor. Previously, after a parse with no syntax violation the constructor called makeString(' ', inputString) to force a syntax violation and re-parse for comparison. When inputString is exactly String::MaxLength characters, the length sum overflows the CheckedInt32 inside tryMakeStringFromAdapters, tryMakeString returns null, and makeString calls CRASH(). The fix builds the string with tryMakeString directly, skips the comparison when it returns null, and moves the resulting string into the nested parser instead of copying.
Security risks
None. The modified code is compiled only when ASSERT_ENABLED is set, so release builds are byte-for-byte unaffected. The change strictly narrows the behavior of a debug self-check (skip instead of abort) for one degenerate input length; no parsing, canonicalization, or output logic changes. There is no new untrusted input handling, no auth/crypto/permission surface.
Level of scrutiny
Low. This is a 10-line, mechanical change in a debug-only assertion path with clear intent. I confirmed tryMakeString is already available via the existing #include <wtf/text/MakeString.h> (and exported with using WTF::tryMakeString;), and that WTF::move is the codebase's std::move wrapper in wtf/StdLibExtras.h whose is_lvalue_reference static_assert is satisfied by the named local. The nested URLParser constructor takes String&&, so the move matches the previous rvalue-temporary binding. For every input shorter than String::MaxLength, tryMakeString returns a non-null string and the re-parse-and-compare check runs exactly as before.
Other factors
No CODEOWNERS entry covers Source/WTF/wtf/URLParser.cpp. There are no prior reviews or outstanding objections on the PR. The bug hunter exited on dry_streak with no findings and no ruled-out candidates. The PR description's reasoning (overflow in CheckedInt32 → null → CRASH() in makeString) matches the implementation in MakeString.h.
|
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. WalkthroughChangesURL parser equivalence check
Merge Risk: ⚪ Minimal · up to Maximum-length URL inputs now avoid the assertion abort while shorter inputs retain the equivalence check. No actionable merge-blocking risk remains. 🚥 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
|
Problem
WTF::URLbuilt from a string of exactlyString::MaxLength(2147483647) characters aborts the process in a build with assertions on. Release builds are not affected. In Bun,new URL(s)andnew WebSocket(s)with such a string exit 134 on a debug build and throw on a release build.URLParserconstructor (Source/WTF/wtf/URLParser.cpp:1572). After a parse that saw no syntax violation it re-parsesmakeString(' ', inputString)and compares the two results.tryMakeStringsums the adapter lengths into aCheckedInt32, so1 + 2147483647overflows and it returns a nullString.makeStringthen callsCRASH().Fix
tryMakeStringand skip the check when it returns a nullString. Every shorter input still gets the second parse.dfd696443b, the commit Bun pins, so the preview build differs from the pin by this change only.Verification
Source/WTF/wtf/URLParser.cppcompiles with the flags from the pinned build'scompile_commands.json(clang++-21 -std=c++23 -fsyntax-only,ASSERT_ENABLEDon).autobuild-preview-pr-621-b620d880):test/js/web/url/on the same build: 138 pass, 16 skip, 0 fail.URLParser::allValuesEqualis hit3 times for one
new URL("https://example.com/a?b#c").new URL()call on it takes 5m54s and 2.4 GB of RSS in a debug build.Notes
new URL,new WebSocket,fetchand every other entry point that hands a script-supplied string toWTF::URL.new WebSocket("ws://127.0.0.1:1/", { proxy: s })still aborts with this change. That one is a separatemakeStringoverflow in Bun's ownsrc/jsc/bindings/webcore/WebSocket.cpp, which WebSocket: bound every script-supplied value that an error message quotes bun#42216 fixes.Vector::allocateBuffer(Vector.h:228) whenm_asciiBufferreserves pastisValidCapacityForVector. For examplenew URL("a:/.// " + "x".repeat(2147483633)). That is theVectorcapacity family that URLParser: stop crashing on URL-encoded forms that do not fit a Vector #520 covers, and it is out of scope here.