GROOVY-12276: Extend the closure cycle check to the closures a wrappe…

src/main/java/groovy/lang/Closure.java

@@ -1466,11 +1466,13 @@ 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[] links = concat(new Object[]{node.owner, node.delegate, node.thisObject},
+                    node.additionalReferences());
+            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 +1482,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,31 @@ 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 testLegitimateTrampolineRoundTrips() {
+        byte[] bytes = Holder.serializeTrampoline()
+        def t = deserialize(bytes)
+        assert t.call(5) == 10
+    }
+
     @Test
     void testLegitimateComposedClosureRoundTrips() {
         byte[] bytes = Holder.serializeComposed()
@@ -136,6 +177,24 @@ 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[] 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