From acb2ff00de50f4e2ad7c9cfe5a9b5297aa64e8d7 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Wed, 8 Apr 2020 15:41:34 -0400 Subject: [PATCH] [JENKINS-61841] Limit the number of exceptions stored by CompositeIOException --- .../jenkins/util/io/CompositeIOException.java | 55 ++++++++++++++++++- .../java/jenkins/util/io/PathRemoverTest.java | 36 +++++++++++- 2 files changed, 88 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/jenkins/util/io/CompositeIOException.java b/core/src/main/java/jenkins/util/io/CompositeIOException.java index f6ddfa246c..fb325870f6 100644 --- a/core/src/main/java/jenkins/util/io/CompositeIOException.java +++ b/core/src/main/java/jenkins/util/io/CompositeIOException.java @@ -30,19 +30,52 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; import edu.umd.cs.findbugs.annotations.NonNull; import java.io.IOException; import java.io.UncheckedIOException; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @Restricted(NoExternalUse.class) public class CompositeIOException extends IOException { + private static final long serialVersionUID = 121943141387608148L; + + /** + * The maximum number of exceptions that can be reported by a single + * {@code CompositeIOException}. + *

+ * The number of exceptions is limited to avoid pathological cases where + * where a huge number of exceptions could lead to excessive memory usage. + * For example, if the number of exceptions was unlimited, a call to + * {@code Util.deleteRecursive} could fail with a + * {@code CompositeIOException} that contains an exception for every + * single file inside of the directory. + */ + public static final int EXCEPTION_LIMIT = 10; + private final List exceptions; + /** + * Construct a new {@code CompositeIOException} where the given list of + * exceptions are added as suppressed exceptions to the new exception. + *

+ * If the given list of exceptions is longer than {@link #EXCEPTION_LIMIT}, + * the list will be truncated to that length, and an exception whose message + * contains the number of exceptions that were removed will be added as a + * suppressed exception to the new exception. + */ public CompositeIOException(String message, @NonNull List exceptions) { super(message); - this.exceptions = exceptions; - exceptions.forEach(this::addSuppressed); + if (exceptions.size() > EXCEPTION_LIMIT) { + this.exceptions = new ArrayList<>(exceptions.subList(0, EXCEPTION_LIMIT)); + this.exceptions.add(new ExceptionLimitReached(exceptions.size() - EXCEPTION_LIMIT)); + } else { + this.exceptions = exceptions; + } + this.exceptions.forEach(this::addSuppressed); } + /** + * @see CompositeIOException(String, List) + */ public CompositeIOException(String message, IOException... exceptions) { this(message, Arrays.asList(exceptions)); } @@ -54,4 +87,22 @@ public class CompositeIOException extends IOException { public UncheckedIOException asUncheckedIOException() { return new UncheckedIOException(this); } + + /** + * An exception with no stack trace used to inform users that not all + * exceptions are being reported. + */ + private static class ExceptionLimitReached extends IOException { + private static final long serialVersionUID = 1L; + + public ExceptionLimitReached(int truncated) { + super(truncated + " additional exceptions were thrown"); + } + + @Override + public Throwable fillInStackTrace() { + // Do not create a strack trace for this exception. + return this; + } + } } diff --git a/core/src/test/java/jenkins/util/io/PathRemoverTest.java b/core/src/test/java/jenkins/util/io/PathRemoverTest.java index 860315f6fc..6a309cca68 100644 --- a/core/src/test/java/jenkins/util/io/PathRemoverTest.java +++ b/core/src/test/java/jenkins/util/io/PathRemoverTest.java @@ -51,10 +51,16 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; -import static org.junit.Assert.*; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.junit.Assume.assumeTrue; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; @@ -418,6 +424,34 @@ public class PathRemoverTest { assertFalse(d1.exists()); } + @Test + @Issue("JENKINS-55448") + public void testForceRemoveRecursive_TruncatesNumberOfExceptions() throws IOException { + assumeTrue(Functions.isWindows()); + final int maxExceptions = CompositeIOException.EXCEPTION_LIMIT; + final int lockedFiles = maxExceptions + 5; + final int totalFiles = lockedFiles + 5; + File dir = tmp.newFolder(); + File[] files = new File[totalFiles]; + for (int i = 0; i < totalFiles; i++) { + files[i] = new File(dir, "f" + i); + } + touchWithFileName(files); + for (int i = 0; i < lockedFiles; i++) { + locker.acquireLock(files[i]); + } + try { + PathRemover.newSimpleRemover().forceRemoveRecursive(dir.toPath()); + fail("Deletion should have failed"); + } catch (CompositeIOException e) { + assertThat(e.getSuppressed(), arrayWithSize(maxExceptions + 1)); + assertThat(e.getSuppressed()[maxExceptions].getMessage(), + containsString((lockedFiles + 1 - maxExceptions) + " additional exceptions")); + } + assertTrue(dir.exists()); + assertThat(dir.listFiles().length, equalTo(lockedFiles)); + } + private static void mkdirs(File... dirs) { for (File dir : dirs) { assertTrue("Could not mkdir " + dir, dir.mkdir());