Merge pull request #286 from dwnusbaum/isUnserializableException

Prevent StackOverflowError in ErrorAction.isUnserializableError due to cyclic errors

src/main/java/org/jenkinsci/plugins/workflow/actions/ErrorAction.java

@@ -54,7 +54,7 @@ public class ErrorAction implements PersistentAction {
     private final @NonNull Throwable error;
 
     public ErrorAction(@NonNull Throwable error) {
-        if (isUnserializableException(error)) {
+        if (isUnserializableException(error, new HashSet<>())) {
             error = new ProxyException(error);
         } else if (error != null) {
             try {
@@ -75,8 +75,8 @@ public ErrorAction(@NonNull Throwable error) {
      * Some exceptions don't serialize properly. If so, we need to replace that with
      * an equivalent that captures the same details but serializes nicely.
      */
-    private boolean isUnserializableException(@CheckForNull Throwable error) {
-        if (error == null) {
+    private boolean isUnserializableException(@CheckForNull Throwable error, Set<Throwable> visited) {
+        if (error == null || !visited.add(error)) {
             return false;
         }
         // If the exception was defined in a Pipeline script, we don't want to serialize it
@@ -95,11 +95,11 @@ private boolean isUnserializableException(@CheckForNull Throwable error) {
         if (error instanceof MultipleCompilationErrorsException || error instanceof MissingMethodException) {
             return true;
         }
-        if (isUnserializableException(error.getCause())) {
+        if (isUnserializableException(error.getCause(), visited)) {
             return true;
         }
         for (Throwable t : error.getSuppressed()) {
-            if (isUnserializableException(t)) {
+            if (isUnserializableException(t, visited)) {
                 return true;
             }
         }

src/test/java/org/jenkinsci/plugins/workflow/actions/ErrorActionTest.java

@@ -26,6 +26,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -46,6 +47,7 @@
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsSessionRule;
 
+import groovy.lang.MissingMethodException;
 import hudson.Functions;
 import hudson.model.Result;
 import hudson.remoting.ProxyException;
@@ -227,4 +229,20 @@ public static class X extends Exception {
         });
     }
 
+    @Test public void cyclicErrorsAreSupported() throws Throwable {
+        Exception cyclic1 = new Exception();
+        Exception cyclic2 = new Exception(cyclic1);
+        cyclic1.initCause(cyclic2);
+        assertNotNull(new ErrorAction(cyclic1));
+        assertNotNull(new ErrorAction(cyclic2));
+    }
+
+    @Ignore("TODO: broken until https://github.com/jenkinsci/remoting/pull/645 is picked up")
+    @Test public void unserializableCyclicErrorsAreSupported() throws Throwable {
+        Exception unserializable = new MissingMethodException("thisMethodDoesNotExist", String.class, new Object[0]);
+        Exception cyclic = new Exception(unserializable);
+        unserializable.initCause(cyclic);
+        assertNotNull(new ErrorAction(unserializable));
+        assertNotNull(new ErrorAction(cyclic));
+    }
 }