From cb2864de0b107490fd2ed3fd6836e80dc53fe000 Mon Sep 17 00:00:00 2001 From: Paul King Date: Tue, 18 Aug 2026 20:16:04 +1000 Subject: [PATCH] GROOVY-12276: Extend the closure cycle check to the closures a wrapper calls Closure.checkForReferenceCycle walks owner, delegate and thisObject, which are the fields a closure dispatches through. A closure which wraps another and invokes it recurses through its own field instead, so a forged graph cycling there passed the check and then exhausted the stack on invocation. Measured before the change, building each cycle by reflection, serializing and reading it back: CurriedClosure, cycle via owner rejected, InvalidObjectException ComposedClosure, cycle via first/second accepted, then StackOverflowError TrampolineClosure, cycle via original accepted, then StackOverflowError ComposedClosure was the more surprising of the two: it already opts into the check from its readResolve, so the check ran and had nothing to say about the closure's own recursion. TrampolineClosure had no readResolve at all. Add Closure.additionalReferences for a subclass to declare the closures it calls through, and walk those as well. ComposedClosure declares first and second; TrampolineClosure declares original and gains the readResolve it was missing. Only fields a closure calls belong there, which is why the walk is not simply made reflective over every Closure-valued field: a captured closure is not a recursion edge, and treating it as one would reject graphs that invoke perfectly well. The existing ComposedClosure cycle test forges its cycle through owner and delegate, so it exercised the walk that already worked; the new tests forge through the wrapped fields, and fail without this change. --- src/main/java/groovy/lang/Closure.java | 35 +++++++- .../java/groovy/lang/TrampolineClosure.java | 20 +++++ .../groovy/runtime/ComposedClosure.java | 9 ++ .../lang/ClosureSerializationCycleTest.groovy | 83 +++++++++++++++++++ 4 files changed, 145 insertions(+), 2 deletions(-) diff --git a/src/main/java/groovy/lang/Closure.java b/src/main/java/groovy/lang/Closure.java index 8f0f4d611f9..7a1c531f006 100644 --- a/src/main/java/groovy/lang/Closure.java +++ b/src/main/java/groovy/lang/Closure.java @@ -56,6 +56,7 @@ import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.function.BiConsumer; import java.util.function.Function; @@ -1466,11 +1467,14 @@ protected static void checkForReferenceCycle(final Closure root) throws Inval } grey.add(node); stack.push(new Marker(node)); - for (final Object link : new Object[]{node.owner, node.delegate, node.thisObject}) { + final Object[] extra = Objects.requireNonNull(node.additionalReferences(), + () -> node.getClass().getName() + ".additionalReferences() must not return null"); + final Object[] links = concat(new Object[]{node.owner, node.delegate, node.thisObject}, extra); + for (final Object link : links) { if (link instanceof Closure child) { if (grey.contains(child)) { throw new InvalidObjectException( - "Closure owner/delegate/thisObject references form a cycle; refusing to deserialize"); + "Closure references form a cycle; refusing to deserialize"); } if (!black.contains(child)) { stack.push(child); @@ -1480,6 +1484,33 @@ protected static void checkForReferenceCycle(final Closure root) throws Inval } } + private static Object[] concat(final Object[] first, final Object[] second) { + if (second.length == 0) return first; + final Object[] joined = new Object[first.length + second.length]; + System.arraycopy(first, 0, joined, 0, first.length); + System.arraycopy(second, 0, joined, first.length, second.length); + return joined; + } + + /** + * The closures this one calls through, beyond {@code owner}, {@code delegate} and + * {@code thisObject}, for the purposes of {@link #checkForReferenceCycle}. + *

+ * A closure which wraps another and invokes it, as + * {@link org.codehaus.groovy.runtime.ComposedClosure} and {@code TrampolineClosure} do, + * recurses through its own fields rather than through the three the cycle check walks by + * default, so a forged graph would escape the check unless it declares them here. + *

+ * Only fields the closure calls belong here. A merely captured closure is not a + * recursion edge, and declaring one would reject graphs which invoke perfectly well. + * + * @return the additional closures reached when this one is invoked; never {@code null} + * @since 6.0.0 + */ + protected Object[] additionalReferences() { + return EMPTY_OBJECT_ARRAY; + } + /** Sentinel pushed below a node's children during the {@link #checkForReferenceCycle} cycle check. */ private static final class Marker { final Closure closure; diff --git a/src/main/java/groovy/lang/TrampolineClosure.java b/src/main/java/groovy/lang/TrampolineClosure.java index 2253b61e03c..e0d13616ee7 100644 --- a/src/main/java/groovy/lang/TrampolineClosure.java +++ b/src/main/java/groovy/lang/TrampolineClosure.java @@ -18,6 +18,7 @@ */ package groovy.lang; +import java.io.ObjectStreamException; import java.io.Serial; /** @@ -43,6 +44,25 @@ final class TrampolineClosure extends Closure { this.original = original; } + /** + * A trampoline calls through the closure it wraps, so {@code original} is a recursion edge + * in addition to the three the cycle check walks by default. + */ + @Override + protected Object[] additionalReferences() { + return new Object[]{original}; + } + + /** + * Rejects a deserialized trampoline whose references form a cycle, which would otherwise + * recurse indefinitely on invocation. See {@link Closure#checkForReferenceCycle}. + */ + @Serial + private Object readResolve() throws ObjectStreamException { + Closure.checkForReferenceCycle(this); + return this; + } + /** * Delegates to the wrapped closure */ diff --git a/src/main/java/org/codehaus/groovy/runtime/ComposedClosure.java b/src/main/java/org/codehaus/groovy/runtime/ComposedClosure.java index cb795221ce0..8359e708d46 100644 --- a/src/main/java/org/codehaus/groovy/runtime/ComposedClosure.java +++ b/src/main/java/org/codehaus/groovy/runtime/ComposedClosure.java @@ -129,4 +129,13 @@ private Object readResolve() throws ObjectStreamException { Closure.checkForReferenceCycle(this); return this; } + + /** + * A composed closure calls through {@code first} and then {@code second}, so those are + * recursion edges in addition to the three the cycle check walks by default. + */ + @Override + protected Object[] additionalReferences() { + return new Object[]{first, second}; + } } diff --git a/src/test/groovy/groovy/lang/ClosureSerializationCycleTest.groovy b/src/test/groovy/groovy/lang/ClosureSerializationCycleTest.groovy index e8fa73f9dc8..21ec127780d 100644 --- a/src/test/groovy/groovy/lang/ClosureSerializationCycleTest.groovy +++ b/src/test/groovy/groovy/lang/ClosureSerializationCycleTest.groovy @@ -45,6 +45,22 @@ final class ClosureSerializationCycleTest { field.set(target, value) } + /** Sets a field declared anywhere in the target's hierarchy, not just on Closure. */ + private static void setDeclaredField(Object target, String name, Object value) { + Class c = target.getClass() + while (c != null) { + try { + def field = c.getDeclaredField(name) + field.accessible = true + field.set(target, value) + return + } catch (NoSuchFieldException ignored) { + c = c.superclass + } + } + throw new NoSuchFieldException(name) + } + private static byte[] serialize(Object obj) { def out = new ByteArrayOutputStream() out.withObjectOutputStream { it.writeObject(obj) } @@ -96,6 +112,41 @@ final class ClosureSerializationCycleTest { assert err.message.contains('cycle') } + @Test + void testComposedClosureCycleThroughItsOwnFieldsRejected() { + // The wrapped closures a ComposedClosure calls through are its own first/second fields, + // not owner/delegate/thisObject, so a cycle formed there is a different graph from the + // one above and would otherwise pass the check and then recurse on invocation. + byte[] bytes = Holder.serializeComposedCyclicThroughWrappedFields() + def err = shouldFail(InvalidObjectException) { deserialize(bytes) } + assert err.message.contains('cycle') + } + + @Test + void testTrampolineClosureCycleRejected() { + // TrampolineClosure calls through its original field, and had no readResolve at all. + byte[] bytes = Holder.serializeCyclicTrampoline() + def err = shouldFail(InvalidObjectException) { deserialize(bytes) } + assert err.message.contains('cycle') + } + + @Test + void testNullAdditionalReferencesFailsClosed() { + // a broken override must abort deserialization with a diagnostic naming the subclass, + // not silently drop its recursion edges from the cycle check + byte[] bytes = Holder.serializeCurriedOverNullRefsClosure() + def err = shouldFail(NullPointerException) { deserialize(bytes) } + assert err.message.contains('NullRefsClosure') + assert err.message.contains('additionalReferences() must not return null') + } + + @Test + void testLegitimateTrampolineRoundTrips() { + byte[] bytes = Holder.serializeTrampoline() + def t = deserialize(bytes) + assert t.call(5) == 10 + } + @Test void testLegitimateComposedClosureRoundTrips() { byte[] bytes = Holder.serializeComposed() @@ -120,6 +171,16 @@ final class ClosureSerializationCycleTest { assert c.call('y') == 'x-y' } + /** A buggy subclass whose override violates the never-null contract of additionalReferences. */ + static final class NullRefsClosure extends Closure { + NullRefsClosure(Object owner) { super(owner) } + + @Override + protected Object[] additionalReferences() { null } + + Object doCall(Object x) { x } + } + static class Holder { static byte[] serializeGreeter() { serialize({ p -> "Hello, $p" }) @@ -136,6 +197,28 @@ final class ClosureSerializationCycleTest { serialize(inc >> twice) } + static byte[] serializeComposedCyclicThroughWrappedFields() { + def composed = ({ x -> x } >> { y -> y }) + setDeclaredField(composed, 'first', composed) + setDeclaredField(composed, 'second', composed) + serialize(composed) + } + + static byte[] serializeTrampoline() { + def base = { a -> a * 2 } + serialize(base.trampoline()) + } + + static byte[] serializeCyclicTrampoline() { + def trampoline = { x -> x }.trampoline() + setDeclaredField(trampoline, 'original', trampoline) + serialize(trampoline) + } + + static byte[] serializeCurriedOverNullRefsClosure() { + serialize(new CurriedClosure(new NullRefsClosure('s'), 'x')) + } + static byte[] serializeCyclicComposed() { // built in a static context so the wrapped closures' owner is the (serializable) // class rather than the test instance, then the owner/delegate are made self-referential