Skip to content

Nix flake with static analysis, fuzzing and VM tests, plus a few small fixes - #6

Merged
troglobit merged 24 commits into
kernelkit:mainfrom
randomizedcoder:nix-flake
Oct 20, 2026
Merged

Nix flake with static analysis, fuzzing and VM tests, plus a few small fixes#6
troglobit merged 24 commits into
kernelkit:mainfrom
randomizedcoder:nix-flake

Conversation

@randomizedcoder

@randomizedcoder randomizedcoder commented Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hi,

Thanks for mcd, it's a really interesting project. It was a pleasure to read, and easy to get going with.

We took a closer look and added a Nix flake under nix/. It gives a reproducible dev shell, native and cross builds (aarch64, armv7l, riscv64, ppc64le) and OCI images. It also wraps static analysis (clang-tidy, cppcheck, flawfinder, scan-build, gcc -fanalyzer, semgrep) and libFuzzer harnesses for the IGMP/MLD, config, IPC and bridge parsers. On top of that it runs the real make check suite as root inside a KVM microVM, under ASan/UBSan and valgrind too, and adds a three-VM querier/snooping topology test. nix/README.md explains how it all fits together.

Along the way we found a few small things. Each fix is its own commit and comes with a test that failed before it: a test/*.sh case, a fuzz regression input or a C unit test.

Parsing and memory safety (found by fuzzing)

  • Config lexer overflowed s1 on a long dotted-quad token: f93a0fe
  • Config lexer read past its line buffer on a 1023-byte line: 230350a
  • chomp() read before the buffer on all-newline input, reachable from any IPC client: 77b2d33
  • NULL dereference on a truncated bridge mdb show line: a9e20e6
  • populate() port list overflowed for a group on more than about 30 ports: 1f0a157
  • bridge_prop() read past its 256 JSON tokens: 3659ab0
  • bridge_prop() wrote its NUL one byte past the buffer on 8 KiB or more of JSON: eec2f0f
  • Misaligned IP header overlays in the packet buffers: 8a8ae57

Daemon and IPC behaviour (found in the microVM runs)

  • IPC error replies went to the listening socket instead of the client: 5c5da3c
  • mcd died of SIGPIPE when an IPC client hung up early: 8b73dbc
  • The default router timeout ignored the configured query intervals: 64ea2da

Found during static-analysis review

  • Shell command injection through bridge names in the router-ports query (bridge names come from the admin, so it's low risk, but now they're quoted): f441ff8
  • Timer expiry left at tv_nsec = 1000000000 at a whole-second clock reading, so timers could stop: 01289b1
  • mctl overflowed its argv on a daemon command of more than 32 words: 3044603
  • mcd exited 0, without running, when it couldn't fork to daemonize: ab4661f
  • populate() had an unused counter that blocked -Werror: 0783ac8

Test-suite robustness

  • More generous timeouts for IPC replies and tshark startup, which were flaky on slower machines: ffafcbe, 225de23

The Nix side is self-contained under flake.nix and nix/, and it doesn't change the autotools build. The series is the fixes first, a ChangeLog update, then five self-contained Nix commits.

Thanks again!

🤖 Generated with Claude Code

troglobit
troglobit reviewed Oct 19, 2026
View reviewed changes

@troglobit troglobit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Dave, thank you, this is a great first contribution! The fixes are spot on, and I really like that each one comes with a test that fails without it. All 13 tests pass here, and the new ones fail on main like they should.

The Nix bits can stay in this PR, no need to split it. A few things before I merge:

  • Please drop the Co-Authored-By: Claude trailers and sign off the commits yourself instead, e.g. git rebase --signoff main.
  • Some commits in the Nix series rework earlier ones (ea7b8fd, 688048b, fc58fcd, ad32152, ...). Could you squash those into a few self-contained commits?
  • Please add ChangeLog.md entries for the user-visible fixes: router-ports with hyphenated bridge names, IPC error replies reaching the client, mcd surviving clients that hang up, the router timeout default, etc.
  • Small nit: a few new code comments explain the change rather than the code. One line, or none, is plenty. The reasoning belongs in the commit messages, which are already good.

Thanks again!

@randomizedcoder
Fix chomp() reading before the buffer on all-newline input
77b2d33 
chomp() walked back over trailing newlines with no lower bound: for a
string of only newlines it cleared str[0], stepped to str - 1 and read
it.  Any IPC client could trigger this by sending "\n".  Count down a
length instead.

Regression input: regress-chomp-all-newlines (fuzz_ipc), which made
checks.fuzz-replay fail before this fix.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
test: quote tenacious()'s command, give tshark 30 s to start
ffafcbe 
tenacious() ran its command as an unquoted $@, so the capture wait ran
`grep -q Capturing on <log>`: a grep for "Capturing" in a file named
"on" and the log.  It only worked because grep -q exits 0 on a match
despite the error.  Run it as "$@".

On a loaded host tshark can take more than 10 s to start, failing every
test that captures; the wait returns as soon as it is up, so allow 30 s.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix config lexer overflowing s1 on a long dotted-quad token
f93a0fe 
yylex() scanned any run of digits and dots with sscanf("%[.0-9]") into
the global s1, INET_ADDRSTR_LEN (46, or 16 without IPv6) bytes, with no
width: a long number in mcd.conf wrote past it.  Scan into a local
16-byte buffer with a width of 15; nothing longer is an IPv4 address,
and such tokens still fall through to NUMBER/STRING as before.

Regression input: regress-long-dotted-quad (fuzz_cfparse), which made
checks.fuzz-replay fail before this fix.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix config lexer reading past its line buffer
230350a 
yytoken() NUL-terminated each token by writing over the separator and
stepping past it.  A token that ran to the end of buf (a 1023-byte line
with no newline, or a last line without one) had no separator: p was
left one past the buffer and the next call read it.  Only step over a
real separator.

Regression input: regress-full-line-no-newline (fuzz_cfparse), which
made checks.fuzz-replay fail before this fix.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix NULL dereference on a truncated bridge mdb show line
a9e20e6 
populate() read the value after each dev/port/grp/vid keyword with
strtok() and passed it straight to strlcpy() or strtol(); a line ending
right after a keyword handed them NULL.  Stop parsing the line there.

Regression inputs: regress-mdb-line-ends-after-keyword and
regress-mdb-line-ends-after-vid (fuzz_bridge_mdb), which made
checks.fuzz-replay fail before this fix.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix mdb port list overflow for a group on many ports
1f0a157 
populate() strcat()ed every port of a group into the 576-byte
struct mdb port list unchecked: past about 30 ports (or fewer with
JSON quotes and long names) it wrote off the end of the heap entry.
Append each port whole if it fits, else warn and leave it out; a name
cut short would also break the JSON output.

Regression input: regress-mdb-many-ports-one-group (fuzz_bridge_mdb),
40 ports in one group, which made checks.fuzz-replay fail before this
fix.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Drop populate()'s unused counter
0783ac8 
gcc 16 flags `num` in src/bridge.c populate() as set but never used,
which was the only thing keeping a -Werror build from passing.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix IPC error replies going to the listening socket
5c5da3c 
ipc_handle() passed the listening socket sd to ipc_err() instead of the
accepted client, so a client sending an unknown command got no reply
(and mcd wrote to a socket that cannot carry data).  Reply on client,
and keep errno across the warn() before it, since ipc_err() picks its
message from errno.

New test/ipc-errors.sh: a table of bad commands, each expecting "No such
command", with control rows that must still be served.  It failed
before this fix: every bad command got an empty reply.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix mcd dying of SIGPIPE when an IPC client hangs up early
8b73dbc 
mcd did not ignore SIGPIPE, so a reply write() to a client that had
already closed its end killed the daemon (exit 141).  socat waits only
0.5 s for a reply, so a slow `show` on a loaded host was enough.  Send
replies with MSG_NOSIGNAL, through ipc_write() everywhere: the failed
write then returns EPIPE, which is already handled.  The signal stays
at its default for the bridge/jq children run by popen().

New test/ipc-disconnect.sh stops mcd while a client sends a command and
gives up, then checks mcd is alive and still answers, for each reply
path (help, version, show, json show, error).  It failed before this
fix: mcd died replying to the first one.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
test: give IPC clients 10 s for a reply
225de23 
socat hangs up 0.5 s after sending the command; on a loaded host, or
with mcd under valgrind, `show igmp json` (which runs bridge and jq)
takes longer, and the test reads an empty reply.  mcd closes the
connection after replying, so a longer wait costs nothing when it is
fast.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix default router timeout ignoring the configured intervals
64ea2da 
The router timeout (other querier present interval) defaults, per RFC
3376 and mcd.conf(5), to robustness * query-interval +
response-interval / 2.  igmp_init() computed it before the config was
read, so it was always 2 * 125 + 10 / 2 = 255 whatever
global-query-interval, global-response-interval and robustness said.
Leave it unset there and compute it in config_init() once the file is
parsed, unless router-timeout was given.

New test/router-timeout.sh: a table of global settings, each with the
timeout `show igmp` must report.  It failed before this fix (255 for
every derived row); the explicit-setting rows passed both ways.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix misaligned IP headers in the packet buffers
8a8ae57 
mcd overlays struct ip / struct ip6_hdr right after the Ethernet header,
14 bytes (18 with a VLAN tag) into send_buf and recv_buf, so every query
built and every packet parsed accessed 4-byte fields misaligned:
harmless on x86, a fault or a slow trap on strict-alignment targets.
Start frames SOCK_BUF_ALIGN (2) bytes into each buffer, as the kernel's
NET_IP_ALIGN does, which aligns the IP header in both cases.

With -fsanitize=alignment, checks.fuzz-replay failed at build_ipv4()
and vm-check-asan reported 302 misaligned accesses; both are clean now.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix bridge_prop() reading past its JSON tokens
3659ab0 
jsmn is built non-strict, so an object's size counts keys that have no
value, e.g. {"a" "b"} has size 2.  The key/value walk trusted size * 2
tokens to follow the object, and on an object ending the token array it
read tokens[256] and tokens[257] past the 256 parsed.  Found by fuzz-run
(fuzz_bridge_mdb, UBSan index out of bounds).

Bound the walk by the number of parsed tokens too.  The regression
input, regress-json-keys-without-values, fails fuzz-replay without the
fix.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix bridge_prop() writing its NUL past the JSON buffer
eec2f0f 
bridge_prop() reads up to BUFSIZ bytes of `bridge -j link | jq` output
into a BUFSIZ buffer and then terminates it at data[len], one byte past
the end when the output fills the buffer, e.g. a bridge with many ports.
Found in review; libFuzzer's default max input length (4096) is too
short to get there.

Allocate the terminator's byte.  The regression input,
regress-json-fills-read-buffer, fails fuzz-replay without the fix.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix shell command injection through bridge names
f441ff8 
bridge_router_ports() pasted the bridge name unquoted into the shell
pipeline it popen()s, twice: into the jq filter and as the `bridge vlan
global show dev` argument.  The names come from the kernel (`ip link
show type bridge`), and Linux allows ; $ ` | & > and ' in interface
names, so creating a bridge named e.g. `b;>pwned` ran a command as root
mcd the next time an IPC client asked for `show router-ports` or `show
igmp json`.  Creating a bridge takes only CAP_NET_ADMIN in mcd's network
namespace.  Found reviewing analysis-deep's popen() findings.

Pass the name to jq with --arg and shell quote it.  This also fixes the
router ports of bridges named like br-0, never listed: jq parsed
.router.br-0 as a subtraction.

test/bridge-names.sh creates a bridge per hostile name, each with a
permanent router port, and fails if any name ran as a command or a
bridge lacks its router port in `show router-ports json` and `show igmp
json`.  Before the fix all six hostile names ran and br-ok had no ports.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix timer expiry left unnormalized on a whole second
01289b1 
timer_run() carried the expiry's tv_nsec into tv_sec only when it was
above 1000000000, so an expiry landing exactly on a second kept tv_nsec
at 1000000000.  If the clock then read exactly that next second when
timer_start() armed the itimer, it computed tv_usec 1000000, setitimer()
failed with EINVAL, and as the return value is ignored no timer fired
again.  Very unlikely in practice; found reviewing analysis-deep's
clang-tidy findings in pev.c.

Carry on >= 1000000000.

The Nix checks.unit test_pev #includes pev.c, wraps clock_gettime()
and setitimer() to feed chosen clock readings, and checks each timer's
expiry and the itimer armed from it.  Before the fix both rows landing
on a whole second failed, and so did arming at the next second.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix mctl overflowing its argv on a long daemon command
3044603 
mctl builds its command table from the daemon's 'help' reply, one word
per argument, into a struct cmd with room for MAXARGS (32).  add_cmd()
never checked, so a command of more than 32 words ran past argv[] into
the rest of the heap allocated struct; at a few hundred words mctl
segfaulted.  The reply comes from whatever answers on the socket mctl
connects to (-u / MCD_SOCK).  Found reading add_cmd() while reviewing
analysis-deep's clang-analyzer leak report for ipc_fetch().

Stop at MAXARGS words, with a warning.

test/mctl-help.sh runs mctl -h against a fake daemon (socat) answering
with commands of 1, 2, 32, 33 and 300 words, each followed by a
'version' command mctl must still list.  Before the fix the 300 word row
crashed mctl (exit 139); vm-check-asan is clean with it.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Fix mcd exiting 0 when it cannot fork to daemonize
ab4661f 
daemonize took any non-zero fork() return as being the parent, so when
fork() failed (-1, e.g. EAGAIN at the process limit, ENOMEM) the only
process exited 0: mcd never ran, and its supervisor saw success.  Found
reading the daemonize code behind analysis-deep's gcc-analyzer dup2()
reports.

Exit 1 with an error when fork() fails, and leave the terminal only in
the child.

The Nix checks.unit test_daemonize runs the built mcd under an
LD_PRELOAD shim whose fork() fails, and whose geteuid() says root to
pass mcd's root check in the sandbox.  Before the fix both fork failure
rows exited 0.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
Update ChangeLog with the fixes since v3.0
4e808fb 
Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
nix: add flake with dev shell, native/cross builds and OCI images
b8a02b6 
A slim flake.nix wires single-purpose modules under nix/, sharing one
context from nix/context.nix: the version read from configure.ac, the
runtime configure paths (/etc, /run, /var), and lib.fileset sources
split so that a test script or doc edit rebuilds only what reads it.

- package.nix: autotools build of mcd + mctl as one callPackage
  function; mcd-clang and mcd-werror are .overrides of it
- cross.nix: aarch64, armv7l, riscv64 and ppc64le via
  pkgsCross.<target>.callPackage
- oci.nix: streamLayeredImage, minimal and debug, per arch, with a slim
  iproute2 + jq
- devshell.nix: build, test-suite and debug tools, plus mcd-configure,
  mcd-build, mcd-check, mcd-clean and mcd-help as writeShellApplications;
  mcd-help's targets come from nix/targets.nix

Checks: build-gcc, build-clang and build-werror.
Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
nix: add static analysis with triage, and C unit tests
46ebd04 
Per-tool report derivations (clang-tidy, cppcheck, flawfinder, semgrep,
scan-build, gcc warnings, gcc -fanalyzer) and quick/standard/deep
aggregates that run the triage prioritiser, adapted from rocm-ernic for
autotools:

- the compile DB is recorded with bear around a real out-of-tree build,
  so it includes the bison-generated cfparse.c
- build-driven tools configure by absolute path in _build/, so every
  diagnostic strips to a repo-relative src/ path
- each runner fails when its run is incomplete instead of reporting 0
- a new semgrep rule, strtok-result-unchecked, with fixtures

Every report is a package, analysis-<tool> or analysis-<level>.

checks.unit holds table-driven tests of mcd internals built with gcc +
ASan/UBSan: test_pev (#includes pev.c, wraps clock_gettime() and
setitimer() to check timer expiry and the itimer armed from it) and
test_daemonize (mcd under an LD_PRELOAD shim whose fork() fails).

Checks: analysis-runners (table-driven tests of the triage parsers,
filters and semgrep-report.jq), semgrep-rules and unit.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
nix: add libFuzzer harnesses for packet, config, IPC and bridge parsing
9440716 
Six harnesses (igmp, mld, cfparse, ipc, inet_parse, bridge_mdb) link
against the objects of a real out-of-tree autotools build, with
main/pev/sock replaced by fuzz/stubs.c: a fireable timer table, captured
sends, and link-time wraps of exit() (fatal paths longjmp back) and
popen()/pclose() (command output comes from the fuzz input).

Seed corpora are generated by gen_seeds.py in their own derivation:
checksummed IGMP/MLD packets and multi-packet sequences, mcd.conf
variants, every IPC command, canned bridge/ip output, and a REGRESSIONS
table with one input per fixed bug.  Each harness is its own derivation,
so an edit rebuilds and reruns only the harnesses it touches.

- .#fuzz                  harnesses + corpora + run-fuzzers
- .#fuzz-run              20 s per harness in the sandbox, crashes +
                          symbolized repro; also fuzz-run-<harness>
- checks.fuzz-replay      gcc + ASan/UBSan build replaying every seed

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
nix: run the test suite in microVMs, and a three-VM topology test
8ec02aa 
nix run .#vm-check boots a NixOS microVM (qemu_kvm, the host store over
9p, serial console on TCP) and runs `make check` from a kept
--enable-test build tree, driven over the serial console by expect.
vm-check-asan and vm-check-valgrind do the same against an ASan + UBSan
tree and with mcd wrapped in valgrind, and fail on any finding in the
test logs.

nix run .#vm-topo: a switch VM bridges two host VMs over QEMU dgram
unix-socket links; mcd on the switch is checked for querier election,
IGMPv3/MLDv2 general queries and their interval, snooped joins and
leaves in the mdb, losing the election to a lower-addressed rival mcd,
taking over again when the rival goes away, and the router timeout
following the configured intervals.

The runner (runner.sh) boots any number of VMs from one JSON config,
tails their consoles, and maps the outcome to an exit code: 0 pass,
1 test failure, 2 no console, 3 setup, 124 timeout.  The expect library
and drivers are ported from partybus, and checks.vm-lib-exp tests them
hermetically against a bash pty, including make-check.exp against fake
build trees.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder
nix: add nix/README.md, and check mcd-help's targets against it
5037c41 
nix/README.md documents the flake: the dev shell and builds, static
analysis and triage, fuzzing, the microVM tests, how the sources are
split so edits rebuild little (with measured rebuild counts), a module
map, and the bugs found so far with the test that covers each.

checks.help-targets fails if a target mcd-help lists (nix/targets.nix)
is not a flake output or is not mentioned in nix/README.md.

Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
@randomizedcoder

randomizedcoder commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! All done:

  • Commits are signed off, and the Co-Authored-By trailers are gone.
  • The Nix series is squashed into five self-contained commits after the fixes, and each one passes nix flake check on its own.
  • There's a ChangeLog entry under UNRELEASED.
  • I trimmed the new comments down to one line or none.

@troglobit

troglobit commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Awesome, looks good, thanks!

@troglobit
troglobit merged commit d9dfe71 into kernelkit:main Oct 20, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@troglobit troglobit troglobit left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@randomizedcoder @troglobit