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.

Author: paulk-asert

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}.
+     * <p>
+     * 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.
+     * <p>
+     * Only fields the closure <em>calls</em> 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;

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<V> extends Closure<V> {
         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
      */

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};
+    }
 }

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<Object> {
+        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