Merge pull request #1067 from jglick/refersToCycle

`StackOverflowError` in `CpsStepContext.refersTo`

Author: jglick

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java

@@ -58,7 +58,10 @@
 import java.io.IOException;
 import java.io.Serializable;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.IdentityHashMap;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
@@ -346,14 +349,14 @@ private void completed(@NonNull Outcome newOutcome) {
                     LOGGER.log(Level.FINE, "earlier success: {0}", outcome.getNormal());
                 }
             }
-            if (failure != null && earlierFailure != null && !refersTo(failure, earlierFailure)) {
+            if (failure != null && earlierFailure != null && !refersTo(failure, earlierFailure, Collections.newSetFromMap(new IdentityHashMap<>()))) {
                 earlierFailure.addSuppressed(failure);
             }
         }
     }
 
-    private static boolean refersTo(Throwable t1, Throwable t2) {
-        return t1 == t2 || t1.getCause() != null && refersTo(t1.getCause(), t2) || Stream.of(t1.getSuppressed()).anyMatch(t3 -> refersTo(t3, t2));
+    private static boolean refersTo(Throwable t1, Throwable t2, Set<Throwable> checked) {
+        return checked.add(t1) && (t1 == t2 || t1.getCause() != null && refersTo(t1.getCause(), t2, checked) || Stream.of(t1.getSuppressed()).anyMatch(t3 -> refersTo(t3, t2, checked)));
     }
 
     /**

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java

@@ -33,6 +33,7 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.not;
+import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
 
 public class CpsStepContextTest {
     @Rule
@@ -82,19 +83,6 @@ public void executionStartExceptionNotLeakClosures() throws Exception {
         assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
     }
 
-    @Issue("JENKINS-75067")
-    @Test
-    public void executionWithBodyRunningSyncNotLeakClosures() throws Exception {
-        logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
-        WorkflowJob job = r.createProject(WorkflowJob.class, "p");
-        job.setDefinition(new CpsFlowDefinition("def r = passthrough {}; echo r", true));
-
-        WorkflowRun build = r.buildAndAssertSuccess(job);
-        r.assertLogContains("hooray", build);
-        assertThat(ClosureCounter.get().closureCount, equalTo(0));
-        assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
-    }
-
     public static class BadBlockStep extends Step {
 
         @DataBoundConstructor
@@ -127,6 +115,19 @@ public boolean takesImplicitBlockArgument() {
         }
     }
 
+    @Issue("JENKINS-75067")
+    @Test
+    public void executionWithBodyRunningSyncNotLeakClosures() throws Exception {
+        logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
+        WorkflowJob job = r.createProject(WorkflowJob.class, "p");
+        job.setDefinition(new CpsFlowDefinition("def r = passthrough {}; echo r", true));
+
+        WorkflowRun build = r.buildAndAssertSuccess(job);
+        r.assertLogContains("hooray", build);
+        assertThat(ClosureCounter.get().closureCount, equalTo(0));
+        assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
+    }
+
     public static class PassthroughStep extends Step {
 
         @DataBoundConstructor
@@ -181,4 +182,34 @@ static ClosureCounter get() {
             return ExtensionList.lookupSingleton(ClosureCounter.class);
         }
     }
+
+    @Test public void refersToCycle() throws Exception {
+        logger.record(CpsStepContext.class, Level.ALL);
+        var p = r.createProject(WorkflowJob.class, "p");
+        p.setDefinition(new CpsFlowDefinition("refersToCycle()", true));
+        r.assertLogNotContains("StackOverflowError", r.buildAndAssertStatus(Result.UNSTABLE, p));
+    }
+    public static final class RefersToCycleStep extends Step {
+        @DataBoundConstructor public RefersToCycleStep() {}
+        @Override public StepExecution start(StepContext context) throws Exception {
+            return StepExecutions.synchronousNonBlockingVoid(context, ctx -> {
+                var t1 = new IllegalStateException("extra");
+                var t2 = new FlowInterruptedException(Result.UNSTABLE, false);
+                var t3 = new IllegalArgumentException("whatever");
+                t1.addSuppressed(t3);
+                t3.addSuppressed(t1);
+                ctx.onFailure(t2);
+                ctx.onFailure(t1);
+            });
+        }
+        @TestExtension("refersToCycle") public static final class DescriptorImpl extends StepDescriptor {
+            @Override public String getFunctionName() {
+                return "refersToCycle";
+            }
+            @Override public Set<? extends Class<?>> getRequiredContext() {
+                return Set.of();
+            }
+        }
+    }
+
 }