all: Make IDE and CLI executions of JUnit fuzz tests more consistent

Previously, IDE executions of JUnit fuzz tests registered a
`findingHandler` in `FuzzTargetRunner` whereas CLI executions did not.
This lead to inconsistent behavior that was hard to reason about and a
lack of feature parity between the two modes (e.g. `--keep_going` was
only supported on the CLI). 

Instead, we now use a `findingHandler` to report the last, "fatal",
finding in structured form to `FuzzTestExecutor`, with all other
findings having their stack traces printed. `JUnitRunner` now handles
findings from lifecycle methods correctly, including for the exit code.

Author: fmeum

src/main/java/com/code_intelligence/jazzer/driver/FuzzTargetRunner.java

@@ -48,8 +48,9 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
+import java.util.Objects;
 import java.util.Set;
-import java.util.function.Predicate;
+import java.util.function.Consumer;
 import java.util.stream.Stream;
 import sun.misc.Unsafe;
 
@@ -118,7 +119,7 @@ public final class FuzzTargetRunner {
   private static final boolean useFuzzedDataProvider;
   private static final ArgumentsMutator mutator;
   private static final ReproducerTemplate reproducerTemplate;
-  private static Predicate<Throwable> findingHandler;
+  private static Consumer<Throwable> fatalFindingHandlerForJUnit;
 
   static {
     FuzzTargetHolder.FuzzTarget fuzzTarget = FuzzTargetHolder.fuzzTarget;
@@ -136,7 +137,6 @@ public final class FuzzTargetRunner {
     if (!useFuzzedDataProvider && IS_ANDROID) {
       Log.error("Android fuzz targets must use " + FuzzedDataProvider.class.getName());
       exit(1);
-      throw new IllegalStateException("Not reached");
     }
 
     Class<?> fuzzTargetClass = fuzzTarget.method.getDeclaringClass();
@@ -147,9 +147,8 @@ public final class FuzzTargetRunner {
     try {
       lifecycleMethodsInvoker.beforeFirstExecution();
     } catch (Throwable t) {
-      Log.finding(t);
+      Log.finding(ExceptionUtils.preprocessThrowable(t));
       exit(1);
-      throw new IllegalStateException("Not reached");
     }
 
     if (useExperimentalMutator) {
@@ -166,11 +165,7 @@ public final class FuzzTargetRunner {
       CoverageRecorder.updateCoveredIdsWithCoverageMap();
     }
 
-    // When running with a custom finding handler, such as from within JUnit, we can't reason about
-    // when the JVM shuts down and thus don't use shutdown handlers.
-    if (findingHandler == null) {
-      Runtime.getRuntime().addShutdownHook(new Thread(FuzzTargetRunner::shutdown));
-    }
+    Runtime.getRuntime().addShutdownHook(new Thread(FuzzTargetRunner::shutdown));
   }
 
   /** A test-only convenience wrapper around {@link #runOne(long, int)}. */
@@ -215,7 +210,6 @@ private static int runOne(long dataPtr, int dataLength) {
       data = copyToArray(dataPtr, dataLength);
       argument = data;
     }
-    Object fuzzTargetInstance;
     try {
       lifecycleMethodsInvoker.beforeEachExecution();
     } catch (Throwable uncaughtFinding) {
@@ -225,7 +219,7 @@ private static int runOne(long dataPtr, int dataLength) {
     // always enter the try block that calls afterEachExecution in finally.
     if (finding == null) {
       try {
-        fuzzTargetInstance = lifecycleMethodsInvoker.getTestClassInstance();
+        Object fuzzTargetInstance = lifecycleMethodsInvoker.getTestClassInstance();
         if (useExperimentalMutator) {
           // No need to detach as we are currently reading in the mutator state from bytes in every
           // iteration.
@@ -237,7 +231,7 @@ private static int runOne(long dataPtr, int dataLength) {
         }
       } catch (Throwable uncaughtFinding) {
         finding = uncaughtFinding;
-      } finally{
+      } finally {
         try {
           lifecycleMethodsInvoker.afterEachExecution();
         } catch (Throwable t) {
@@ -272,6 +266,12 @@ private static int runOne(long dataPtr, int dataLength) {
     if (finding == null || finding.getClass().getName().equals(OPENTEST4J_TEST_ABORTED_EXCEPTION)) {
       return LIBFUZZER_CONTINUE;
     }
+
+    // The user-provided fuzz target method has returned. Any further exits, e.g. due to uncaught
+    // exceptions, are on us and should not result in a "fuzz target exited" warning being printed
+    // by libFuzzer.
+    temporarilyDisableLibfuzzerExitHook();
+
     if (useHooks) {
       finding = ExceptionUtils.preprocessThrowable(finding);
     }
@@ -280,54 +280,44 @@ private static int runOne(long dataPtr, int dataLength) {
     if (emitDedupToken && !ignoredTokens.add(dedupToken)) {
       return LIBFUZZER_CONTINUE;
     }
-
-    if (findingHandler != null) {
-      // We still print the libFuzzer crashing input information, which also dumps the crashing
-      // input as a side effect.
-      printCrashingInput();
-      if (findingHandler.test(finding)) {
-        return LIBFUZZER_CONTINUE;
-      } else {
-        try {
-          // We have to call afterLastExecution here as we do not register the shutdown hook that
-          // would otherwise call it when findingHandler != null.
-          lifecycleMethodsInvoker.afterLastExecution();
-        } catch (Throwable t) {
-          // We already have a finding and do not know whether the fuzz target is in an expected
-          // state, so report this as a warning rather than an error or finding.
-          Log.warn("Failed to run @AfterAll or fuzzerTearDown methods", t);
-        }
-        return LIBFUZZER_RETURN_FROM_DRIVER;
-      }
+    boolean continueFuzzing =
+        emitDedupToken && Long.compareUnsigned(ignoredTokens.size(), keepGoing) < 0;
+    boolean isFuzzingFromCommandLine =
+        fatalFindingHandlerForJUnit == null || Opt.isJUnitAndCommandLine.get();
+    // In case of --keep_going, only the last finding is reported to JUnit as a Java object, all
+    // previous ones are merely printed. When fuzzing from the command line, we always print all
+    // findings.
+    if (isFuzzingFromCommandLine || continueFuzzing) {
+      Log.finding(finding);
+    }
+    if (fatalFindingHandlerForJUnit != null && !continueFuzzing) {
+      fatalFindingHandlerForJUnit.accept(finding);
     }
-
-    // The user-provided fuzz target method has returned. Any further exits are on us and should not
-    // result in a "fuzz target exited" warning being printed by libFuzzer.
-    temporarilyDisableLibfuzzerExitHook();
-
-    Log.finding(finding);
     if (emitDedupToken) {
       // Has to be printed to stdout as it is parsed by libFuzzer when minimizing a crash. It does
       // not necessarily have to appear at the beginning of a line.
       // https://github.com/llvm/llvm-project/blob/4c106c93eb68f8f9f201202677cd31e326c16823/compiler-rt/lib/fuzzer/FuzzerDriver.cpp#L342
       Log.structuredOutput(String.format(Locale.ROOT, "DEDUP_TOKEN: %016x", dedupToken));
     }
-    Log.println("== libFuzzer crashing input ==");
-    printCrashingInput();
+    if (isFuzzingFromCommandLine) {
+      // We emit this line for backwards compatibility when fuzzing on the CLI only.
+      Log.println("== libFuzzer crashing input ==");
+    }
+    printAndDumpCrashingInput();
+
     // dumpReproducer needs to be called after libFuzzer printed its final stats as otherwise it
     // would report incorrect coverage - the reproducer generation involved rerunning the fuzz
     // target.
     // It doesn't support @FuzzTest fuzz targets, but these come with an integrated regression test
     // that satisfies the same purpose.
     // It also doesn't support the experimental mutator yet as that requires implementing Java code
     // generation for mutators.
-    if (findingHandler == null && !useExperimentalMutator) {
+    if (fatalFindingHandlerForJUnit == null && !useExperimentalMutator) {
       dumpReproducer(data);
     }
 
-    if (!emitDedupToken || Long.compareUnsigned(ignoredTokens.size(), keepGoing) >= 0) {
-      // Reached the maximum amount of findings to keep going for, crash after shutdown.
-      if (!Opt.autofuzz.get().isEmpty() && Opt.dedup.get()) {
+    if (!continueFuzzing) {
+      if (!Opt.autofuzz.get().isEmpty() && emitDedupToken) {
         Log.println("");
         Log.info(
             String.format(
@@ -342,8 +332,15 @@ private static int runOne(long dataPtr, int dataLength) {
                         Opt.autofuzzIgnore.get().stream(), Stream.of(finding.getClass().getName()))
                     .collect(joining(","))));
       }
-      System.exit(JAZZER_FINDING_EXIT_CODE);
-      throw new IllegalStateException("Not reached");
+      if (fatalFindingHandlerForJUnit == null) {
+        // When running a legacy fuzzerTestOneInput test, exit now with the correct exit code.
+        // This will trigger the shutdown hook that runs fuzzerTearDown.
+        System.exit(JAZZER_FINDING_EXIT_CODE);
+      } else {
+        // When running within JUnit, pass control back to FuzzTestExecutor, which has received
+        // the finding via the handler.
+        return LIBFUZZER_RETURN_FROM_DRIVER;
+      }
     }
     return LIBFUZZER_CONTINUE;
   }
@@ -435,15 +432,8 @@ public static int startLibFuzzer(List<String> args) {
         args.stream().map(str -> str.getBytes(StandardCharsets.UTF_8)).toArray(byte[][]::new));
   }
 
-  /**
-   * Registers a custom handler for findings.
-   *
-   * @param findingHandler a consumer for the finding that returns true if the fuzzer should
-   *     continue fuzzing and false if it should return from {@link
-   *     FuzzTargetRunner#startLibFuzzer(List)}.
-   */
-  public static void registerFindingHandler(Predicate<Throwable> findingHandler) {
-    FuzzTargetRunner.findingHandler = findingHandler;
+  public static void registerFatalFindingHandlerForJUnit(Consumer<Throwable> findingHandler) {
+    FuzzTargetRunner.fatalFindingHandlerForJUnit = Objects.requireNonNull(findingHandler);
   }
 
   private static void shutdown() {
@@ -556,8 +546,8 @@ private static int startLibFuzzer(byte[][] args) {
    * Causes libFuzzer to write the current input to disk as a crashing input and emit some
    * information about it to stderr.
    */
-  public static void printCrashingInput() {
-    FuzzTargetRunnerNatives.printCrashingInput();
+  public static void printAndDumpCrashingInput() {
+    FuzzTargetRunnerNatives.printAndDumpCrashingInput();
   }
 
   /** Returns the debug string of the current mutator. If no mutator is used, returns null. */

src/main/java/com/code_intelligence/jazzer/driver/Opt.java

@@ -216,6 +216,10 @@ public final class Opt {
 
   // Internal options:
 
+  // Whether Jazzer is running a JUnit fuzz test from the command line.
+  public static final OptItem<Boolean> isJUnitAndCommandLine =
+      boolSetting("command_line", false, null);
+
   // Whether this is a subprocess created by libFuzzer's `-merge` mode.
   public static final OptItem<Boolean> mergeInner = boolSetting("merge_inner", false, null);
 

src/main/java/com/code_intelligence/jazzer/driver/junit/JUnitRunner.java

@@ -17,7 +17,6 @@
 package com.code_intelligence.jazzer.driver.junit;
 
 import static com.code_intelligence.jazzer.driver.Constants.JAZZER_FINDING_EXIT_CODE;
-import static com.code_intelligence.jazzer.driver.FuzzTargetRunner.printCrashingInput;
 import static org.junit.platform.engine.FilterResult.includedIf;
 import static org.junit.platform.engine.TestExecutionResult.Status.ABORTED;
 import static org.junit.platform.engine.TestExecutionResult.Status.FAILED;
@@ -30,6 +29,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
@@ -91,7 +91,7 @@ public static Optional<JUnitRunner> create(String testClassName, List<String> li
             // all fuzz test invocations are combined in a single JUnit test method execution.
             // https://junit.org/junit5/docs/current/user-guide/#writing-tests-declarative-timeouts-mode
             .configurationParameter("junit.jupiter.execution.timeout.mode", "disabled")
-            .configurationParameter("jazzer.internal.commandLine", "true")
+            .configurationParameter("jazzer.internal.command_line", "true")
             .configurationParameters(indexedArgs)
             .selectors(selectClass(testClassName))
             .filters(includeTags("jazzer"));
@@ -118,28 +118,30 @@ public static Optional<JUnitRunner> create(String testClassName, List<String> li
   }
 
   public int run() {
-    AtomicReference<TestExecutionResult> resultHolder =
-        new AtomicReference<>(TestExecutionResult.successful());
+    AtomicReference<TestExecutionResult> testResultHolder = new AtomicReference<>();
+    AtomicBoolean sawContainerFailure = new AtomicBoolean();
     launcher.execute(
         testPlan,
         new TestExecutionListener() {
           @Override
           public void executionFinished(
               TestIdentifier testIdentifier, TestExecutionResult testExecutionResult) {
-            // Lifecycle methods can fail too, which results in failed execution results on
-            // container
-            // nodes. We keep the last failing one with a stack trace. For tests, we also keep the
-            // stack
-            // traces of aborted tests so that we can show a warning. In JUnit Jupiter, tests and
-            // containers always fail with a throwable:
-            // https://github.com/junit-team/junit5/blob/ac31e9a7d58973db73496244dab4defe17ae563e/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ThrowableCollector.java#LL176C37-L176C37
-            if ((testIdentifier.isTest() && testExecutionResult.getThrowable().isPresent())
-                || testExecutionResult.getStatus() == FAILED) {
-              resultHolder.set(testExecutionResult);
-            }
-            if (testExecutionResult.getStatus() == FAILED
-                && testExecutionResult.getThrowable().isPresent()) {
-              resultHolder.set(testExecutionResult);
+            if (testIdentifier.isTest()) {
+              testResultHolder.set(testExecutionResult);
+            } else {
+              // Lifecycle methods can fail too, which results in failed execution results on
+              // container nodes. We emit all these failures as errors, not findings, since the
+              // lifecycle methods invoked by JUnit, which don't include @BeforeEach and
+              // @AfterEach executed during individual fuzz test executions, usually aren't
+              // reproducible with any given input (e.g. @AfterAll methods).
+              testExecutionResult
+                  .getThrowable()
+                  .map(ExceptionUtils::preprocessThrowable)
+                  .ifPresent(
+                      throwable -> {
+                        sawContainerFailure.set(true);
+                        Log.error(throwable);
+                      });
             }
           }
 
@@ -149,28 +151,37 @@ public void reportingEntryPublished(TestIdentifier testIdentifier, ReportEntry e
           }
         });
 
-    TestExecutionResult result = resultHolder.get();
+    TestExecutionResult result = testResultHolder.get();
+    if (result == null) {
+      // This can only happen if a test container failed, in which case we will have printed a
+      // stack trace.
+      Log.error("Failed to run fuzz test");
+      return 1;
+    }
     if (result.getStatus() != FAILED) {
-      // We do not generate a finding for Aborted tests (i.e. tests whose preconditions were not
+      // We do not generate a finding for aborted tests (i.e. tests whose preconditions were not
       // met) as such tests also wouldn't make a test run fail.
       if (result.getStatus() == ABORTED) {
         Log.warn("Fuzz test aborted", result.getThrowable().orElse(null));
       }
+      if (sawContainerFailure.get()) {
+        // A failure in a test container indicates a setup error, so we don't return the finding
+        // exit code in this case.
+        return 1;
+      }
       return 0;
     }
 
-    // Safe to unwrap as result is either TestExecutionResult.successful() (initial value) or has
-    // a throwable (set in the TestExecutionListener above).
+    // Safe to unwrap as in JUnit Jupiter, tests and containers always fail with a Throwable:
+    // https://github.com/junit-team/junit5/blob/ac31e9a7d58973db73496244dab4defe17ae563e/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ThrowableCollector.java#LL176C37-L176C37
     Throwable throwable = result.getThrowable().get();
     if (throwable instanceof ExitCodeException) {
-      // Jazzer found a regular finding and printed it, so just return the exit code.
+      // libFuzzer exited with a non-zero exit code, but Jazzer didn't produce a finding. Forward
+      // the exit code and assume that information has already been printed (e.g. a timeout).
       return ((ExitCodeException) throwable).exitCode;
     } else {
-      // Jazzer didn't report a finding, but an afterAll-type function threw an exception. Report it
-      // as a finding, cleaning up the stack trace.
-      Log.finding(ExceptionUtils.preprocessThrowable(throwable));
-      Log.println("== libFuzzer crashing input ==");
-      printCrashingInput();
+      // Non-fatal findings and exceptions in containers have already been printed, the fatal
+      // finding is passed to JUnit as the test result.
       return JAZZER_FINDING_EXIT_CODE;
     }
   }

src/main/java/com/code_intelligence/jazzer/junit/FuzzTestExecutor.java

@@ -361,17 +361,9 @@ public Optional<Throwable> execute(
               JUnitLifecycleMethodsInvoker.of(extensionContext, lifecycle));
     }
 
-    // Only register a finding handler in case the fuzz test is executed by JUnit.
-    // It short-circuits the handling in FuzzTargetRunner and prevents settings
-    // like --keep_going.
     AtomicReference<Throwable> atomicFinding = new AtomicReference<>();
-    if (!isRunFromCommandLine) {
-      FuzzTargetRunner.registerFindingHandler(
-          t -> {
-            atomicFinding.set(t);
-            return false;
-          });
-    }
+    // Non-fatal findings (with --keep_going) are logged by FuzzTargetRunner.
+    FuzzTargetRunner.registerFatalFindingHandlerForJUnit(atomicFinding::set);
 
     int exitCode = FuzzTargetRunner.startLibFuzzer(libFuzzerArgs);
     javaSeedsDir.ifPresent(FuzzTestExecutor::deleteJavaSeedsDir);

src/main/java/com/code_intelligence/jazzer/junit/Utils.java

@@ -236,7 +236,7 @@ static boolean isFuzzing(ExtensionContext extensionContext) {
 
   static boolean runFromCommandLine(ExtensionContext extensionContext) {
     return extensionContext
-        .getConfigurationParameter("jazzer.internal.commandLine")
+        .getConfigurationParameter("jazzer.internal.command_line")
         .map(Boolean::parseBoolean)
         .orElse(false);
   }

src/main/java/com/code_intelligence/jazzer/runtime/FuzzTargetRunnerNatives.java

@@ -35,7 +35,7 @@ public class FuzzTargetRunnerNatives {
   public static native int startLibFuzzer(
       byte[][] args, Class<?> runner, boolean useExperimentalMutator);
 
-  public static native void printCrashingInput();
+  public static native void printAndDumpCrashingInput();
 
   public static native void temporarilyDisableLibfuzzerExitHook();
 }

src/main/native/com/code_intelligence/jazzer/driver/fuzz_target_runner.cpp

@@ -192,7 +192,7 @@ Java_com_code_1intelligence_jazzer_runtime_FuzzTargetRunnerNatives_startLibFuzze
 }
 
 [[maybe_unused]] void
-Java_com_code_1intelligence_jazzer_runtime_FuzzTargetRunnerNatives_printCrashingInput(
+Java_com_code_1intelligence_jazzer_runtime_FuzzTargetRunnerNatives_printAndDumpCrashingInput(
     JNIEnv *, jclass) {
   if (gLibfuzzerPrintCrashingInput == nullptr) {
     std::cerr << "<not available>" << std::endl;