commit 56713e94bbc7ca31a197db4103b76cef3a6c882c
parent a1fb82fc3c26d7017f8313bf763edd9f40d396d0
Author: Christian Grothoff <christian@grothoff.org>
Date: Tue, 28 Jul 2026 17:18:20 +0200
-comments
Diffstat:
8 files changed, 123 insertions(+), 72 deletions(-)
diff --git a/contrib/ci/jobs/4-fuzz/2-fuzz.sh b/contrib/ci/jobs/4-fuzz/2-fuzz.sh
@@ -23,13 +23,13 @@ make -C src/fuzz check \
MHD_FUZZ_ITERATIONS="${FUZZ_ITERATIONS}" \
MHD_FUZZ_SEED="${FUZZ_SEED}"
-# Replay the whole committed seed corpus through every harness. This is
-# the part that proves a fixed crash stays fixed, and it is cheap.
+# Replay the whole committed seed corpus - including
+# corpus/known-findings/, the reproducers of src/fuzz/README section 6 -
+# through every harness. This is the part that proves a fixed crash
+# stays fixed, and it is cheap.
#
-# corpus/known-findings/ is deliberately NOT included: check-corpus does
-# not recurse, and the reproducers of the findings that are still open
-# (src/fuzz/README section 6, patches/) are expected to fail on a build
-# with --enable-asserts. Replay those by hand when working on a fix:
-# for f in src/fuzz/corpus/known-findings/*.bin ; do
-# ./src/fuzz/fuzz_request --file="$f" ; done
+# It only means something because 1-build.sh configures --enable-asserts:
+# most of those reproducers trip an mhd_assert(), which compiles away
+# without it, so on a default build they would pass whether MHD is fixed
+# or not.
make -C src/fuzz check-corpus
diff --git a/contrib/oss-fuzz/README b/contrib/oss-fuzz/README
@@ -341,12 +341,14 @@ regression tests: ClusterFuzz keeps every seed in the corpus and replays
it on every run.
Reproducers of findings that are still *open* are skipped. A finding is
-open exactly when `patches/$ID.diff` exists (see `patches/README`), and
-its reproducer crashes the target by construction, so shipping it would
-make every ClusterFuzz run open by rediscovering a bug that is already
-written down. Committing the fix means deleting the diff, and that
-alone promotes the reproducer to a shipped regression seed. At the time
-of writing this skips K7.
+open exactly when `patches/$ID.diff` exists, and its reproducer crashes
+the target by construction, so shipping it would make every ClusterFuzz
+run open by rediscovering a bug that is already written down. Committing
+the fix means deleting the diff, and that alone promotes the reproducer
+to a shipped regression seed.
+
+Nothing is open at the time of writing, so `patches/` does not exist and
+all eight reproducers ship.
Regenerating the corpus from the harnesses' built-in seeds:
diff --git a/contrib/oss-fuzz/make_seed_corpus.sh b/contrib/oss-fuzz/make_seed_corpus.sh
@@ -64,14 +64,17 @@ for fuzzer in ${FUZZERS}; do
# The K* reproducers are fuzz_request inputs.
#
- # Only the ones whose defect is already fixed are shipped. A finding
- # that is still open has an unapplied fix in patches/$ID.diff (see
- # patches/README), and its reproducer, by construction, crashes the
- # target: shipping it would make every ClusterFuzz run start by
+ # Only the ones whose defect is already fixed are shipped. While a
+ # finding is open its proposed fix is kept as an unapplied diff in
+ # patches/$ID.diff, and its reproducer crashes the target by
+ # construction; shipping it would make every ClusterFuzz run start by
# rediscovering a bug that is already written down, and bury the
- # findings that are actually new. Deleting the diff -- which is what
- # committing the fix should do -- promotes the reproducer to a
- # permanent regression seed here, with no further edit.
+ # findings that are actually new. Committing the fix deletes the diff,
+ # and that alone promotes the reproducer to a permanent regression seed
+ # here, with no further edit.
+ #
+ # patches/ therefore does not exist while nothing is open, which is the
+ # normal state; the test below simply never fires then.
if [ "${fuzzer}" = "fuzz_request" ] && [ -d "${FINDINGS}" ]; then
for f in "${FINDINGS}"/*.bin; do
[ -f "${f}" ] || continue
diff --git a/src/fuzz/BUILD-INTEGRATION.md b/src/fuzz/BUILD-INTEGRATION.md
@@ -188,11 +188,12 @@ make -C src/fuzz check-corpus
* `make check` in `src/fuzz` runs the full discipline / memory-limit
range (`MHD_FUZZ_MIN_DISCIPLINE=-3`, `MHD_FUZZ_MIN_MEM_LIMIT=0`) with
50000 iterations per harness, roughly 3 seconds in total under
- ASAN+UBSAN. The findings K1-K6 of `README` section 6 are all fixed on
+ ASAN+UBSAN. The findings K1-K8 of `README` section 6 are all fixed on
master, so the full range is clean; their reproducers stay in
- `corpus/known-findings/` as regressions. K7 is open, and its
- reproducer lives there too — `check-corpus` does not recurse into that
- directory, which is what keeps `make check` green while it is.
+ `corpus/known-findings/`, and `make check-corpus` replays that
+ directory alongside the generated corpus. Both only mean anything on a
+ tree configured with `--enable-asserts`: most of those reproducers trip
+ an `mhd_assert()`, which compiles away without it.
---
@@ -257,5 +258,5 @@ Two consequences for anyone editing `src/fuzz/`:
`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
-`fuzz_request_seed_corpus.zip` so that K1–K6 become permanent
+`fuzz_request_seed_corpus.zip` so that they become permanent
regressions. It is deliberately **not** part of `contrib/ci/jobs/`.
diff --git a/src/fuzz/Makefile.am b/src/fuzz/Makefile.am
@@ -115,10 +115,27 @@ refresh-corpus: $(check_PROGRAMS)
# Replay the whole on-disk corpus through every harness; this is what a
# CI regression run should do after a crash has been fixed.
+#
+# corpus/known-findings/ is listed separately because --corpus-dir does
+# not recurse. It holds the reproducers of the findings in README
+# section 6; all of them are fixed, so anything failing there is a
+# regression. The one intended exception is a reproducer committed while
+# its finding is still open: that fails here until the fix lands, which
+# is what a regression test for an unfixed bug is supposed to do.
+#
+# Note that these only bite on a tree configured with --enable-asserts;
+# without it mhd_assert() compiles away and most of these reproducers
+# pass whether MHD is fixed or not.
+CORPUS_DIRS = \
+ $(srcdir)/corpus \
+ $(srcdir)/corpus/known-findings
+
.PHONY: check-corpus
check-corpus: $(check_PROGRAMS)
for p in $(check_PROGRAMS) ; do \
- MHD_FUZZ_MIN_DISCIPLINE="$(MHD_FUZZ_MIN_DISCIPLINE)" \
- MHD_FUZZ_MIN_MEM_LIMIT="$(MHD_FUZZ_MIN_MEM_LIMIT)" \
- ./$$p --corpus-dir=$(srcdir)/corpus || exit 1 ; \
+ for d in $(CORPUS_DIRS) ; do \
+ MHD_FUZZ_MIN_DISCIPLINE="$(MHD_FUZZ_MIN_DISCIPLINE)" \
+ MHD_FUZZ_MIN_MEM_LIMIT="$(MHD_FUZZ_MIN_MEM_LIMIT)" \
+ ./$$p --corpus-dir=$$d || exit 1 ; \
+ done ; \
done
diff --git a/src/fuzz/README b/src/fuzz/README
@@ -202,11 +202,13 @@ violating either makes MHD abort on input that is perfectly legal:
header naming the upgrade token and not "close"). All three come
off the wire, so the path stays attacker-driven.
- Unlike the suspend rule, this one is not sufficient, and is not
- supposed to be: it is exactly the check a real application can
- perform, and a request that passes it can still abort MHD. That is
- finding K7 in section 6. Do not tighten it further to keep the
- suite green -- the point is that an application cannot do better.
+ This is exactly the check a real application can perform, and no
+ more. It deliberately does not try to predict whether MHD will
+ accept the upgrade: a request that passes it used to abort MHD, which
+ is finding K7 in section 6, and MHD_queue_response() now answers
+ MHD_NO for that case instead. Do not tighten the condition to keep
+ the suite green -- an application cannot do better than this, so
+ neither should the harness.
The message-framing headers (Content-Length, Transfer-Encoding) are
deliberately absent from the response-header table: MHD generates them
@@ -529,7 +531,8 @@ K6 digestauth.c:860 check_nonce_nc():
K7 connection.c:2596 build_header_response():
mhd_assert ((NULL == r->upgrade_handler) ||
(MHD_CONN_MUST_UPGRADE == c->keepalive))
- Open. Driven entirely from the wire, with default daemon options.
+ Fixed by commit acef58a0. Was driven entirely from the wire, with
+ default daemon options.
While parsing the request headers, connection.c sets
c->keepalive = MHD_CONN_MUST_CLOSE
@@ -565,9 +568,10 @@ K7 connection.c:2596 build_header_response():
default-configured server, which is a good deal worse than the
"application misuse" it first looked like.
- Fix: patches/K7.diff, which rejects the response in
- MHD_queue_response() next to the other upgrade preconditions, so the
- application gets MHD_NO instead of an abort.
+ The fix rejects the response in MHD_queue_response()
+ (connection.c:8338) next to the other upgrade preconditions, so the
+ application gets MHD_NO -- which it already has to handle -- instead
+ of an abort.
Repro: corpus/known-findings/K7-upgrade-after-must-close.bin
GET / HTTP/1.1
Host: x
@@ -580,8 +584,9 @@ K7 connection.c:2596 build_header_response():
K8 digestauth.c MHD_digest_auth_check_digest2():
MHD_PANIC ("Wrong 'malgo3' value, only one base hashing
algorithm ... must be specified, API violation")
- Open. An API-contract defect rather than a parsing bug: unlike K7
- the offending argument comes from the application, not the network.
+ Fixed by commit 07c051dd. An API-contract defect rather than a
+ parsing bug: unlike K7 the offending argument came from the
+ application, not from the network.
MHD_DIGEST_ALG_AUTO is a documented member of
enum MHD_DigestAuthAlgorithm, and the `algo` parameter of
@@ -605,26 +610,41 @@ K8 digestauth.c MHD_digest_auth_check_digest2():
because the digest length does not identify the algorithm (SHA-256
and SHA-512/256 digests are both 32 bytes).
- Fix: patches/K8.diff, which documents the restriction in
- microhttpd.h and returns MHD_NO instead of aborting.
+ The fix documents the restriction in microhttpd.h and returns MHD_NO
+ instead of aborting. AUTO is left unusable with the
+ pre-computed-digest entry points, deliberately: there is nothing to
+ map it to.
Repro: none committed. The argument is the application's, so the
harness never passes AUTO there (see run_digest_check() in
fuzz_request.c); to see it, change the MHD_DIGEST_ALG_* argument of
the variant-4 call to MHD_DIGEST_ALG_AUTO and replay
- corpus/fuzz_request-36.bin.
+ corpus/fuzz_request-37.bin.
Status
------
-K1-K5 are fixed on master by commits 300a2ab0, 68c83f22, e04eb218,
-0b750975 and 6fcdfd43 respectively; K6 by f438804c. Their seven
-reproducers in `corpus/known-findings/` therefore replay clean, and
-`make check-corpus` asserts exactly that.
-
-K7 and K8 are open, and their fixes are unapplied diffs in
-`../../patches/`. K7 has a reproducer, which aborts on an
-`--enable-asserts` build and passes with patches/K7.diff applied; K8 has
-none, because its trigger is an argument the application chooses.
+All eight findings are fixed on master:
+
+ K1 300a2ab0 K4a 0b750975 K7 acef58a0
+ K2 68c83f22 K4b 0b750975 K8 07c051dd
+ K3 e04eb218 K5 6fcdfd43
+ K6 f438804c
+
+The eight reproducers in `corpus/known-findings/` therefore all replay
+clean on a build configured with `--enable-asserts`, and
+`make check-corpus` asserts exactly that -- it replays that directory
+along with the generated corpus. K8 has no reproducer, because its
+trigger is an argument the application chooses rather than anything that
+comes off the wire.
+
+When the next finding is opened, its reproducer goes into
+`corpus/known-findings/` and will make `make check-corpus` fail until the
+fix lands. That is the intended behaviour: it is the same arrangement as
+the `XFAIL_TESTS` entries in `src/microhttpd/Makefile.am`, except that
+here the expectation is not encoded, so the commit that adds a live
+reproducer should say so. `contrib/oss-fuzz/make_seed_corpus.sh` has to
+skip such a reproducer as well, or every ClusterFuzz run starts by
+rediscovering it; see the comment there.
The `make check` defaults in Makefile.am
diff --git a/src/fuzz/corpus/README b/src/fuzz/corpus/README
@@ -132,13 +132,14 @@ known-findings/
---------------
Reproducers for the issues that this suite found in MHD itself; see
-section 6 of ../README, which records the status of each. K1-K6 are
-fixed on master and therefore replay clean; K7 is open and *aborts* on a
-build configured with --enable-asserts.
+section 6 of ../README, which records the status of each. All of them
+are fixed on master, so all of them replay clean, and `check-corpus`
+replays this directory too.
-They are kept out of the main corpus directory for exactly that reason:
-`check-corpus` does not recurse, so it stays green while an open finding
-still has a live reproducer here. Replay one with
+They are kept as a separate, explicitly named set rather than being
+folded into the main corpus because the files there are *generated*: a
+hand-written input would be clobbered or renumbered by the next
+`refresh-corpus`. Replay one with
./fuzz_request --file=src/fuzz/corpus/known-findings/K1-digest-empty-realm.bin
@@ -156,17 +157,24 @@ still has a live reproducer here. Replay one with
K6-nonce-length-collision.bin digestauth.c check_nonce_nc()
mhd_assert (0 == nn->nonce[noncelen])
K7-upgrade-after-must-close.bin connection.c build_header_response()
- mhd_assert (upgrade -> MUST_UPGRADE)
- *** still open, see ../../../patches ***
+ mhd_assert (upgrade -> MUST_UPGRADE);
+ a single request carrying both
+ Content-Length and chunked
+ Transfer-Encoding, then an upgrade
These files are *not* regenerated by `--write-corpus`; they are edited by
hand. When the input format changes they have to be migrated, and the
migration has to be checked -- replay each one with `--verbose` before
and after and confirm the daemon, handler, body and challenge counts are
unchanged. The last such change made the API-selection block
-unconditional, which is six zero bytes inserted at offset 4 for all
-seven.
+unconditional, which is six zero bytes inserted at offset 4 for K1-K6.
K8 has no reproducer here: its trigger is an argument the application
chooses, not network input, so the harness never performs it. See
section 6 of ../README for how to reach it by hand.
+
+A reproducer added while its finding is still open will make
+`make check-corpus` fail, which is intended -- that is what a regression
+test for an unfixed bug does. `contrib/oss-fuzz/make_seed_corpus.sh`
+skips such a file so that ClusterFuzz does not spend every run
+rediscovering it; the rule it uses is whether `patches/$ID.diff` exists.
diff --git a/src/fuzz/fuzz_request.c b/src/fuzz/fuzz_request.c
@@ -1518,18 +1518,18 @@ ahc (void *cls,
application performs, and all three inputs come off the wire, so
the path stays fully attacker-driven.
- It is deliberately NOT enough to keep MHD's
+ It is deliberately NOT enough to predict whether MHD will accept
+ the upgrade, and it is not meant to be: MHD may have decided during
+ header parsing that the connection must close, and does not expose
+ that decision. That used to trip
mhd_assert (NULL == r->upgrade_handler ||
MHD_CONN_MUST_UPGRADE == c->keepalive)
- in build_header_response() satisfied, and it is not meant to be:
- MHD may have decided during header parsing that the connection must
- close, and does not expose that decision. That is finding K7 (see
- README section 6); corpus/known-findings/K7-upgrade-after-must-close.bin
- is the reproducer, and patches/K7.diff turns the abort into the
- MHD_NO that the code below already handles. Do not "fix" this by
- weakening the condition here -- the check above is what an
- application can actually do, and the harness has to behave like
- one. */
+ in build_header_response() -- finding K7, see README section 6,
+ reproducer corpus/known-findings/K7-upgrade-after-must-close.bin --
+ and MHD_queue_response() now returns MHD_NO for it instead, which
+ the code below already handles. Do not "fix" this by tightening
+ the condition here: the check above is what an application can
+ actually do, and the harness has to behave like one. */
if (cfg.allow_upgrade &&
(NULL != version) &&
(0 == strcmp (version, MHD_HTTP_VERSION_1_1)) &&
