From d48b6a689d8cfbe177c3be666b136e54f4cdd1d5 Mon Sep 17 00:00:00 2001 From: robobun Date: Sun, 30 Aug 2026 17:01:06 +0000 Subject: [PATCH] JSON.stringify: give the DynamicBuffer fast path a depth limit so cyclic values fail fast A cyclic value made FastStringifier in DynamicBuffer mode re-serialize the cycle until it ran out of room. Without a gap that is the stack limit. With a gap the indentation grows with depth, so the buffer doubles up to the 2GB string length limit before the attempt fails: about 1.5 seconds and 2GB of writes per call, roughly 100000x slower than the general Stringifier, which detects the cycle at once and throws the TypeError. Values nested deeper than 512 now bail to the general Stringifier. jsc shell, x64 release, 8 keys of 50 bytes plus a self reference: cyclic flat 3.15 ms/op -> 0.13 ms/op cyclic indent 2 1464 ms/op -> 1.63 ms/op acyclic unchanged Fixes the JSON.stringify half of oven-sh/bun#40974. --- .../json-stringify-cyclic-depth-limit.js | 89 +++++++++++++++++++ Source/JavaScriptCore/runtime/JSONObject.cpp | 28 +++++- 2 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 JSTests/stress/json-stringify-cyclic-depth-limit.js diff --git a/JSTests/stress/json-stringify-cyclic-depth-limit.js b/JSTests/stress/json-stringify-cyclic-depth-limit.js new file mode 100644 index 000000000000..14000a4e2836 --- /dev/null +++ b/JSTests/stress/json-stringify-cyclic-depth-limit.js @@ -0,0 +1,89 @@ +// The DynamicBuffer FastStringifier bails to the general Stringifier past a +// depth limit, so a cyclic value is rejected after bounded work instead of +// growing the buffer toward the 2GB string length limit. This test checks the +// behavior around that limit: cycles still throw TypeError, and values nested +// deeper than the limit still stringify correctly through the general path. + +function shouldThrowTypeError(fn) { + let threw = null; + try { + fn(); + } catch (e) { + threw = e; + } + if (!(threw instanceof TypeError)) + throw new Error("expected TypeError, got " + threw); +} + +function makeCyclicObject(keys) { + const o = {}; + for (let i = 0; i < keys; i++) + o["k" + i] = "y".repeat(50); + o.self = o; + return o; +} + +// A payload large enough that the StaticBuffer attempt fails with BufferFull +// and the DynamicBuffer attempt runs. +const bigPayload = "y".repeat(10 * 1024); + +// Cyclic values throw, flat and with a gap, through objects and arrays, for +// direct and indirect cycles. +for (const space of [undefined, 2, "\t"]) { + for (const keys of [0, 8]) + shouldThrowTypeError(() => JSON.stringify(makeCyclicObject(keys), null, space)); + + shouldThrowTypeError(() => JSON.stringify([makeCyclicObject(1)], null, space)); + + const selfArray = [1]; + selfArray.push(selfArray); + shouldThrowTypeError(() => JSON.stringify(selfArray, null, space)); +} + +// Cycles that carry a large payload per revolution. +for (const space of [undefined, 2]) { + const a = { payload: bigPayload }; + const b = { a }; + a.b = b; + shouldThrowTypeError(() => JSON.stringify(a, null, space)); + + const direct = { payload: bigPayload }; + direct.self = direct; + shouldThrowTypeError(() => JSON.stringify(direct, null, space)); +} + +// Acyclic values nested deeper than any reasonable fast path depth limit +// round-trip correctly, flat and with a gap, for objects and arrays, with +// 8-bit and 16-bit content. +for (const space of [undefined, 1]) { + for (const leafValue of ["eight-bit", "sixte\u00e9n-bit \u2603"]) { + const depth = 1000; + + let root = {}; + let node = root; + for (let i = 0; i < depth; i++) + node = node.x = {}; + node.leaf = leafValue; + let parsed = JSON.parse(JSON.stringify(root, null, space)); + let p = parsed; + for (let i = 0; i < depth; i++) + p = p.x; + if (p.leaf !== leafValue) + throw new Error("deep object round-trip broken for space=" + space); + + let arrayRoot = []; + let arrayNode = arrayRoot; + for (let i = 0; i < depth; i++) { + const next = []; + arrayNode.push(next); + arrayNode = next; + } + arrayNode.push(leafValue); + parsed = JSON.parse(JSON.stringify(arrayRoot, null, space)); + p = parsed; + for (let i = 0; i < depth; i++) + p = p[p.length - 1]; + if (p[0] !== leafValue) + throw new Error("deep array round-trip broken for space=" + space); + } +} diff --git a/Source/JavaScriptCore/runtime/JSONObject.cpp b/Source/JavaScriptCore/runtime/JSONObject.cpp index e2ede9fae907..11caf8599398 100644 --- a/Source/JavaScriptCore/runtime/JSONObject.cpp +++ b/Source/JavaScriptCore/runtime/JSONObject.cpp @@ -682,6 +682,12 @@ bool Stringifier::Holder::appendNextProperty(Stringifier& stringifier, StringBui // it counts on hitting the buffer size limit to catch those things. If it fails, // since there is no side effect, the full general purpose Stringifier can be used // and the only cost of the fast stringifying attempt is the time wasted. +// +// In DynamicBuffer mode the buffer limit is the 2GB string length limit, so the +// buffer limit alone is far too expensive as a cycle check: a cyclic value with a +// gap reaches it only after writing (and repeatedly reallocating) gigabytes. A +// depth limit bounds that wasted work. Values nested deeper than the limit take +// the general Stringifier, whose holder stack detects cycles immediately. enum class BufferMode : uint8_t { StaticBuffer, @@ -695,6 +701,7 @@ enum class FailureReason : uint8_t { Found16BitEarly, Found16BitLate, StackOverflow, + DepthLimit, Unknown, }; @@ -707,6 +714,15 @@ class FastStringifier { static constexpr unsigned staticBufferSize = bufferMode == BufferMode::StaticBuffer ? 8192 : 8; static constexpr unsigned dynamicBufferInlineCapacity = bufferMode == BufferMode::StaticBuffer ? 0 : 1024; + // DynamicBuffer mode only: values nested deeper than this bail to the general + // Stringifier. Keeps a cyclic value from filling the buffer up to the string + // length limit before the general Stringifier gets to throw for the cycle. + static constexpr unsigned maximumDepth = 512; + + // m_depth drives the indentation when there is a gap, and the maximumDepth + // check in DynamicBuffer mode. + static constexpr bool trackDepthWithoutGap = bufferMode == BufferMode::DynamicBuffer; + static constexpr bool useShortCopyTier = bufferMode == BufferMode::DynamicBuffer; private: @@ -1351,6 +1367,10 @@ void FastStringifier::append(JSValue value) recordFailure(FailureReason::StackOverflow, "stack overflow"_s); return; } + if (m_depth >= maximumDepth) [[unlikely]] { + recordFailure(FailureReason::DepthLimit, "depth limit"_s); + return; + } } if (value.isNull()) { @@ -1525,7 +1545,7 @@ void FastStringifier::append(JSValue value) recordFailure("object has non-reified static properties"_s); return; } - if constexpr (hasGap == HasGap::Yes) + if constexpr (hasGap == HasGap::Yes || trackDepthWithoutGap) ++m_depth; const unsigned newLineAndIndent = hasGap == HasGap::Yes ? newLineAndIndentSize() : 0; structure.forEachProperty(m_vm, [&](const auto& entry) -> bool { @@ -1654,7 +1674,7 @@ void FastStringifier::append(JSValue value) }); if (haveFailure()) [[unlikely]] return; - if constexpr (hasGap == HasGap::Yes) + if constexpr (hasGap == HasGap::Yes || trackDepthWithoutGap) --m_depth; bool needNewLine = hasGap == HasGap::Yes && buffer()[m_length - 1] != '{'; if (!hasRemainingCapacity(needNewLine ? 1 + newLineAndIndentSize() : 1)) [[unlikely]] { @@ -1701,11 +1721,11 @@ void FastStringifier::append(JSValue value) return; } buffer()[m_length++] = '['; - if constexpr (hasGap == HasGap::Yes) + if constexpr (hasGap == HasGap::Yes || trackDepthWithoutGap) ++m_depth; auto closeArray = [&] { - if constexpr (hasGap == HasGap::Yes) + if constexpr (hasGap == HasGap::Yes || trackDepthWithoutGap) --m_depth; bool needNewLine = hasGap == HasGap::Yes && buffer()[m_length - 1] != '['; if (!hasRemainingCapacity(needNewLine ? 1 + newLineAndIndentSize() : 1)) [[unlikely]] {