Merge chore/unworked-minor-issues: sweep all unworked minor review findings

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NpysYDDJac63yz2mZKKiDf

Author: etr

.github/workflows/codeql-analysis.yml

@@ -63,7 +63,14 @@ jobs:
     - run: git checkout HEAD^2
       if: ${{ github.event_name == 'pull_request' }}
 
-    - name: Install libmicrohttpd dependency
+    - name: Fetch libmicrohttpd from cache
+      id: cache-libmicrohttpd
+      uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830  # v4.3.0
+      with:
+        path: libmicrohttpd-1.0.3
+        key: ${{ runner.os }}-libmicrohttpd-1.0.3-codeql
+
+    - name: Build libmicrohttpd dependency (if not cached)
       # Supply-chain: libmicrohttpd-1.0.3.tar.gz is sha256-pinned. The digest
       # must match the value used in verify-build.yml; both workflows track the
       # same release. To rotate the libmicrohttpd version, update both the URL
@@ -79,7 +86,12 @@ jobs:
         cd libmicrohttpd-1.0.3 ;
         ./configure --disable-examples ;
         make ;
-        sudo make install ;
+      timeout-minutes: 30
+      if: steps.cache-libmicrohttpd.outputs.cache-hit != 'true'
+
+    - name: Install libmicrohttpd dependency
+      run: cd libmicrohttpd-1.0.3 ; sudo make install ;
+      timeout-minutes: 30
 
     # Initializes the CodeQL tools for scanning.
     - name: Initialize CodeQL
@@ -98,6 +110,8 @@ jobs:
         ./bootstrap ;
         ./configure --enable-same-directory-build ;
         make ;
+      timeout-minutes: 30
 
     - name: Perform CodeQL Analysis
       uses: github/codeql-action/analyze@411c4c9a36b3fca4d674f06b6396b2c6d23522c6  # v3.36.3
+      timeout-minutes: 30

.github/workflows/verify-build.yml

@@ -234,7 +234,7 @@ jobs:
             compiler-family: gcc
             c-compiler: gcc-14
             cc-compiler: g++-14
-            debug: nodebug  # configure step passes --enable-debug unconditionally for valgrind-* lanes
+            debug: nodebug
             coverage: nocoverage
             shell: bash
           - test-group: extra
@@ -244,7 +244,7 @@ jobs:
             compiler-family: gcc
             c-compiler: gcc-14
             cc-compiler: g++-14
-            debug: nodebug  # configure step passes --enable-debug unconditionally for valgrind-* lanes
+            debug: nodebug
             coverage: nocoverage
             shell: bash
           # Test build without digest auth support (issue #232)
@@ -674,13 +674,27 @@ jobs:
       uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830  # v4.3.0
       with:
         path: ${{ github.workspace }}/libcxx-msan
-        key: ${{ matrix.os }}-${{ matrix.c-compiler }}-libcxx-msan-origins-v1
+        key: ${{ matrix.os }}-${{ matrix.c-compiler }}-libcxx-msan-llvmorg-18.1.8-origins-v1
       if: ${{ matrix.build-type == 'msan' }}
 
     - name: Build instrumented libc++ (msan lane, if not cached)
+      # Supply-chain: llvmorg-18.1.8 is a mutable git tag, not an immutable
+      # commit SHA, so a post-clone SHA assertion guards against the tag
+      # being force-moved upstream (CWE-829). LLVM_SHA was obtained via
+      # `git ls-remote https://github.com/llvm/llvm-project refs/tags/llvmorg-18.1.8`
+      # at pin time; to rotate the tag, update both the --branch value below
+      # and LLVM_SHA in the same PR. Mirrors the IWYU SHA-pin pattern above.
       run: |
+        set -euo pipefail
         sudo apt-get install -y ninja-build cmake ;
+        LLVM_SHA=3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff ;
         git clone --depth 1 --branch llvmorg-18.1.8 https://github.com/llvm/llvm-project.git llvm-project ;
+        HEAD_SHA="$(git -C llvm-project rev-parse --verify HEAD)" ;
+        if [ "${HEAD_SHA}" != "${LLVM_SHA}" ]; then
+          echo "LLVM SHA mismatch: expected ${LLVM_SHA}, got ${HEAD_SHA}" >&2 ;
+          exit 1 ;
+        fi
+        echo "llvm-project pinned at SHA: ${HEAD_SHA}" ;
         cmake -G Ninja -S llvm-project/runtimes -B build-libcxx-msan \
           -DCMAKE_C_COMPILER=clang-18 \
           -DCMAKE_CXX_COMPILER=clang++-18 \
@@ -754,6 +768,11 @@ jobs:
     - name: Fetch libmicrohttpd from cache (msan lane)
       id: cache-libmicrohttpd-msan
       uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830  # v4.3.0
+      # NOTE: the '-v1' suffix below must be bumped manually whenever the
+      # CFLAGS/LDFLAGS in the "Build instrumented libmicrohttpd" step change
+      # -- this key does not otherwise encode them, so a flag change alone
+      # would silently keep reusing a stale (differently-instrumented)
+      # cached artifact.
       with:
         path: libmicrohttpd-1.0.3
         key: ${{ matrix.os }}-${{ matrix.c-compiler }}-libmicrohttpd-1.0.3-msan-v1
@@ -772,7 +791,7 @@ jobs:
         export CFLAGS='-fsanitize=memory -fsanitize-memory-track-origins -fno-omit-frame-pointer -g -O1' ;
         export LDFLAGS='-fsanitize=memory' ;
         ./configure --disable-examples --disable-https ;
-        make ;
+        make -j$(nproc) ;
       if: ${{ matrix.build-type == 'msan' && steps.cache-libmicrohttpd-msan.outputs.cache-hit != 'true' }}
 
     - name: Install libmicrohttpd
@@ -911,14 +930,24 @@ jobs:
     - name: Run valgrind-lane structural gate
       # TASK-088: asserts the Valgrind CI lane is wired correctly (valid YAML,
       # valgrind matrix entry present, no --disable-valgrind-{helgrind,drd,
-      # sgcheck} flags remain, `make check-valgrind` still invoked, and the
-      # results-print step surfaces helgrind + drd logs). Valgrind is
+      # sgcheck} flags remain, `make "check-valgrind-${TOOL}"` (per-tool
+      # AX_VALGRIND_CHECK target) invoked, and the results-print step
+      # surfaces helgrind + drd logs). Valgrind is
       # Linux-only, so the actual race-detector run + finding-triage happen on
       # the PR's Linux lane; this gate only guards the wiring. Mirrors the
       # check-msan-lane.sh gate idiom.
       run: ./scripts/check-valgrind-lane.sh
       if: ${{ matrix.build-type == 'lint' && matrix.os-type == 'ubuntu' }}
 
+    - name: Run DR-008 stress-lane structural gate
+      # TASK-092: asserts the two DR-008 concurrency/deadlock stress-gate
+      # steps (route_table_concurrency under TSan, stop()-from-handler
+      # deadlock contract) stay wired correctly -- present, correctly `if:`
+      # gated, and timeout-boxed. See test/PERFORMANCE.md ("CI wiring —
+      # DR-008 stress gates"). Mirrors the check-msan-lane.sh gate idiom.
+      run: ./scripts/check-dr008-lanes.sh
+      if: ${{ matrix.build-type == 'lint' && matrix.os-type == 'ubuntu' }}
+
     - name: Run parallel-install-lane structural gate
       # TASK-089: asserts the parallel-install CI gate is wired correctly
       # (valid YAML, a matrix entry opts in via parallel-install: check, a step
@@ -1158,8 +1187,27 @@ jobs:
             export MSAN_OPTIONS="${MSAN_OPTIONS}:suppressions=$(pwd)/../test/msan.supp" ;
           fi
           # Derive the unit/ subset from the test Makefile so newly-added
-          # unit tests are covered automatically.
-          MSAN_TESTS="$(grep -oE '^[A-Za-z0-9_]+_SOURCES[[:space:]]*=[[:space:]]*unit/' ../test/Makefile.am | sed -E 's/_SOURCES.*//' | tr '\n' ' ')" ;
+          # unit tests are covered automatically, then further exclude any
+          # unit/-sourced target whose test file transitively links curl
+          # (e.g. via curl_easy_perform() round-trips) -- curl cannot
+          # practically be msan-instrumented, so its uninstrumented output
+          # would surface as false use-of-uninitialized-value reports.
+          # NOTE: this only matches a target's _SOURCES entry when it is
+          # written on a single line (`<target>_SOURCES = unit/<file>.cpp`);
+          # a future multi-line _SOURCES assignment, or a unit/ target whose
+          # first source is not itself under unit/, would be silently
+          # dropped without failing CI (the empty-set guard below only
+          # catches total loss, not partial narrowing).
+          MSAN_TESTS="" ;
+          while IFS= read -r src_line; do
+            target="$(printf '%s' "$src_line" | sed -E 's/_SOURCES.*//')" ;
+            source_rel="$(printf '%s' "$src_line" | sed -E 's/^[A-Za-z0-9_]+_SOURCES[[:space:]]*=[[:space:]]*//')" ;
+            if grep -q 'curl/curl.h' "../test/${source_rel}" 2>/dev/null; then
+              echo "msan: excluding ${target} (${source_rel} includes curl/curl.h)" ;
+              continue ;
+            fi
+            MSAN_TESTS="${MSAN_TESTS}${target} " ;
+          done <<< "$(grep -E '^[A-Za-z0-9_]+_SOURCES[[:space:]]*=[[:space:]]*unit/' ../test/Makefile.am)" ;
           if [ -z "$MSAN_TESTS" ]; then echo "ERROR: msan: derived empty test set — grep pattern found no unit/ targets in test/Makefile.am" >&2; exit 1; fi ;
           make check TESTS="$MSAN_TESTS" ;
         else
@@ -1178,7 +1226,7 @@ jobs:
       # Reuses the same TSAN_OPTIONS suppression file as "Run tests" to
       # mask the benign libstdc++ std::ctype narrow-cache race; any
       # libhttpserver-internal race stays fatal.
-      timeout-minutes: 2
+      timeout-minutes: 5
       run: |
         cd build ;
         export TSAN_OPTIONS="suppressions=$(pwd)/../test/tsan.supp" ;
@@ -1196,8 +1244,36 @@ jobs:
       # is the safety net; a clean zero-exit child would be a regression.
       timeout-minutes: 3
       run: |
+        set -o pipefail ;
         cd build ;
-        make -C test check-stop-from-handler ;
+        make -C test check-stop-from-handler 2>&1 | tee test/stop_from_handler.log ;
+      if: ${{ matrix.os-type == 'ubuntu' && matrix.test-group == 'basic' && matrix.c-compiler == 'gcc' && matrix.debug == 'nodebug' && matrix.linking == 'dynamic' && matrix.build-type == 'classic' }}
+
+    - name: Verify stop()-from-handler contract fired (TASK-092)
+      # TASK-092: guards against Sub-test B
+      # (stop_from_handler_deadlocks_as_documented) silently reporting
+      # [SKIP] instead of exercising the DR-008 deadlock contract -- mirrors
+      # the "Verify [SKIP] markers fire on tls-no-cli lane (TASK-076)"
+      # grep-the-log pattern.
+      run: |
+        set -euo pipefail
+        cd build
+        LOG=test/stop_from_handler.log
+        if [ ! -f "$LOG" ]; then
+            echo "ERROR: $LOG missing — check-stop-from-handler did not run" >&2
+            exit 1
+        fi
+        if grep -q '^\[SKIP\] stop_from_handler_deadlocks_as_documented' "$LOG"; then
+            echo "ERROR: stop_from_handler_deadlocks_as_documented reported [SKIP] instead of exercising the deadlock contract" >&2
+            cat "$LOG"
+            exit 1
+        fi
+        if ! grep -q '^\[OK\] stop_from_handler' "$LOG"; then
+            echo "ERROR: expected an '[OK] stop_from_handler' marker in $LOG; none found" >&2
+            cat "$LOG"
+            exit 1
+        fi
+        echo "PASS — stop_from_handler_deadlocks_as_documented exercised the DR-008 deadlock contract"
       if: ${{ matrix.os-type == 'ubuntu' && matrix.test-group == 'basic' && matrix.c-compiler == 'gcc' && matrix.debug == 'nodebug' && matrix.linking == 'dynamic' && matrix.build-type == 'classic' }}
 
     - name: Fetch master ref for parallel-install
@@ -1249,7 +1325,7 @@ jobs:
             echo "ERROR: $LOG missing — ws_start_stop did not run" >&2
             exit 1
         fi
-        SKIP_COUNT=$(grep -c '^\[SKIP\].*gnutls-cli' "$LOG" || true)
+        SKIP_COUNT=$(grep -c '^\[SKIP\].*psk_connection' "$LOG" || true)
         echo "Found $SKIP_COUNT [SKIP] markers attributable to gnutls-cli"
         if [ "$SKIP_COUNT" -lt 5 ]; then
             echo "ERROR: expected >=5 [SKIP] markers in $LOG; got $SKIP_COUNT" >&2
@@ -1329,6 +1405,8 @@ jobs:
           if [ -f "test/$log" ]; then
             echo "===== test/$log =====" ;
             cat "test/$log" ;
+          else
+            echo "test/$log not produced by this lane" ;
           fi
         done
       if: ${{ always() && startsWith(matrix.build-type, 'valgrind-') }}

Makefile.am

@@ -43,6 +43,7 @@ EXTRA_DIST = libhttpserver.pc.in $(DX_CONFIG) scripts/extract-release-notes.sh s
 	scripts/check-doxygen.sh scripts/check-hooks-doc-spotcheck.sh \
 	scripts/check-soversion.sh scripts/check-parallel-install.sh \
 	scripts/check-parallel-install-lane.sh \
+	scripts/check-msan-lane.sh scripts/check-valgrind-lane.sh \
 	scripts/check-codeql-workflow.sh scripts/check-workflow-pinning.sh \
 	scripts/test_check_parallel_install.sh \
 	scripts/test_check_parallel_install_lane.sh \
@@ -54,6 +55,7 @@ EXTRA_DIST = libhttpserver.pc.in $(DX_CONFIG) scripts/extract-release-notes.sh s
 	scripts/check-complexity.sh scripts/check-duplication.sh \
 	scripts/check-file-size.sh \
 	scripts/check-warning-suppressions.sh \
+	scripts/test_check_warning_suppressions.sh \
 	scripts/check-server-ready-helper.sh \
 	scripts/test_check_server_ready_helper.sh \
 	scripts/check-no-tautological-asserts.sh \
@@ -556,11 +558,35 @@ lint-fence-balance:
 # `make check-parallel-install` invocation, and no SKIP authorization so
 # environment-quirk SKIPs stay fatal). Parses the workflow file, so it is
 # invoked directly by CI's `lint` lane rather than every check-local run —
-# mirrors the msan/valgrind lane gates.
+# mirrors the msan/valgrind lane gates below.
+# Unlike the newer lint-codeql-workflow/lint-workflow-pinning targets, this
+# target does not run test_check_parallel_install_lane.sh first -- that is
+# the original (TASK-089) idiom; the self-test-then-gate pattern in the
+# newer targets is an intentional TASK-090 enhancement, not a regression to
+# fix here.
 lint-parallel-install-lane:
 	@echo "=== lint-parallel-install-lane: verify parallel-install CI wiring ==="
 	@$(top_srcdir)/scripts/check-parallel-install-lane.sh
 
+# TASK-087: structural gate asserting the msan CI lane in verify-build.yml
+# stays wired (matrix entry present, stale commented-out block gone,
+# MSAN_OPTIONS + scoped TESTS= wiring on the run step). Parses the workflow
+# file, so — like lint-parallel-install-lane — it is invoked directly by
+# CI's `lint` lane rather than every check-local run.
+lint-msan-lane:
+	@echo "=== lint-msan-lane: verify msan CI wiring ==="
+	@$(top_srcdir)/scripts/check-msan-lane.sh
+
+# TASK-088: structural gate asserting the valgrind CI lane in
+# verify-build.yml stays wired (helgrind/drd matrix entries present, the
+# --disable-valgrind-* configure flags gone, per-tool check-valgrind-*
+# targets invoked, helgrind/drd logs surfaced). Parses the workflow file, so
+# — like lint-parallel-install-lane — it is invoked directly by CI's `lint`
+# lane rather than every check-local run.
+lint-valgrind-lane:
+	@echo "=== lint-valgrind-lane: verify valgrind CI wiring ==="
+	@$(top_srcdir)/scripts/check-valgrind-lane.sh
+
 # TASK-090: structural gate asserting codeql-analysis.yml stays hardened
 # (every action SHA-pinned, Autobuild deleted, explicit ./configure + make
 # build, init/analyze on one v3 SHA). The gate parses the workflow with
@@ -582,7 +608,7 @@ lint-workflow-pinning:
 	@$(top_srcdir)/scripts/test_check_workflow_pinning.sh
 	@$(top_srcdir)/scripts/check-workflow-pinning.sh
 
-.PHONY: check-headers check-install-layout check-hygiene check-local check-examples check-readme check-release-notes check-doxygen check-hooks-doc-spotcheck check-soversion check-parallel-install lint-complexity lint-duplication lint-file-size lint-warning-suppressions lint-server-ready-helper lint-server-ready-helper-selftest lint-no-tautological-asserts lint-deprecated-cookie-overload lint-littletest-skip-exit-code lint-parallel-install-skip-contract lint-fence-balance lint-parallel-install-lane lint-codeql-workflow lint-workflow-pinning
+.PHONY: check-headers check-install-layout check-hygiene check-local check-examples check-readme check-release-notes check-doxygen check-hooks-doc-spotcheck check-soversion check-parallel-install lint-complexity lint-duplication lint-file-size lint-warning-suppressions lint-server-ready-helper lint-server-ready-helper-selftest lint-no-tautological-asserts lint-deprecated-cookie-overload lint-littletest-skip-exit-code lint-parallel-install-skip-contract lint-fence-balance lint-parallel-install-lane lint-msan-lane lint-valgrind-lane lint-codeql-workflow lint-workflow-pinning
 
 # TASK-039: top-level convenience rule that descends into test/ to
 # build and run the bench binaries (see test/Makefile.am and

RELEASE_NOTES.md

@@ -86,14 +86,16 @@ v1.x is end-of-life on the day v2.0 ships.
 - **`create_webserver::validator(...)` dispatch semantics — confirmed
   never wired (TASK-078).** The `validator` builder method and the
   `validator_ptr` callback typedef on `create_webserver` are retained
-  as inert v1 surface — the callback has not been invoked from the
-  dispatch path since commit `9163a4f` (Jan 2013, "Eliminated unescaper
-  and validator delegates"). v2 ships the surface unchanged for source
-  compatibility, but a configured validator callback is dead code. The
+  unchanged — the callback has not been invoked from the dispatch path
+  since commit `9163a4f` (Jan 2013, "Eliminated unescaper and validator
+  delegates"). v2 ships the surface for source compatibility, but a
+  configured validator callback is dead code. The
   v2 replacement is `webserver::add_hook(hook_phase::request_received,
   ...)` returning `hook_action::respond_with(http_response)` to reject
-  a request, or `hook_phase::accept_decision` for a connection-scoped
-  veto (see `specs/architecture/04-components/hooks.md`). The legacy
+  a request, or `webserver::block_ip`/`unblock_ip` for a connection-scoped
+  veto; `hook_phase::accept_decision` is observation-only (Short-circuit:
+  no, per `specs/architecture/04-components/hooks.md`) and cannot reject
+  a connection. The legacy
   `validator_builder` integ test in `test/integ/basic.cpp` exercised
   no validation behaviour (it only asserted the server booted with a
   validator set) and has been removed; the equivalent compile-time

docs/debug-env-vars.md

@@ -58,6 +58,19 @@ Multiple `webserver` instances constructed in the same process share a
 process-wide print-once flag, so the warning is emitted exactly once
 per process.
 
+**stdout/stderr are independent streams.** The request-body dump goes to
+`stdout`; the `SECURITY WARNING` above goes to `stderr`. Process
+supervisors, container log aggregators, and the systemd journal may
+capture these two streams separately (different files, different log
+sinks, or different retention policies) rather than interleaving them
+into one ordered log. If only `stdout` is retained/shipped, the leaked
+credential/PII lines can end up with no accompanying warning context;
+if only `stderr` is retained, the warning fires with no visible dump to
+correlate it against. Operators enabling this variable must ensure both
+streams are captured together (e.g. `2>&1` redirection, or a supervisor
+configured to merge/correlate them) so the warning and the dump it
+describes stay associated.
+
 ### How to disable
 
 ```sh

examples/centralized_authentication.cpp

@@ -64,7 +64,7 @@ namespace {
 // mirrors OpenSSL's CRYPTO_memcmp, which also requires equal lengths).
 bool constant_time_equal(std::string_view a, std::string_view b) {
     if (a.size() != b.size()) return false;
-    unsigned char diff = 0;
+    volatile unsigned char diff = 0;
     for (std::size_t i = 0; i < a.size(); ++i) {
         diff |= static_cast<unsigned char>(a[i] ^ b[i]);
     }

examples/clf_access_log.cpp

@@ -75,7 +75,10 @@ std::string sanitize_clf(std::string_view sv) {
     std::string out;
     out.reserve(sv.size());
     for (unsigned char c : sv) {
-        out += (c < 0x20 || c == 0x7f) ? '-' : static_cast<char>(c);
+        // '"' is also replaced: an unescaped double-quote inside a CLF
+        // quoted field ("METHOD PATH VERSION") would prematurely close
+        // the quoted-string and corrupt the log line's structure.
+        out += (c < 0x20 || c == 0x7f || c == '"') ? '-' : static_cast<char>(c);
     }
     return out;
 }
@@ -102,8 +105,8 @@ void emit_clf_line(const hs::response_sent_ctx& ctx) {
     // read from ctx.request->get_version() (TASK-018). It is sanitized
     // like method and path for defence-in-depth even though it comes from
     // MHD's request-line parse.
-    const std::string method  = sanitize_clf(ctx.request->get_method());
-    const std::string path    = sanitize_clf(ctx.request->get_path());
+    const std::string method = sanitize_clf(ctx.request->get_method());
+    const std::string path = sanitize_clf(ctx.request->get_path());
     const std::string version = sanitize_clf(ctx.request->get_version());
     std::printf("- - - [%s] \"%s %s %s\" %d %zu %lld\n",
                 ts,