Fuzz the GDL reader with libFuzzer; fix 5 reader UB/DoS bugs

Add an opt-in libFuzzer harness (test/fuzz/, -DXCONQ_FUZZ=ON, Clang only)
for the GDL lisp reader -- the parser that untrusted downloaded modules,
save files, and network transfer-protocol messages all feed through.
fuzz_gdl.cc drives read_form_from_string() (the lowest reader entry) over
the whole input buffer; it links a fuzz-instrumented copy of the low-level
reader objects plus fuzz_stubs.cc (fake high-level callbacks, mirroring
imf2imf.cc's standalone-reader link closure). Interpreting parsed forms
(interp_form in read.cc) is not fuzzed: it needs full world state and does
file I/O, so a harness over it would surface init-order artifacts.

Corpus seeds regression cases + small real GDL modules; gdl.dict is ~1000
GDL tokens extracted from kernel/*.def by gen_dict.sh. A non-optional CI
`fuzz` leg builds the harness, replays the corpus (the fuzz-labelled CTest),
and runs ~5 min of live fuzzing under ASan+UBSan. Longer runs / OSS-Fuzz
documented in test/fuzz/README.md.

Two campaigns (a shallow-bug burst, then a post-fix run to coverage
saturation) surfaced five memory-safety / UB bugs, all in kernel/lisp.cc,
each reachable from a truncated or hostile module/save/network message:

- Infinite allocation loop (DoS) on an unclosed list ending in a symbol
  (e.g. "(foo"): strmgetc returns EOF at a string's terminating NUL without
  consuming, but strmungetc decremented the position unconditionally, so
  pushing that EOF back re-exposed the previous char and read_list re-read
  it forever, consing without bound. Fixed: strmungetc(EOF) is now a no-op,
  as in stdio's ungetc.
- Signed overflow accumulating a long digit run (num*10 + digit); saturates
  at INT_MAX, with (ch - '0') parenthesized so precedence can't overflow the
  intermediate near INT_MAX.
- Signed overflow in the decimal factor*num step; saturates.
- Left shift of a negative value in dice bit-packing ((dice-2)<<7 etc.) for
  out-of-range dice like "1d"/"1d1" (already warned, then computed anyway);
  fields packed as unsigned now, bit-identical for valid dice.
- Signed overflow accumulating a long octal escape (8*octch + digit); only
  the low byte is kept, so it accumulates as unsigned now.

All behavior-preserving for valid input: the quick ctest lane (555 tests,
incl. check-save's save/restore fidelity comparison) stays green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: brentjohnson

.github/workflows/c-cpp.yml

@@ -92,6 +92,52 @@ jobs:
     - name: test
       run: ctest --test-dir build --output-on-failure --label-exclude long -j$(nproc)
 
+  fuzz:
+    # Smoke-fuzz the GDL reader: build the opt-in libFuzzer harness
+    # (test/fuzz/, -DXCONQ_FUZZ=ON, Clang only) and run it for a few minutes
+    # over the checked-in corpus under ASan+UBSan.  This is a regression guard
+    # for the reader bugs the fuzzer found (see test/fuzz/README.md) -- the
+    # corpus-replay CTest re-parses every seed, and a short live run keeps
+    # mutating from them.  Longer campaigns are a local/OSS-Fuzz job, not CI.
+    runs-on: ubuntu-latest
+    env:
+      CC: clang
+      CXX: clang++
+      # Same rationale as the sanitizers leg: the reader never frees, so leak
+      # detection is deferred; any ASan/UBSan finding must abort so libFuzzer
+      # reports it as a crash.
+      ASAN_OPTIONS: detect_leaks=0:abort_on_error=1
+      UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1
+
+    steps:
+    - uses: actions/checkout@v4
+    - name: install dependencies
+      run: |
+        sudo apt update
+        sudo apt install -y clang
+    - name: configure
+      run: >
+        cmake -B build
+        -DCMAKE_BUILD_TYPE=RelWithDebInfo
+        -DXCONQ_FUZZ=ON
+        -DXCONQ_UI_CURSES=OFF -DXCONQ_UI_SDL=OFF
+    - name: build
+      run: cmake --build build -j$(nproc) --target fuzz_gdl
+    - name: corpus regression replay
+      # -runs=0 executes every checked-in seed once and exits nonzero on any
+      # crash; this is the fuzz-labelled CTest.
+      run: ctest --test-dir build --output-on-failure -L fuzz
+    - name: smoke fuzz
+      # A short live run (~5 min) mutating from the checked-in corpus with the
+      # GDL keyword dictionary; -runs caps it so a fast machine still stops.
+      run: >
+        ./build/test/fuzz/fuzz_gdl
+        -dict=test/fuzz/gdl.dict
+        -rss_limit_mb=4096
+        -max_total_time=300
+        -runs=20000000
+        test/fuzz/corpus
+
   macos:
     runs-on: macos-latest
     # sdlconq also links directly against Xlib/Xmu/Xext (see sdl/CMakeLists.txt),

CMakeLists.txt

@@ -39,6 +39,12 @@ set(XCONQ_SANITIZE "" CACHE STRING
 set(XCONQ_TEST_TIMEOUT_SCALE 1 CACHE STRING
     "Integer multiplier for test timeouts (raise for slow sanitized runs)")
 
+# Opt-in libFuzzer harness for the GDL reader (Clang only). When ON, builds a
+# dedicated fuzz_gdl target under test/fuzz/ with -fsanitize=fuzzer,address,
+# undefined on the reader objects it links. Kept out of default builds; see
+# test/fuzz/README.md. A short CI smoke-fuzz leg turns this on.
+option(XCONQ_FUZZ "Build the libFuzzer GDL-reader harness (Clang only)" OFF)
+
 # ---------------------------------------------------------------------------
 # Dependency discovery.  Each UI is skipped (with a warning) when its
 # dependencies are missing, so a bare kernel build always works.

MODERNIZATION-PLAN.md

@@ -389,59 +389,51 @@ and the BWidget vendor-audit are resolved by this removal (see §4 below).
   Also fixed two RelWithDebInfo-only `-Wmaybe-uninitialized` errors (that build
   type is new to CI): `economy.cc` `newtype` and `move.cc` `acpcost`, both
   false positives value-inited with a comment.
-- **[M] Fuzz the GDL reader** (`kernel/read.c`, `kernel/lisp.c`) with
-  libFuzzer/AFL. It parses untrusted input: downloaded game modules, network
-  messages, and save files all go through it.
-
-**⚙ PROMPT 3.4 — recommended model: Opus.** *(Harness design against a
-global-state kernel plus crash triage; a naive harness either won't reach
-the interesting code or will "find" init-order artifacts that aren't real.)*
-
-```text
-Task: build a libFuzzer harness for Xconq's GDL reader, run it locally
-until quiet, fix what it finds, and wire a short CI smoke-fuzz job.
-This hardens a defensive surface: the reader parses downloaded modules,
-save files, and network-fed data.
-
-PREREQUISITE: builds with Clang (§1 prompt 2.1 done). The §3 sanitizer
-task (3.3) is complementary but not required.
-Method:
-1. Harness (new test/fuzz/fuzz_gdl.cc): LLVMFuzzerTestOneInput feeds one
-   input buffer through the lisp-object reader. Study kernel/lisp.c and
-   read.c for the entry point that parses a stream/string into objects
-   (the same machinery module loading uses); prefer the lowest entry that
-   still exercises real parsing (strings, escapes, numbers/dice notation,
-   symbols, nested lists) WITHOUT needing a full game world. Mind global
-   state: the lisp package/obstack allocators persist across iterations —
-   do one-time init in LLVMFuzzerInitialize, and confirm iteration N's
-   parse cannot be corrupted by N-1's leftovers (interned symbols
-   accumulate — that is acceptable memory growth, not corruption; use
-   -rss_limit_mb generously). If interpretation of *forms* (interp_form
-   and friends) is reachable without a full game, add a second harness
-   for it; do not force it in one.
-2. Build wiring: an opt-in CMake target (XCONQ_FUZZ=ON, Clang-only,
-   -fsanitize=fuzzer,address,undefined on the kernel objects it links).
-   Keep it out of default builds.
-3. Corpus: seed from test/*.g and a selection of lib/*.g (small ones),
-   plus a dictionary generated from kernel/keyword.def's GDL symbols
-   (a small script can extract them — keywords steer the fuzzer into
-   real syntax fast).
-4. Run locally >= a few CPU-hours (parallel jobs fine). Triage every
-   crash/OOM: minimize (llvm's -minimize_crash), identify root cause in
-   the reader, fix with bounds/validation matching surrounding idiom
-   (history: the reader already had a BIGBUF off-by-one — expect
-   relatives). Every fixed crash's minimized input goes into
-   test/fuzz/corpus/ as a regression seed.
-5. CI: a short job (Clang, ~5 minutes of fuzzing with the checked-in
-   corpus + -runs bound) as a smoke test, non-optional once green.
-   Document longer local runs in test/fuzz/README.md. Note OSS-Fuzz as a
-   future option on the plan item, don't apply.
-Verify: harness builds and runs clean over the full checked-in corpus;
-normal build + quick ctest unaffected and green (reader fixes are on the
-hot path for every game load — the lib sweep is the regression test).
-Commit(s); mark this item done (strikethrough + date) in
-MODERNIZATION-PLAN.md, recording bugs found/fixed.
-```
+- ~~**[M] Fuzz the GDL reader** (`kernel/read.cc`, `kernel/lisp.cc`) with
+  libFuzzer/AFL.~~ *(done 7/2026)*: added a libFuzzer harness
+  (`test/fuzz/fuzz_gdl.cc`) driving `read_form_from_string()` — the lowest
+  reader entry, the same tokenizer/parser that module loads, save restore, and
+  the network transfer protocol all feed untrusted bytes through — over the
+  whole input buffer one form at a time. It links a dedicated copy of the
+  low-level reader objects with `-fsanitize=fuzzer,address,undefined` plus a
+  stub file mirroring `imf2imf.cc`'s standalone-reader link closure; the target
+  is opt-in (`-DXCONQ_FUZZ=ON`, Clang only, `test/fuzz/CMakeLists.txt`) and out
+  of default builds. Interpreting parsed forms (`interp_form` in `read.cc`) is
+  deliberately *not* fuzzed — it needs full world state and does file I/O
+  (include/image/default-game loading), so a harness over it would surface
+  init-order artifacts, not parser bugs; the untrusted-bytes-to-parser surface
+  is `lisp.cc`. Corpus (`test/fuzz/corpus/`) seeds from the reader's own
+  regression cases plus representative GDL; `gdl.dict` is ~1000 GDL tokens
+  extracted from `kernel/*.def` by `gen_dict.sh`. A `fuzz` CI leg (Clang) builds
+  the harness, replays the corpus (the `fuzz`-labelled CTest), and runs ~5 min
+  of live fuzzing under ASan+UBSan; longer runs and OSS-Fuzz are noted as future
+  options in `test/fuzz/README.md`. Two rounds of fuzzing (a shallow-bug burst,
+  then a ~5 CPU-hour post-fix campaign that found nothing new; coverage
+  saturated at ~325 edges) surfaced **five memory-safety / UB bugs, all fixed in
+  `kernel/lisp.cc`**, every one reachable from a truncated or hostile module /
+  save / network message and every one a real BIGBUF-relative the prompt
+  predicted:
+  - **Infinite allocation loop (DoS)** on an unclosed list ending in a symbol
+    (`(foo`): `strmgetc` returns EOF at a string's terminating NUL *without*
+    consuming, but `strmungetc` decremented the position unconditionally, so
+    pushing that EOF back re-exposed the previous char and `read_list` re-read
+    it forever, consing without bound. Fixed by making `strmungetc(EOF, …)` a
+    no-op, as in stdio's `ungetc`.
+  - **Signed overflow accumulating a long digit run** (`num*10 + digit`) — UB on
+    `999999999999`; saturates at `INT_MAX` now, with `(ch - '0')` parenthesized
+    so operator precedence can't overflow the intermediate near `INT_MAX`.
+  - **Signed overflow in the decimal `factor * num` step** — large value times
+    the ×100 decimal factor; saturates now.
+  - **Left shift of a negative value in dice bit-packing** (`(dice - 2) << 7`,
+    `(numdice - 1) << 11`) — UB for out-of-range dice like `1d`/`1d1` (already
+    warned about, then computed anyway); packs the fields as unsigned now,
+    bit-identical for valid dice.
+  - **Signed overflow accumulating a long octal escape** (`8*octch + digit`) —
+    UB on `"\04444444444"`; only the low byte is kept, so it accumulates as
+    unsigned (defined wrap, same result) now.
+  All fixes are behavior-preserving for valid input: the full quick ctest lane
+  (555 tests, including `check-save`'s save/restore fidelity comparison) stays
+  green in the normal build.
 - **[S] Port the `test/*-diff.sh` / `*-uses.sh` consistency checks into CTest**
   (docs ↔ `.def` symbols ↔ library usage) so they can't silently rot.
 

kernel/lisp.cc

@@ -173,6 +173,15 @@ strmgetc(Strm *strm)
 static void
 strmungetc(int ch, Strm *strm)
 {
+    /* Pushing EOF back is a no-op, as in stdio's ungetc(EOF, ...).  strmgetc
+       returns EOF at a string's terminating NUL (or a file's end) WITHOUT
+       consuming anything, so moving the position back here would re-expose
+       the previous character.  Several callers ungetc unconditionally after a
+       read that may have hit EOF (e.g. the symbol/number readers), so an
+       unclosed list ending in a symbol like "(foo" would otherwise loop
+       forever re-reading that character and allocating without bound. */
+    if (ch == EOF)
+      return;
     if (strm->type == stringstrm) {
 	--strm->ptr.sp;
     } else {
@@ -373,7 +382,12 @@ read_form_aux(Strm *strm)
 	    while ((ch = strmgetc(strm)) != EOF) {
 	    	if (isdigit(ch)) {
 	    	    /* should ignore decimal digits past second one */
-		    num = num * 10 + ch - '0';
+		    /* Saturate instead of overflowing int (UB) on an absurdly
+		       long digit run from hostile input. */
+		    if (num > (INT_MAX - 9) / 10)
+		      num = INT_MAX;
+		    else
+		      num = num * 10 + (ch - '0');
 		    if (factor > 1)
 		      factor /= 10;
 		} else if (ch == 'd') {
@@ -430,16 +444,27 @@ read_form_aux(Strm *strm)
 				 numdice, dice, num, num);
 		if (!minus && minus2)
 		    num = num - 1;
-		num = 
+		/* Pack the fields as unsigned: out-of-range dice/addon values
+		   (already warned about just above) can make (dice - 2) etc.
+		   negative, and left-shifting a negative int is UB. */
+		num =
 		    (short)(
-			((minus | minus2) << 15) 
-			| (((minus | minus2) ? 0 : 1) << 14) 
-			| ((numdice - 1) << 11) | ((dice - 2) << 7) 
-			| (num & 0x7f));
+			(((unsigned) (minus | minus2)) << 15)
+			| (((minus | minus2) ? 0u : 1u) << 14)
+			| (((unsigned) (numdice - 1)) << 11)
+			| (((unsigned) (dice - 2)) << 7)
+			| ((unsigned) num & 0x7f));
 	    }
 	    // "Integerize" decimal spec, if neccesary.
 	    else {
-	    	num = factor * num;
+		/* Saturate instead of overflowing int (UB): a huge accumulated
+		   value times factor (up to 100 for a decimal) can exceed int. */
+		if (factor > 1 && num > INT_MAX / factor)
+		    num = INT_MAX;
+		else if (factor > 1 && num < INT_MIN / factor)
+		    num = INT_MIN;
+		else
+		    num = factor * num;
 	    }
 	    if (!actually_read_lisp)
 	      return lispnil;
@@ -534,7 +559,10 @@ read_delimited_text(Strm *strm, const char *delim, int spacedelimits,
 		/* Soak up numeric digits (don't complain about 8 or 9,
 		   sloppy but traditional). */
 		while ((ch = strmgetc(strm)) != EOF && isdigit(ch)) {
-		    octch = 8 * octch + ch - '0';
+		    /* Unsigned accumulation: only the low byte is kept
+		       (ch = octch below), but a long digit run from hostile
+		       input overflows a signed int (UB). */
+		    octch = (int) (8u * (unsigned) octch + (ch - '0'));
 		}
 		/* The non-digit char is actually next one in the string. */
 		strmungetc(ch, strm);

test/CMakeLists.txt

@@ -88,3 +88,6 @@ set_tests_properties(check-ai check-long check-run PROPERTIES LABELS "long")
 # matching that same per-run bound; check-auto is a single fixed run.
 set_tests_properties(check-auto PROPERTIES TIMEOUT ${XCONQ_AUTO_TIMEOUT})
 set_tests_properties(check-ai check-run check-long PROPERTIES TIMEOUT ${XCONQ_LONG_TIMEOUT})
+
+# The opt-in libFuzzer GDL-reader harness (only active under -DXCONQ_FUZZ=ON).
+add_subdirectory(fuzz)

test/fuzz/CMakeLists.txt

@@ -0,0 +1,64 @@
+# libFuzzer harness for the GDL reader.  Opt-in (-DXCONQ_FUZZ=ON), Clang only,
+# kept out of default builds.  See test/fuzz/README.md.
+#
+# Xconq is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2, or (at your option)
+# any later version.
+
+if(NOT XCONQ_FUZZ)
+    return()
+endif()
+
+if(NOT CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+    message(FATAL_ERROR
+        "XCONQ_FUZZ needs Clang for libFuzzer "
+        "(-fsanitize=fuzzer); configure with CXX=clang++.")
+endif()
+
+# fuzzer = coverage instrumentation + libFuzzer main (linked into the exe);
+# address,undefined catch the memory-safety / UB bugs we are hunting.  Applied
+# to compile *and* link of every object in the fuzz target so instrumentation
+# is uniform.
+set(XCONQ_FUZZ_FLAGS -fsanitize=fuzzer,address,undefined
+                     -fno-omit-frame-pointer -g)
+
+# The low-level reader objects (kernel/conqlow set: lisp/util + platform glue),
+# built here with fuzz instrumentation rather than reusing the plain `conqlow`
+# library.  Only the objects the harness actually references get pulled into
+# the executable from this static lib (imf/gif/socket stay out), exactly as
+# imf2imf links the same low-level layer standalone.  Keep in sync with
+# CONQLOW_SOURCES in kernel/CMakeLists.txt.
+set(_reader_srcs
+    ${CMAKE_SOURCE_DIR}/kernel/lisp.cc
+    ${CMAKE_SOURCE_DIR}/kernel/util.cc
+    ${CMAKE_SOURCE_DIR}/kernel/unix.cc
+    ${CMAKE_SOURCE_DIR}/kernel/socket.cc
+    ${CMAKE_SOURCE_DIR}/kernel/imf.cc
+    ${CMAKE_SOURCE_DIR}/kernel/gif.cc)
+
+add_library(conqfuzz_reader STATIC ${_reader_srcs})
+target_link_libraries(conqfuzz_reader PUBLIC xconq_common)
+target_compile_definitions(conqfuzz_reader PRIVATE
+    XCONQDATA="${XCONQ_DATA_DIR}"
+    XCONQSCORES="${XCONQ_SCORES_DIR}")
+target_compile_options(conqfuzz_reader PRIVATE ${XCONQ_FUZZ_FLAGS})
+
+add_executable(fuzz_gdl fuzz_gdl.cc fuzz_stubs.cc)
+target_link_libraries(fuzz_gdl PRIVATE conqfuzz_reader xconq_common)
+target_compile_options(fuzz_gdl PRIVATE ${XCONQ_FUZZ_FLAGS})
+target_link_options(fuzz_gdl PRIVATE ${XCONQ_FUZZ_FLAGS})
+
+# CTest smoke test: replay the whole checked-in corpus under the sanitizers
+# (no mutation, -runs=0 just executes each seed and exits nonzero on any
+# crash).  This is the regression guard for every reader bug the fuzzer found.
+# The corpus dir is passed as a directory argument; libFuzzer runs each file.
+add_test(NAME fuzz-gdl-corpus
+    COMMAND fuzz_gdl -runs=0 ${CMAKE_CURRENT_SOURCE_DIR}/corpus)
+set_tests_properties(fuzz-gdl-corpus PROPERTIES
+    LABELS "fuzz"
+    TIMEOUT 120
+    # detect_leaks=0: the reader interns/allocates and frees nothing by design
+    # (same rationale as the sanitizers CI leg), so LSan would flag the corpus.
+    # halt_on_error makes any UB finding fail the test.
+    ENVIRONMENT "ASAN_OPTIONS=detect_leaks=0:abort_on_error=1;UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1")