LiteralParser: throw RangeError on OOM when copying a JSON string value - #317
Conversation
JSONAtomStringCache::makeJSString called String(characters) for string values longer than 16 chars. That constructor goes through StringImpl::createUninitializedInternalNonEmpty -> StringImplMalloc::malloc, which is the crash-or-succeed allocator. Parsing a JSON string value that cannot be allocated killed the process. Use StringImpl::tryCreateUninitialized and surface a null result up through LiteralParser::parsePrimitiveValue, which now throws the same RangeError: Out of memory that String.prototype.repeat and friends throw.
WalkthroughLarge JSON strings now use explicit ChangesJSON string allocation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the change looks correct and follows the established tryCreateUninitialized + throwOutOfMemoryError pattern — but this sits on the JSON.parse hot path (every string value > 16 chars flows through the new branch in an ALWAYS_INLINE function), so it's worth a human confirming the success path stays perf-neutral.
What was reviewed:
StringImpl::tryCreateUninitialized(size_t, span&)andString(RefPtr<StringImpl>&&)signatures match the new call site;copyElementsmirrors existing usage in this file.- All
parsePrimitiveValuecallers (parseRecursively,parse,evalRecursivelyEntry,tryStreamingParse) already treat an empty return as failure and bail;parseRecursively'sEXCEPTION_ASSERTcovers the new pending-exception case. tryStreamingParsecan already receive!valuewith a pending exception viaparseRecursively'sRETURN_IF_EXCEPTION, so the new OOM path doesn't introduce a new state there.
Extended reasoning...
Overview
Two-file change to make JSON.parse throw RangeError: Out of memory instead of crashing when copying a large JSON string value fails to allocate. JSONAtomStringCache::makeJSString swaps the crash-or-succeed String(span) constructor for StringImpl::tryCreateUninitialized + WTF::copyElements, returning nullptr on failure. LiteralParser::parsePrimitiveValue gains a null check that declares a throw scope and calls throwOutOfMemoryError.
Security risks
None identified. The change only affects allocation-failure handling; the success path is a byte-for-byte equivalent (allocate uninitialized StringImpl of characters.size(), memcpy the span in). No new user-controlled sizes or pointer arithmetic — characters.size() was already the allocation size before.
Level of scrutiny
Medium-high. This is core JavaScriptCore runtime on the JSON.parse hot path: makeJSString is ALWAYS_INLINE and the modified branch runs for every JSON string value longer than 16 characters. The transformation itself is mechanical and matches the pattern used widely across JSC for fallible string allocation, and the added branch is [[unlikely]], but hot-path changes in JSC generally warrant a human look for codegen/inlining regressions.
Other factors
I traced every parsePrimitiveValue call site in LiteralParser.cpp: each one checks if (!result/value/primitive/lastValue) and returns {}, and the recursive/iterative parsers already have throw scopes with EXCEPTION_ASSERT((!!scope.exception() || !m_parseErrorMessage.isNull()) == !value), which the new state (empty value + pending exception, null m_parseErrorMessage) satisfies. tryStreamingParse (Bun-specific) could already see !value with a pending exception via constructEmptyArray OOM in parseRecursively, so its !value → Error/NeedMoreData handling is not a new concern introduced here. The PR description's caller analysis matches what I found, and the bug-hunting pass surfaced nothing.
Preview Builds
|
Follows the JSC convention of prefixing try- on functions that return nullptr on allocation failure (tryCreateUninitialized, tryMakeString).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/JavaScriptCore/runtime/JSONAtomStringCacheInlines.h (1)
98-104: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd an OOM regression test for both character widths.
Exercise forced
tryCreateUninitializedfailure for 8-bit and 16-bit JSON strings, asserting that parsing raisesRangeError: Out of memoryinstead of terminating or reporting a syntax error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/JavaScriptCore/runtime/JSONAtomStringCacheInlines.h` around lines 98 - 104, Add regression coverage for the JSON parsing path using both 8-bit and 16-bit strings, forcing StringImpl::tryCreateUninitialized to fail. Assert each parse raises RangeError with the message “Out of memory,” and verify it does not terminate or produce a syntax error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Source/JavaScriptCore/runtime/JSONAtomStringCacheInlines.h`:
- Around line 98-104: Add regression coverage for the JSON parsing path using
both 8-bit and 16-bit strings, forcing StringImpl::tryCreateUninitialized to
fail. Assert each parse raises RangeError with the message “Out of memory,” and
verify it does not terminate or produce a syntax error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f82340d8-ac6d-45be-860e-75f8b1e68a2b
📒 Files selected for processing (4)
Source/JavaScriptCore/runtime/JSONAtomStringCache.hSource/JavaScriptCore/runtime/JSONAtomStringCacheInlines.hSource/JavaScriptCore/runtime/LiteralParser.cppSource/JavaScriptCore/runtime/LiteralParser.h
There was a problem hiding this comment.
I didn't find any issues — the success path is identical to what String(span) did internally, just with tryMalloc instead of the crash-on-fail path. Deferring to a human only because this sits directly on the JSON.parse string-materialization hot path.
What was reviewed:
tryCreateUninitializedsignature/semantics vs. the oldString(span)→createUninitializedInternalNonEmptypath — same allocation size and copy, only the malloc variant differs.- All
parsePrimitiveValuecall sites (parseRecursively,parse,evalRecursivelyEntry,tryLiteralParsePrimitiveValue,tryStreamingParse) already treat an empty return as failure and callers check for pending exceptions. jsNontrivialStringlength assertion — this branch only runs forsize() > 16, solength() > 1holds.- Rename is complete;
LiteralParseris the only caller ofJSONAtomStringCache::tryMakeJSString.
Extended reasoning...
Overview
The PR renames JSONAtomStringCache::makeJSString → tryMakeJSString and replaces the > 16 char branch's jsNontrivialString(vm, String(characters)) with an explicit StringImpl::tryCreateUninitialized + copyElements + jsNontrivialString. On allocation failure it returns nullptr, which LiteralParser::parsePrimitiveValue converts into throwOutOfMemoryError and an empty JSValue. Four files touched: two headers for the rename, JSONAtomStringCacheInlines.h for the allocation change, and LiteralParser.cpp for the null check + throw.
Security risks
None introduced. The buffer returned by tryCreateUninitialized is exactly characters.size() elements and is filled by WTF::copyElements(buffer, characters) from a span of the same length, so there is no under/over-copy. tryCreateUninitialized already guards isValidLength<CharacterType>(length) before computing allocationSize, and stringOrIdentifierLength is a 31-bit unsigned, so no overflow concerns are added. The change strictly turns a process crash into a catchable RangeError.
Level of scrutiny
High — this is the string-value materialization step of JSON.parse, one of the hottest paths in the runtime. That said, the success path is byte-for-byte what String(std::span<const CharacterType>) already did (allocate uninitialized StringImpl of length, copyElements into it), only substituting StringImplMalloc::tryMalloc for the crashing malloc. The added branch is [[unlikely]] and only taken on OOM.
Other factors
- Verified
String(RefPtr<StringImpl>&&)exists (WTFString.h:84) andjsNontrivialString(VM&, String&&)callsreleaseImpl().releaseNonNull()— safe becauseimplis non-null on this branch and length > 16. - Every
parsePrimitiveValuecaller already branches on!valueand either has aThrowScopewithRETURN_IF_EXCEPTIONupstream or asserts the exception/error-message invariant, so the new pending-exception-with-empty-return combination is handled without further changes. WTF::moveandWTF::copyElementsare already used in the same function for the atom-cache branch, so no new includes or idioms.
Deferring per the guideline to not auto-approve changes on critical hot paths, not because of any concrete concern with the diff.
2 participants
What
JSONAtomStringCache::makeJSStringconstructed a newStringfrom the lexer's span for every JSON string value longer than 16 chars.String(characters)lands inStringImpl::createUninitializedInternalNonEmptywhich callsStringImplMalloc::malloc, the crash-or-succeed path. When the allocator cannot satisfy that request,JSON.parsekills the process instead of throwing.This switches the copy to
StringImpl::tryCreateUninitializedand bubbles a null result up toLiteralParser::parsePrimitiveValue, which throwsRangeError: Out of memory(the same errorString.prototype.repeatand other WTF-string callers throw on allocation failure). All ofparsePrimitiveValue's callers already treat an empty return with a pending exception correctly:parseRecursivelyasserts(!!scope.exception() || !m_parseErrorMessage.isNull()) == !value, and the top-levelJSON.parsecallers inJSONObject.cppcheckRETURN_IF_EXCEPTIONbefore falling back to aSyntaxError.Why
Seen in Sentry as a
fastCompactMallocnull dereference on Windows (Bun issue BUN-2Z94). Reproducible on Linux by runningJSON.parseon a ~200 MB quoted string underulimit -vwith the remaining address space filled so the value copy cannot fit:The
maxAtomizeStringLength == 16cached/atomized branches are unchanged; a 16-byte allocation failing is not a realistic path, andAtomStringImpl::addalready returnsRefPtr.