fix fuzzer setup logic

contrib/oss-fuzz/README

@@ -166,6 +166,63 @@ modifies the checkout.  The git checkout ships no `configure`, so
 chain and therefore cannot be trusted to return a failure status,
 `build.sh` checks for the product and falls back to `autoreconf -fi`.
 
+Note that out-of-tree does *not* mean the checkout can be a tree you
+have already configured in place: an in-tree `config.status` makes any
+subsequent out-of-tree `configure` stop with "source directory already
+configured; run \"make distclean\" there first".  Either `make
+distclean` the checkout or -- better, and closer to what OSS-Fuzz
+actually does -- point `$MHD_SRC` at a pristine clone:
+
+    git clone --shared /path/to/libmicrohttpd /tmp/mhd-fuzz-src
+
+
+### $CFLAGS / $CXXFLAGS on a local run
+
+Under OSS-Fuzz these are always exported by the base-builder image and
+`build.sh` uses them verbatim.  When they are unset -- i.e. only on a
+local run -- `build.sh` derives them from `$SANITIZER`.  Two flags are
+what make such a run a real fuzzing run rather than a build that merely
+succeeds:
+
+  * `-fsanitize=fuzzer-no-link` installs libFuzzer's coverage
+    instrumentation into every translation unit of the library.  Without
+    it there is no feedback signal at all: libFuzzer degenerates to blind
+    random generation, `cov:` never moves and the corpus never grows.
+    This is the single easiest thing to leave out, and nothing about the
+    resulting build looks wrong.
+  * a sanitizer, because libFuzzer on its own only notices crashes the
+    kernel delivers.  (MHD's own oracles -- the `MHD_set_panic_func()`
+    tripwire and `mhd_assert()` -- do fire without one, which is why
+    `SANITIZER=none` is still worth something.)
+
+The `address` default folds UBSan into the ASan build, with
+`-fno-sanitize-recover=undefined` so that UB actually kills the process
+instead of printing and continuing.  That differs deliberately from
+OSS-Fuzz, which runs `address` and `undefined` as two separate
+campaigns: it has unlimited machine time and wants each report
+attributed to one sanitizer, whereas a local run has an afternoon and is
+better off with two oracles per CPU-hour.  Pass `SANITIZER=undefined`
+explicitly to get the split OSS-Fuzz shape.
+
+`SANITIZER=memory` is accepted but is only meaningful inside the
+OSS-Fuzz image, where the C library is instrumented too; on a distro
+toolchain it reports uninitialised reads in libc frames.
+
+A complete local run then needs nothing but clang and the compiler-rt
+runtimes (on Debian: `clang`, `libclang-rt-dev`, and `llvm` for
+`llvm-symbolizer`, without which every trace is bare addresses):
+
+    git clone --shared . /tmp/mhd-fuzz-src
+    WORK=/tmp/mhd-fuzz-work OUT=/tmp/mhd-fuzz-out MHD_SRC=/tmp/mhd-fuzz-src \
+      /tmp/mhd-fuzz-src/contrib/oss-fuzz/build.sh
+    mkdir /tmp/c && unzip -q /tmp/mhd-fuzz-out/fuzz_request_seed_corpus.zip -d /tmp/c
+    /tmp/mhd-fuzz-out/fuzz_request /tmp/c \
+        -dict=/tmp/mhd-fuzz-out/fuzz_request.dict -max_len=8192 \
+        -jobs=8 -workers=8 -max_total_time=3600
+
+`build.sh` prints the resolved `CC`, `CXX`, `CFLAGS` and `CXXFLAGS` in
+its banner; check there first if a local run finds nothing.
+
 
 -------------------------------------------------------------------
 4. Building and running locally with infra/helper.py

contrib/oss-fuzz/build.sh

@@ -32,11 +32,101 @@ WORK="${WORK:-${SRC}/work}"
 OUT="${OUT:-${SRC}/out}"
 CC="${CC:-clang}"
 CXX="${CXX:-clang++}"
-CFLAGS="${CFLAGS:--O1 -fno-omit-frame-pointer -gline-tables-only}"
-CXXFLAGS="${CXXFLAGS:-${CFLAGS}}"
 LIB_FUZZING_ENGINE="${LIB_FUZZING_ENGINE:--fsanitize=fuzzer}"
 SANITIZER="${SANITIZER:-address}"
 
+# --- $CFLAGS / $CXXFLAGS -----------------------------------------------------
+#
+# Under OSS-Fuzz $CFLAGS and $CXXFLAGS are always exported by the
+# base-builder image and MUST be used verbatim, so everything below is
+# dead code there: it fires only when the variable is unset, i.e. only on
+# a local run.
+#
+# The point of the defaults is that a local run must be a *real* fuzzing
+# run.  Two flags are what make it one, and leaving either out produces a
+# build that looks fine and finds nothing:
+#
+#   -fsanitize=fuzzer-no-link
+#         installs libFuzzer's coverage instrumentation (SanitizerCoverage
+#         trace-pc-guard + the comparison hooks) in every translation unit
+#         of the library.  Without it libFuzzer gets no feedback signal at
+#         all and degenerates into blind random input generation --
+#         `cov:` stays flat and the corpus never grows.  "-no-link" is the
+#         compile-time half; $LIB_FUZZING_ENGINE supplies the driver at
+#         link time.
+#   a sanitizer
+#         libFuzzer by itself only notices a crash the kernel delivers.
+#         ASan/UBSan are what turn a silently-tolerated overflow into a
+#         report.  Note that MHD's own oracles -- the
+#         MHD_set_panic_func() tripwire and mhd_assert(), enabled by the
+#         --enable-asserts below -- work without any sanitizer, which is
+#         why SANITIZER=none is still worth something.
+#
+# The mapping below mirrors what OSS-Fuzz's own helper passes for each
+# $SANITIZER value, with one deliberate difference in the "address" case,
+# noted there.
+if [ -z "${CFLAGS:-}" ]; then
+  # -O1                     OSS-Fuzz's optimisation level for fuzz builds:
+  #                         fast enough to get exec/s, low enough that
+  #                         inlining does not destroy the stack traces.
+  # -fno-omit-frame-pointer needed for usable ASan/libFuzzer backtraces.
+  # -gline-tables-only      just enough debug info to symbolize; a full -g
+  #                         would multiply build time and object size.
+  _mhd_base_cflags="-O1 -fno-omit-frame-pointer -gline-tables-only"
+
+  case "${SANITIZER}" in
+    address)
+      # OSS-Fuzz builds "address" and "undefined" as two separate
+      # campaigns, because it has unlimited machine time and wants each
+      # report attributed to one sanitizer.  A local run has an afternoon
+      # at most, so the default folds UBSan into the ASan build: two
+      # oracles per CPU-hour instead of one, at a few percent of speed.
+      # Set SANITIZER=undefined explicitly for the split OSS-Fuzz shape.
+      #
+      # -fno-sanitize-recover=undefined is essential: by default UBSan
+      # *prints* and continues, and libFuzzer only records a finding for a
+      # process that dies.  Without it UB scrolls past and the run is
+      # reported clean.
+      _mhd_san_cflags="-fsanitize=address,undefined"
+      _mhd_san_cflags="${_mhd_san_cflags} -fsanitize-address-use-after-scope"
+      _mhd_san_cflags="${_mhd_san_cflags} -fno-sanitize-recover=undefined"
+      ;;
+    undefined)
+      _mhd_san_cflags="-fsanitize=undefined -fno-sanitize-recover=undefined"
+      ;;
+    memory)
+      # MSan reports uninitialised reads from *any* uninstrumented code it
+      # links against, so this is only usable when the C library is
+      # instrumented too -- true inside the OSS-Fuzz image, essentially
+      # never true on a distro toolchain.  Expect false positives in libc
+      # frames locally; use the OSS-Fuzz container for a real MSan run.
+      _mhd_san_cflags="-fsanitize=memory -fsanitize-memory-track-origins"
+      ;;
+    coverage)
+      # A coverage build is not a fuzzing build: it replays an existing
+      # corpus to produce a report, so no sanitizer and no libFuzzer
+      # coverage instrumentation.
+      _mhd_san_cflags="-fprofile-instr-generate -fcoverage-mapping"
+      ;;
+    none | "")
+      _mhd_san_cflags=""
+      ;;
+    *)
+      echo "ERROR: unknown SANITIZER='${SANITIZER}'" >&2
+      echo "       expected: address | undefined | memory | coverage | none" >&2
+      exit 1
+      ;;
+  esac
+
+  if [ "${SANITIZER}" = "coverage" ]; then
+    CFLAGS="${_mhd_base_cflags} ${_mhd_san_cflags}"
+  else
+    CFLAGS="${_mhd_base_cflags} ${_mhd_san_cflags} -fsanitize=fuzzer-no-link"
+  fi
+  unset _mhd_base_cflags _mhd_san_cflags
+fi
+CXXFLAGS="${CXXFLAGS:-${CFLAGS}}"
+
 # Directory holding the libmicrohttpd sources.  OSS-Fuzz clones them to
 # $SRC/libmicrohttpd (see Dockerfile); allow an override for local runs.
 MHD_SRC="${MHD_SRC:-${SRC}/libmicrohttpd}"
@@ -56,6 +146,9 @@ echo "    BUILD              = ${BUILD}"
 echo "    OUT                = ${OUT}"
 echo "    SANITIZER          = ${SANITIZER}"
 echo "    LIB_FUZZING_ENGINE = ${LIB_FUZZING_ENGINE}"
+echo "    CC / CXX           = ${CC} / ${CXX}"
+echo "    CFLAGS             = ${CFLAGS}"
+echo "    CXXFLAGS           = ${CXXFLAGS}"
 
 # ---------------------------------------------------------------------------
 # 1. Bootstrap (the git checkout ships no 'configure')

src/fuzz/BUILD-INTEGRATION.md

@@ -217,11 +217,41 @@ Two consequences for anyone editing `src/fuzz/`:
   `main()` may be `#ifndef FUZZ_NO_MAIN`; a harness whose fuzz target is
   itself conditional silently produces an empty OSS-Fuzz binary.
 * **Anything the fuzz target needs must not live inside the
-  `#ifndef FUZZ_NO_MAIN` block of `fuzz_common.h`.**  Today the split is
-  right: the PRNG, the crash bookkeeping and `fuzz_report_finding()` are
+  `#ifndef FUZZ_NO_MAIN` block of `fuzz_common.h`.**  The PRNG, the crash
+  bookkeeping, `fuzz_report_finding()` and `fuzz_ignore_sigpipe()` are
   outside it; the generator loop, the mutator, the corpus walker and
   `main()` are inside.
 
+  This is easy to get wrong and the failure is silent, so it is worth
+  spelling out how it already bit us once.  `signal (SIGPIPE, SIG_IGN)`
+  used to sit in `fuzz_install_handlers()`, i.e. inside the block.  Under
+  the built-in driver everything looked perfect; under `-DFUZZ_NO_MAIN`
+  the call vanished, and since `fuzz_request` writes into a socketpair
+  whose peer MHD closes on any input that terminates the connection
+  early, the process took a SIGPIPE and died after a few dozen
+  executions.  libFuzzer does not intercept SIGPIPE (there is no
+  `-handle_sigpipe`; see `-help=1`), so there was no stack trace, no
+  artifact and no crash report -- the target just stopped, which reads
+  exactly like a clean run that found nothing.  It is now
+  `fuzz_ignore_sigpipe()`, called from both the driver and
+  `LLVMFuzzerTestOneInput()`.
+
+  The lesson generalises: a bug in this split cannot be caught by
+  `make -C src/fuzz check`, because that path always defines `main()`.
+  After touching `fuzz_common.h`, build the OSS-Fuzz way as well and
+  confirm the target still runs -- `contrib/oss-fuzz/build.sh` works
+  standalone on any machine with clang and `libclang-rt-dev`:
+
+  ```sh
+  git clone --shared . /tmp/mhd-fuzz-src        # build.sh needs a tree
+                                                # with no in-tree config.status
+  WORK=/tmp/mhd-fuzz-work OUT=/tmp/mhd-fuzz-out \
+    MHD_SRC=/tmp/mhd-fuzz-src /tmp/mhd-fuzz-src/contrib/oss-fuzz/build.sh
+  /tmp/mhd-fuzz-out/fuzz_request /tmp/mhd-fuzz-out/../corpus -max_total_time=60
+  echo "exit=$?"        # anything but 0 here is a bug in the harness,
+                        # not a finding
+  ```
+
 `contrib/oss-fuzz/` also relies on two things this directory provides:
 `make -C src/fuzz refresh-corpus` (to regenerate `corpus/`) and the
 `corpus/known-findings/` reproducers, which it packages into

src/fuzz/fuzz_common.h

@@ -376,6 +376,37 @@ extern void
 __sanitizer_set_death_callback (void (*cb)(void)) __attribute__ ((weak));
 
 
+/**
+ * Ignore SIGPIPE.  Idempotent, so it is safe to call on every execution.
+ *
+ * This deliberately lives OUTSIDE the #ifndef FUZZ_NO_MAIN block below,
+ * because it is needed by the fuzz *target*, not merely by the built-in
+ * driver.  fuzz_request writes into an AF_UNIX socketpair whose peer end
+ * MHD may already have closed (any input that makes the daemon drop the
+ * connection early does this), and a write() to a socket with no reader
+ * raises SIGPIPE.
+ *
+ * None of the external engines does this for us: libFuzzer intercepts
+ * SEGV/BUS/ABRT/ILL/FPE/INT/TERM/XFSZ/USR1/USR2 and has no
+ * -handle_sigpipe flag at all, and AFL++/honggfuzz likewise leave the
+ * default disposition in place.  So without this call an OSS-Fuzz build
+ * of fuzz_request is killed by SIGPIPE after a few dozen executions,
+ * with no stack trace, no artifact and no report -- the target simply
+ * stops fuzzing.  Measured here: dead after ~28 execs without it,
+ * 540000 execs in 31 s with it.
+ */
+FUZZ_UNUSED static void
+fuzz_ignore_sigpipe (void)
+{
+  static int sigpipe_ignored;
+
+  if (sigpipe_ignored)
+    return;
+  sigpipe_ignored = 1;
+  (void) signal (SIGPIPE, SIG_IGN);
+}
+
+
 #ifndef FUZZ_NO_MAIN
 
 static void
@@ -395,7 +426,7 @@ fuzz_install_handlers (void)
   (void) sigaction (SIGILL, &sa, NULL);
   (void) sigaction (SIGFPE, &sa, NULL);
   (void) sigaction (SIGALRM, &sa, NULL);
-  (void) signal (SIGPIPE, SIG_IGN);
+  fuzz_ignore_sigpipe ();
 }
 
 

src/fuzz/fuzz_request.c

@@ -638,6 +638,14 @@ LLVMFuzzerTestOneInput (const uint8_t *data,
   unsigned int nconn = 1;
   static uint8_t xbuf[FUZZ_MAX_INPUT + 4096];
 
+  /* Must happen before the first write() into the socketpair.  The
+     built-in driver also does this from fuzz_install_handlers(), but
+     that is compiled out under -DFUZZ_NO_MAIN, which is exactly the
+     build every external fuzzing engine uses; the call is idempotent,
+     so doing it from both places is harmless.  See fuzz_ignore_sigpipe()
+     in fuzz_common.h for why the process dies without it. */
+  fuzz_ignore_sigpipe ();
+
   if (size < 6)
     return 0;