From 577bfcbbcf2a419fc50cc484994fb8ea2404b1f9 Mon Sep 17 00:00:00 2001 From: He-Pin Date: Thu, 25 Jun 2026 04:25:57 +0800 Subject: [PATCH] fix: guard recursive array materialization Motivation: Nested lazy arrays can re-enter the same array value during manifestation. The generic materializer and ByteRenderer previously only guarded object cycles, so recursive arrays could grow partial output until stack or memory failure. Modification: Track currently materialized arrays and objects in the existing identity guard, with try/finally cleanup on recursive, stackless, and ByteRenderer direct paths. Keep compact RangeArr and ByteArr fast paths outside the guard. Add golden regressions for nested lazy array cycles and shared acyclic arrays. Result: Recursive array manifestation now fails before writing partial JSON while shared non-cyclic arrays still materialize normally. --- sjsonnet/src/sjsonnet/ByteRenderer.scala | 58 ++-- sjsonnet/src/sjsonnet/Materializer.scala | 263 ++++++++++-------- .../error.lazy_nested_array_cycle.jsonnet | 3 + ...ror.lazy_nested_array_cycle.jsonnet.golden | 2 + ...materialize_shared_array_not_cycle.jsonnet | 6 + ...lize_shared_array_not_cycle.jsonnet.golden | 32 +++ 6 files changed, 232 insertions(+), 132 deletions(-) create mode 100644 sjsonnet/test/resources/new_test_suite/error.lazy_nested_array_cycle.jsonnet create mode 100644 sjsonnet/test/resources/new_test_suite/error.lazy_nested_array_cycle.jsonnet.golden create mode 100644 sjsonnet/test/resources/new_test_suite/materialize_shared_array_not_cycle.jsonnet create mode 100644 sjsonnet/test/resources/new_test_suite/materialize_shared_array_not_cycle.jsonnet.golden diff --git a/sjsonnet/src/sjsonnet/ByteRenderer.scala b/sjsonnet/src/sjsonnet/ByteRenderer.scala index 7b1d9171d..b76496192 100644 --- a/sjsonnet/src/sjsonnet/ByteRenderer.scala +++ b/sjsonnet/src/sjsonnet/ByteRenderer.scala @@ -273,7 +273,7 @@ class ByteRenderer(out: OutputStream = new java.io.ByteArrayOutputStream(), inde obj: Val.Obj, matDepth: Int, ctx: Materializer.MaterializeContext)(implicit evaluator: EvalScope): Unit = { - if (!ctx.enterObject(obj)) + if (!ctx.enterContainer(obj)) Error.fail("Stackoverflow while materializing, possibly due to recursive value", obj.pos) try { obj.triggerAllAsserts(ctx.brokenAssertionLogic) @@ -287,7 +287,7 @@ class ByteRenderer(out: OutputStream = new java.io.ByteArrayOutputStream(), inde materializeDirectGenericObj(obj, matDepth, ctx) } } finally { - ctx.exitObject(obj) + ctx.exitContainer(obj) } } @@ -495,15 +495,10 @@ class ByteRenderer(out: OutputStream = new java.io.ByteArrayOutputStream(), inde ctx: Materializer.MaterializeContext)(implicit evaluator: EvalScope): Unit = { val len = xs.length - // Inline of visitArray — open bracket - elemBuilder.append('[') - newlineBuffered = true - depth += 1 - resetEmpty() - // Fast paths for compact numeric arrays: emit numbers directly without per-element dispatch. xs match { case range: Val.RangeArr if range.isCompactRange => + openArrBracket() var i = 0 while (i < len) { markNonEmpty() @@ -512,7 +507,9 @@ class ByteRenderer(out: OutputStream = new java.io.ByteArrayOutputStream(), inde commaBuffered = true i += 1 } + closeArrBracket() case ba: Val.ByteArr => + openArrBracket() val bytes = ba.rawBytes var i = 0 while (i < len) { @@ -522,23 +519,42 @@ class ByteRenderer(out: OutputStream = new java.io.ByteArrayOutputStream(), inde commaBuffered = true i += 1 } + closeArrBracket() case _ => - var i = 0 - while (i < len) { - val childVal = xs.value(i) - - markNonEmpty() - flushBuffer() - - // Render element directly — no flush overhead - materializeChild(childVal, matDepth, ctx) - - commaBuffered = true - i += 1 + if (!ctx.enterContainer(xs)) + Error.fail("Stackoverflow while materializing, possibly due to recursive value", xs.pos) + try { + openArrBracket() + var i = 0 + while (i < len) { + val childVal = xs.value(i) + + markNonEmpty() + flushBuffer() + + // Render element directly — no flush overhead + materializeChild(childVal, matDepth, ctx) + + commaBuffered = true + i += 1 + } + closeArrBracket() + } finally { + ctx.exitContainer(xs) } } + } + + /** Open an array bracket and initialize depth/empty state. */ + @inline private def openArrBracket(): Unit = { + elemBuilder.append('[') + newlineBuffered = true + depth += 1 + resetEmpty() + } - // Inline of visitEnd — close bracket + /** Close an array bracket, handling empty vs non-empty formatting. */ + @inline private def closeArrBracket(): Unit = { commaBuffered = false newlineBuffered = false val wasEmpty = isEmpty diff --git a/sjsonnet/src/sjsonnet/Materializer.scala b/sjsonnet/src/sjsonnet/Materializer.scala index 262cc3034..f5bcb8fe0 100644 --- a/sjsonnet/src/sjsonnet/Materializer.scala +++ b/sjsonnet/src/sjsonnet/Materializer.scala @@ -102,7 +102,7 @@ abstract class Materializer { visitor: Visitor[T, T], depth: Int, ctx: Materializer.MaterializeContext)(implicit evaluator: EvalScope): T = { - if (!ctx.enterObject(obj)) + if (!ctx.enterContainer(obj)) Error.fail("Stackoverflow while materializing, possibly due to recursive value", obj.pos) try { storePos(obj.pos) @@ -131,7 +131,7 @@ abstract class Materializer { ov.visitEnd(-1) } } finally { - ctx.exitObject(obj) + ctx.exitContainer(obj) } } @@ -277,17 +277,28 @@ abstract class Materializer { case _ => } val len = xs.length - val av = visitor.visitArray(len, -1) - var i = 0 - while (i < len) { - val childVal = xs.value(i) - av.visitValue( - materializeRecursiveChild(childVal, av.subVisitor.asInstanceOf[Visitor[T, T]], depth, ctx), - -1 - ) - i += 1 + if (!ctx.enterContainer(xs)) + Error.fail("Stackoverflow while materializing, possibly due to recursive value", xs.pos) + try { + val av = visitor.visitArray(len, -1) + var i = 0 + while (i < len) { + val childVal = xs.value(i) + av.visitValue( + materializeRecursiveChild( + childVal, + av.subVisitor.asInstanceOf[Visitor[T, T]], + depth, + ctx + ), + -1 + ) + i += 1 + } + av.visitEnd(-1) + } finally { + ctx.exitContainer(xs) } - av.visitEnd(-1) } private def materializeRecursiveChild[T]( @@ -359,89 +370,98 @@ abstract class Materializer { Math.max(16, Math.min(ctx.recursiveDepthLimit * 4, 8192)) ) - // Push the initial container frame - v match { - case obj: Val.Obj => pushObjFrame(obj, visitor, stack, ctx) - case xs: Val.Arr => pushArrFrame(xs, visitor, stack, ctx) - case _ => () // unreachable - } + try { + // Push the initial container frame + v match { + case obj: Val.Obj => pushObjFrame(obj, visitor, stack, ctx) + case xs: Val.Arr => pushArrFrame(xs, visitor, stack, ctx) + case _ => () // unreachable + } + + while (true) { + stack.peekFirst() match { + case frame: Materializer.MaterializeObjFrame[T @unchecked] => + val keys = frame.keys + val ov = frame.objVisitor + if (frame.index < keys.length) { + val key = keys(frame.index) + val childVal = frame.obj.value(key, ctx.emptyPos) + storePos(childVal) - while (true) { - stack.peekFirst() match { - case frame: Materializer.MaterializeObjFrame[T @unchecked] => - val keys = frame.keys - val ov = frame.objVisitor - if (frame.index < keys.length) { - val key = keys(frame.index) - val childVal = frame.obj.value(key, ctx.emptyPos) - storePos(childVal) - - if (frame.sort) { - if (frame.prevKey != null && Util.compareStringsByCodepoint(key, frame.prevKey) <= 0) - Error.fail( - s"""Internal error: Unexpected key "$key" after "${frame.prevKey}" in sorted object materialization""", - childVal.pos + if (frame.sort) { + if ( + frame.prevKey != null && Util.compareStringsByCodepoint(key, frame.prevKey) <= 0 ) - frame.prevKey = key + Error.fail( + s"""Internal error: Unexpected key "$key" after "${frame.prevKey}" in sorted object materialization""", + childVal.pos + ) + frame.prevKey = key + } + + ov.visitKeyValue(ov.visitKey(-1).visitString(key, -1)) + frame.index += 1 + + val sub = ov.subVisitor.asInstanceOf[Visitor[T, T]] + materializeChild(childVal, sub, ov, stack, ctx) + } else { + ctx.exitContainer(frame.obj) + val result = ov.visitEnd(-1) + stack.removeFirst() + if (stack.isEmpty) return result + feedResult(stack.peekFirst(), result) } - ov.visitKeyValue(ov.visitKey(-1).visitString(key, -1)) - frame.index += 1 - - val sub = ov.subVisitor.asInstanceOf[Visitor[T, T]] - materializeChild(childVal, sub, ov, stack, ctx) - } else { - ctx.exitObject(frame.obj) - val result = ov.visitEnd(-1) - stack.removeFirst() - if (stack.isEmpty) return result - feedResult(stack.peekFirst(), result) - } - - case frame: Materializer.MaterializeArrFrame[T @unchecked] => - val arr = frame.arr - val av = frame.arrVisitor - // Fast paths for compact numeric arrays: emit all elements directly. - if (frame.index == 0) { - arr match { - case range: Val.RangeArr if range.isCompactRange => - val len = range.length - var i = 0 - val sub = av.subVisitor.asInstanceOf[Visitor[T, T]] - while (i < len) { - av.visitValue(sub.visitFloat64(range.doubleAt(i), -1), -1) - i += 1 - } - frame.index = len // mark as done - case ba: Val.ByteArr => - val bytes = ba.rawBytes - val len = bytes.length - var i = 0 - while (i < len) { + case frame: Materializer.MaterializeArrFrame[T @unchecked] => + val arr = frame.arr + val av = frame.arrVisitor + // Fast paths for compact numeric arrays: emit all elements directly. + if (frame.index == 0) { + arr match { + case range: Val.RangeArr if range.isCompactRange => + val len = range.length + var i = 0 val sub = av.subVisitor.asInstanceOf[Visitor[T, T]] - av.visitValue(sub.visitFloat64((bytes(i) & 0xff).toDouble, -1), -1) - i += 1 - } - frame.index = len // mark as done - case _ => + while (i < len) { + av.visitValue(sub.visitFloat64(range.doubleAt(i), -1), -1) + i += 1 + } + frame.index = len // mark as done + case ba: Val.ByteArr => + val bytes = ba.rawBytes + val len = bytes.length + var i = 0 + val sub = av.subVisitor.asInstanceOf[Visitor[T, T]] + while (i < len) { + av.visitValue(sub.visitFloat64((bytes(i) & 0xff).toDouble, -1), -1) + i += 1 + } + frame.index = len // mark as done + case _ => + } + } + if (frame.index < arr.length) { + val childVal = arr.value(frame.index) + frame.index += 1 + + val sub = av.subVisitor.asInstanceOf[Visitor[T, T]] + materializeChild(childVal, sub, av, stack, ctx) + } else { + ctx.exitContainer(frame.arr) + val result = av.visitEnd(-1) + stack.removeFirst() + if (stack.isEmpty) return result + feedResult(stack.peekFirst(), result) } - } - if (frame.index < arr.length) { - val childVal = arr.value(frame.index) - frame.index += 1 - - val sub = av.subVisitor.asInstanceOf[Visitor[T, T]] - materializeChild(childVal, sub, av, stack, ctx) - } else { - val result = av.visitEnd(-1) - stack.removeFirst() - if (stack.isEmpty) return result - feedResult(stack.peekFirst(), result) - } + } } - } - null.asInstanceOf[T] // unreachable — while(true) exits via return + null.asInstanceOf[T] // unreachable — while(true) exits via return + } finally { + while (!stack.isEmpty) { + exitFrameContainer(stack.removeFirst(), ctx) + } + } } // Materialize a child value in iterative mode. Single match dispatches leaf values directly @@ -496,17 +516,23 @@ abstract class Materializer { stack: java.util.ArrayDeque[Materializer.MaterializeFrame], ctx: Materializer.MaterializeContext)(implicit evaluator: EvalScope): Unit = { checkDepth(obj.pos, stack.size, ctx.maxDepth) - if (!ctx.enterObject(obj)) + if (!ctx.enterContainer(obj)) Error.fail("Stackoverflow while materializing, possibly due to recursive value", obj.pos) - storePos(obj.pos) - obj.triggerAllAsserts(ctx.brokenAssertionLogic) - val keyNames = - if (ctx.sort) obj.sortedVisibleKeyNames - else obj.visibleKeyNames - val objVisitor = visitor.visitObject(keyNames.length, jsonableKeys = true, -1) - stack.push( - new Materializer.MaterializeObjFrame[T](objVisitor, keyNames, obj, ctx.sort, 0, null) - ) + var pushed = false + try { + storePos(obj.pos) + obj.triggerAllAsserts(ctx.brokenAssertionLogic) + val keyNames = + if (ctx.sort) obj.sortedVisibleKeyNames + else obj.visibleKeyNames + val objVisitor = visitor.visitObject(keyNames.length, jsonableKeys = true, -1) + stack.push( + new Materializer.MaterializeObjFrame[T](objVisitor, keyNames, obj, ctx.sort, 0, null) + ) + pushed = true + } finally { + if (!pushed) ctx.exitContainer(obj) + } } @inline private def pushArrFrame[T]( @@ -515,11 +541,27 @@ abstract class Materializer { stack: java.util.ArrayDeque[Materializer.MaterializeFrame], ctx: Materializer.MaterializeContext)(implicit evaluator: EvalScope): Unit = { checkDepth(xs.pos, stack.size, ctx.maxDepth) - storePos(xs.pos) - val arrVisitor = visitor.visitArray(xs.length, -1) - stack.push(new Materializer.MaterializeArrFrame[T](arrVisitor, xs, 0)) + if (!ctx.enterContainer(xs)) + Error.fail("Stackoverflow while materializing, possibly due to recursive value", xs.pos) + var pushed = false + try { + storePos(xs.pos) + val arrVisitor = visitor.visitArray(xs.length, -1) + stack.push(new Materializer.MaterializeArrFrame[T](arrVisitor, xs, 0)) + pushed = true + } finally { + if (!pushed) ctx.exitContainer(xs) + } } + @inline private def exitFrameContainer( + frame: Materializer.MaterializeFrame, + ctx: Materializer.MaterializeContext): Unit = + frame match { + case f: Materializer.MaterializeObjFrame[_] => ctx.exitContainer(f.obj) + case f: Materializer.MaterializeArrFrame[_] => ctx.exitContainer(f.arr) + } + // Feed a completed child result into the parent frame's visitor. @inline private def feedResult[T](parentFrame: Materializer.MaterializeFrame, result: T): Unit = parentFrame match { @@ -965,19 +1007,18 @@ object Materializer extends Materializer { val recursiveDepthLimit: Int, val maxDepth: Int) { - // Tracks objects currently being materialized on the current path (entry → exit). - // Used to detect cycles like `{ x: self }` early, before any significant output - // is written. Uses identity equality so structurally equal but distinct objects - // can still be materialized independently. - val visitedObjects: java.util.IdentityHashMap[Val.Obj, java.lang.Boolean] = - new java.util.IdentityHashMap[Val.Obj, java.lang.Boolean]() + // Tracks containers currently being materialized on the current path (entry → exit). + // Uses identity equality so shared but acyclic arrays/objects can still be materialized + // independently, while true cycles fail before recursive output grows without bound. + val visitedContainers: java.util.IdentityHashMap[Val, java.lang.Boolean] = + new java.util.IdentityHashMap[Val, java.lang.Boolean]() - /** Returns true if `obj` was NOT already being materialized (safe to proceed). */ - @inline def enterObject(obj: Val.Obj): Boolean = - visitedObjects.put(obj, java.lang.Boolean.TRUE) eq null + /** Returns true if `container` was NOT already being materialized (safe to proceed). */ + @inline def enterContainer(container: Val): Boolean = + visitedContainers.put(container, java.lang.Boolean.TRUE) eq null - @inline def exitObject(obj: Val.Obj): Unit = - visitedObjects.remove(obj) + @inline def exitContainer(container: Val): Unit = + visitedContainers.remove(container) } private[sjsonnet] object MaterializeContext { diff --git a/sjsonnet/test/resources/new_test_suite/error.lazy_nested_array_cycle.jsonnet b/sjsonnet/test/resources/new_test_suite/error.lazy_nested_array_cycle.jsonnet new file mode 100644 index 000000000..279a79884 --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.lazy_nested_array_cycle.jsonnet @@ -0,0 +1,3 @@ +local a = 1; +local b = local a = 2; local c = local a = 3; [a, c]; [a, b]; +[a, b] diff --git a/sjsonnet/test/resources/new_test_suite/error.lazy_nested_array_cycle.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/error.lazy_nested_array_cycle.jsonnet.golden new file mode 100644 index 000000000..6a3aba389 --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.lazy_nested_array_cycle.jsonnet.golden @@ -0,0 +1,2 @@ +sjsonnet.Error: Stackoverflow while materializing, possibly due to recursive value + at [].(error.lazy_nested_array_cycle.jsonnet:1:7) diff --git a/sjsonnet/test/resources/new_test_suite/materialize_shared_array_not_cycle.jsonnet b/sjsonnet/test/resources/new_test_suite/materialize_shared_array_not_cycle.jsonnet new file mode 100644 index 000000000..2a467e8bc --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/materialize_shared_array_not_cycle.jsonnet @@ -0,0 +1,6 @@ +local shared = [1, [2]]; +[ + shared, + { a: [shared], b: { c: shared } }, + shared, +] diff --git a/sjsonnet/test/resources/new_test_suite/materialize_shared_array_not_cycle.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/materialize_shared_array_not_cycle.jsonnet.golden new file mode 100644 index 000000000..1badaf07d --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/materialize_shared_array_not_cycle.jsonnet.golden @@ -0,0 +1,32 @@ +[ + [ + 1, + [ + 2 + ] + ], + { + "a": [ + [ + 1, + [ + 2 + ] + ] + ], + "b": { + "c": [ + 1, + [ + 2 + ] + ] + } + }, + [ + 1, + [ + 2 + ] + ] +]