From a036a6fd14c3338ab95a2bf72e66ec1a2633e262 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 14 Aug 2026 21:36:28 -0600 Subject: [PATCH 01/30] build(nix): keep debug outputs lean Debug binaries retained the complete Rust toolchain through their standard-library source paths, adding roughly 2.4 GB to the closure. They also carried a sizable DWARF index that neither packaged debugger consumes. Point those paths at the much smaller rust-src component, omit unused prebuilt documentation, and remove .debug_names. Source browsing and symbols remain available while the resulting diagnostic images become practical to store and transfer. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 7b2f63f212c54af94df96e0db0b87f6f2ed24131) --- default.nix | 9 +++++++++ nix/overlays/llvm.nix | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/default.nix b/default.nix index 301dba5408..c1f0f0e509 100644 --- a/default.nix +++ b/default.nix @@ -431,6 +431,8 @@ let # Keep debug paths stable across revisions. Source readers # must resolve this relative prefix from the workspace root. "--remap-path-prefix==${src-prefix}" + # Keep debug outputs from retaining the complete Rust toolchain. + "--remap-path-prefix=${pkgs.rust-toolchain}/lib/rustlib/src/rust=${pkgs.rust-toolchain.passthru.availableComponents.rust-src}/lib/rustlib/src/rust" ] ) else @@ -470,6 +472,13 @@ let mkdir -p $debug/bin for f in $out/bin/*; do mv "$f" "$debug/bin/$(basename "$f")" + # Trade index for size. gdb has consumed `.debug_names` + # as a real DWARF-5 index since 14, and this ships 17.2, + # so dropping it is not free -- gdb rebuilds an index on + # each start instead. The section is large enough on + # these binaries that the image is worth more than the + # startup, and bugstalker does not read it at all. + ${objcopy} --remove-section=.debug_names "$debug/bin/$(basename "$f")" ${strip} --strip-debug "$debug/bin/$(basename "$f")" -o "$f" ${objcopy} --add-gnu-debuglink="$debug/bin/$(basename "$f")" "$f" done diff --git a/nix/overlays/llvm.nix b/nix/overlays/llvm.nix index 334be96d21..9b7c46d39c 100644 --- a/nix/overlays/llvm.nix +++ b/nix/overlays/llvm.nix @@ -36,7 +36,7 @@ let "clippy" "llvm-tools" "rust-analyzer" - "rust-docs" + # Avoid retaining 680M of unused prebuilt documentation. "rust-src" "rust-std" "rustc" From b1ef45d49d23d21435b796884776935f5c32f3d9 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 14 Aug 2026 21:37:45 -0600 Subject: [PATCH 02/30] feat(debug): add a version-matched core viewer A core collected from the lab is useful only with the exact unstripped binaries and sources that produced it. A general debugging toolbox cannot reconstruct that relationship after the release has moved on. Provide a purpose-built gdb image alongside each build and teach it Rust's standard-library types without retaining rustc. This keeps post-mortem debugging reproducible while avoiding unrelated live-debugging tools. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 0a97d9f5fac135fc9c6315c5bb0e6fe09250d725) --- .github/workflows/README.md | 15 ++++++- .github/workflows/dev.yml | 2 + default.nix | 82 ++++++++++++++++++++++++++----------- justfile | 17 ++++---- 4 files changed, 84 insertions(+), 32 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index d28097881f..0c1ef549ef 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -103,7 +103,8 @@ If those queue failures stop being rare, the phasing is worth revisiting. - Checks: `debug` by default; `release` and `fuzz` on deep runs - Coverage: `debug` by default; `fuzz` on deep runs - Miri: required on deep runs; opt-in on pull requests with `ci:+miri` -- Containers: debug/release for dataplane and FRR; release for validator +- Containers: debug/release for dataplane, its debugger, and FRR; release for + validator - VLAB configurations: spine-leaf fabric mode, L2VNI/L3VNI VPC modes, with gateway enabled @@ -111,6 +112,18 @@ If those queue failures stop being rare, the phasing is worth revisiting. - Container images pushed to GitHub Container Registry (GHCR) - Release containers published on tag pushes via `just push` +- `ghcr.io/githedgehog/dataplane/core-viewer` opens a core file from the lab. + It carries gdb plus the unstripped binaries and sources for the matching + `ghcr.io/githedgehog/dataplane` build. + Pull the tag matching the build the core came from; symbols only line up with + the exact version and profile that produced it. + The entrypoint takes the core as its only argument: + + ```console + docker run --rm -it -v /path/to/cores:/cores \ + ghcr.io/githedgehog/dataplane/core-viewer:TAG /cores/core.1234 + ``` + - Coverage reports from each `coverage/` job, kept for 7 days: - `coverage-html-.tar.gz` - `llvm-cov` HTML report, including the per-branch counts that Codecov does not render. Unpack and open diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 5cb2a33d5c..171e694cfd 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -428,6 +428,8 @@ jobs: nix-target: - frr.dataplane - dataplane + # Must match the build that produced the core. + - dataplane-core-viewer - validator # TODO: enable cfi and safe-stack on release when possible profile: "${{ fromJSON(needs.plan.outputs.container_profiles) }}" diff --git a/default.nix b/default.nix index c1f0f0e509..3313c11f20 100644 --- a/default.nix +++ b/default.nix @@ -971,32 +971,68 @@ let }).overrideAttrs source-volatile; - containers.dataplane-debugger = + # Shared runtime and unstripped binaries for the debugger images. + debug-image-paths = [ + pkgs.pkgsBuildHost.coreutils + pkgs.pkgsBuildHost.bashInteractive + pkgs.pkgsHostHost.dockerTools.usrBinEnv + + pkgs.pkgsHostHost.libc.debug + workspace.cli.debug + workspace.dataplane.debug + workspace.init.debug + ]; + + # Copy Rust's gdb helpers without retaining rustc as a runtime dependency. + rust-gdb-printers = pkgs.runCommand "rust-gdb-printers" { } '' + mkdir -p "$out/lib/rustlib/etc" + for f in gdb_load_rust_pretty_printers.py gdb_lookup.py gdb_providers.py rust_types.py; do + cp -L "${pkgs.rust-toolchain}/lib/rustlib/etc/$f" "$out/lib/rustlib/etc/$f" + done + ''; + + # Opens dataplane core files with matching symbols and sources. + containers.dataplane-core-viewer = (pkgs.dockerTools.buildLayeredImage { - name = "ghcr.io/githedgehog/dataplane/debugger"; + name = "ghcr.io/githedgehog/dataplane/core-viewer"; inherit tag; - contents = pkgs.buildEnv { - name = "dataplane-debugger-env"; - pathsToLink = [ - "/bin" - "/etc" - "/var" - "/lib" - ]; - paths = [ - pkgs.pkgsBuildHost.gdb - pkgs.pkgsBuildHost.rr - pkgs.pkgsBuildHost.coreutils - pkgs.pkgsBuildHost.bashInteractive - pkgs.pkgsBuildHost.iproute2 - pkgs.pkgsBuildHost.ethtool - pkgs.pkgsHostHost.dockerTools.usrBinEnv - - pkgs.pkgsHostHost.libc.debug - workspace.cli.debug - workspace.dataplane.debug - workspace.init.debug + contents = + (pkgs.buildEnv { + name = "dataplane-core-viewer-env"; + pathsToLink = [ + "/bin" + "/etc" + "/var" + "/lib" + ]; + paths = [ + pkgs.pkgsBuildHost.gdb + rust-gdb-printers + ] + ++ debug-image-paths; + }).overrideAttrs + source-volatile; + # gdb needs a writable HOME for logs and its index cache. + extraCommands = '' + # Point `src-prefix` at the sources this image ships. Referencing ${src} + # here is also what keeps it in the image closure: with the remap no + # longer naming a store path, nothing else retains it. + mkdir -p ".$(dirname "${src-prefix}")" + ln -s "${src}" ".${src-prefix}" + mkdir -p tmp + chmod 1777 tmp + ''; + config = { + Entrypoint = [ + "/bin/gdb" + "--directory=/lib/rustlib/etc" + "-iex" + "add-auto-load-safe-path /lib/rustlib/etc" + "-iex" + "source /lib/rustlib/etc/gdb_load_rust_pretty_printers.py" + "/bin/dataplane" ]; + Env = [ "HOME=/tmp" ]; }; }).overrideAttrs source-volatile; diff --git a/justfile b/justfile index 6eed433ff8..408eb0be5f 100644 --- a/justfile +++ b/justfile @@ -114,7 +114,7 @@ oci_insecure := "" oci_name := "githedgehog/dataplane" oci_frr_prefix := "githedgehog/dataplane/frr" oci_image_dataplane := oci_repo + "/" + oci_name + ":" + version -oci_image_dataplane_debugger := oci_repo + "/" + oci_name + "/debugger:" + version +oci_image_dataplane_core_viewer := oci_repo + "/" + oci_name + "/core-viewer:" + version oci_image_dataplane_validator := oci_repo + "/" + oci_name + "/validator:" + version oci_image_frr_dataplane := oci_repo + "/" + oci_frr_prefix + ":" + version oci_image_frr_host := oci_repo + "/" + oci_frr_prefix + "-host:" + version @@ -279,10 +279,10 @@ build-container target="dataplane" *args: (build (if target == "dataplane" { "da docker tag "${img}" "{{oci_image_dataplane}}" echo "imported {{ oci_image_dataplane }} (${docker_platform})" ;; - "dataplane-debugger") - docker load < ./results/containers.dataplane-debugger - docker tag "ghcr.io/githedgehog/dataplane/debugger:{{version}}" "{{oci_image_dataplane_debugger}}" - echo "imported {{ oci_image_dataplane_debugger }}" + "dataplane-core-viewer") + docker load < ./results/containers.dataplane-core-viewer + docker tag "ghcr.io/githedgehog/dataplane/core-viewer:{{version}}" "{{oci_image_dataplane_core_viewer}}" + echo "imported {{ oci_image_dataplane_core_viewer }}" ;; "debug-tools") # Uses nix only to produce a base image with the runtime closure (glibc, bash, etc.) @@ -386,8 +386,8 @@ push-container target="dataplane" *args: (build-container target args) && versio "dataplane") push_image "{{ oci_image_dataplane }}" ;; - "dataplane-debugger") - push_image "{{ oci_image_dataplane_debugger }}" + "dataplane-core-viewer") + push_image "{{ oci_image_dataplane_core_viewer }}" ;; "debug-tools") >&2 echo "do not push the debug tools!" @@ -421,7 +421,8 @@ push-container target="dataplane" *args: (build-container target args) && versio [script] push: {{ _just_debuggable_ }} - for container in dataplane frr.dataplane validator; do + # The core viewer must match the release it inspects. + for container in dataplane dataplane-core-viewer frr.dataplane validator; do if [ "${container}" = "validator" ]; then platform="wasm32-wasip1" else From 74af5d2944c88e05ea09cfc385359ed9468b2313 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 14 Aug 2026 21:38:11 -0600 Subject: [PATCH 03/30] feat(debug): add a live DAP debugger Post-mortem inspection and live debugging need different tools. The core viewer cannot offer an editor-driven session, while bugstalker understands Rust layouts and can expose the running dataplane through the Debug Adapter Protocol. Track bugstalker upstream for its current remote DAP support and package it separately with the matching binaries and sources. Keeping the image single-purpose avoids making every diagnostic artifact carry every debugger. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 0000171941f2b996403488afd8e2952c3e396de0) --- .github/workflows/README.md | 49 ++++++++++++++++++++++++++++++++-- .github/workflows/dev.yml | 2 ++ default.nix | 48 +++++++++++++++++++++++++++++++++ justfile | 9 +++++++ nix/overlays/dataplane-dev.nix | 9 +++++++ npins/sources.json | 16 +++++++++++ 6 files changed, 131 insertions(+), 2 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 0c1ef549ef..9fcce2c5cf 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -103,8 +103,8 @@ If those queue failures stop being rare, the phasing is worth revisiting. - Checks: `debug` by default; `release` and `fuzz` on deep runs - Coverage: `debug` by default; `fuzz` on deep runs - Miri: required on deep runs; opt-in on pull requests with `ci:+miri` -- Containers: debug/release for dataplane, its debugger, and FRR; release for - validator +- Containers: debug/release for dataplane, its two debug images, and FRR; + release for validator - VLAB configurations: spine-leaf fabric mode, L2VNI/L3VNI VPC modes, with gateway enabled @@ -124,6 +124,51 @@ If those queue failures stop being rare, the phasing is worth revisiting. ghcr.io/githedgehog/dataplane/core-viewer:TAG /cores/core.1234 ``` +- `ghcr.io/githedgehog/dataplane/dev-debugger` debugs a live dataplane from an + editor. It carries bugstalker, which understands Rust's std collections and + enum layouts, and listens for a Debug Adapter Protocol client on port 4711. + Publish the port and point the editor's DAP client at it: + + ```console + docker run --rm -p 127.0.0.1:4711:4711 ghcr.io/githedgehog/dataplane/dev-debugger:TAG + ``` + + Connecting does not by itself start anything. In remote-DAP mode bugstalker + waits for the client's `launch` request to name the program, so the editor + has to send `program`, and any dataplane arguments as `args`. A request + without `program` is rejected with `launch: missing arguments.program`. + For VS Code, in `.vscode/launch.json`. `type` has to match whatever debug + type the BugStalker extension you installed registers -- it is not a name we + choose, and it differs between extensions, so check the one you have rather + than copying this field blind: + + ```json + { + "type": "bs", + "request": "launch", + "name": "dataplane (container)", + "debugServer": 4711, + "program": "/bin/dataplane", + "args": [] + } + ``` + + For `nvim-dap`, where the first line names the adapter itself, so `type = "bs"` + below is our own label rather than an extension's: + + ```lua + dap.adapters.bs = { type = "server", host = "127.0.0.1", port = 4711 } + dap.configurations.rust = { + { + type = "bs", + request = "launch", + name = "dataplane (container)", + program = "/bin/dataplane", + args = {}, + }, + } + ``` + - Coverage reports from each `coverage/` job, kept for 7 days: - `coverage-html-.tar.gz` - `llvm-cov` HTML report, including the per-branch counts that Codecov does not render. Unpack and open diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 171e694cfd..5d4f972c06 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -430,6 +430,8 @@ jobs: - dataplane # Must match the build that produced the core. - dataplane-core-viewer + # Live debugging of the same build, driven from an editor over DAP. + - dataplane-dev-debugger - validator # TODO: enable cfi and safe-stack on release when possible profile: "${{ fromJSON(needs.plan.outputs.container_profiles) }}" diff --git a/default.nix b/default.nix index 3313c11f20..74ad7468d3 100644 --- a/default.nix +++ b/default.nix @@ -1037,6 +1037,54 @@ let }).overrideAttrs source-volatile; + # Exposes bugstalker's DAP server for live debugging. + containers.dataplane-dev-debugger = + (pkgs.dockerTools.buildLayeredImage { + name = "ghcr.io/githedgehog/dataplane/dev-debugger"; + inherit tag; + contents = + (pkgs.buildEnv { + name = "dataplane-dev-debugger-env"; + pathsToLink = [ + "/bin" + "/etc" + "/var" + "/lib" + ]; + paths = [ pkgs.pkgsBuildHost.bugstalker ] ++ debug-image-paths; + }).overrideAttrs + source-volatile; + # bugstalker needs a writable HOME for its keymap and history. + extraCommands = '' + # Point `src-prefix` at the sources this image ships. Referencing ${src} + # here is also what keeps it in the image closure: with the remap no + # longer naming a store path, nothing else retains it. + mkdir -p ".$(dirname "${src-prefix}")" + ln -s "${src}" ".${src-prefix}" + mkdir -p tmp + chmod 1777 tmp + ''; + config = { + Entrypoint = [ + "/bin/bs" + # Bind the published interface rather than container-local loopback. + "--dap-remote=0.0.0.0:4711" + # rustc is absent, so bugstalker cannot infer this path. + "--std-lib-path=${pkgs.rust-toolchain.passthru.availableComponents.rust-src}/lib/rustlib/src/rust" + # No debuggee here on purpose. In `--dap-remote` mode bugstalker + # ignores the CLI debuggee and waits for the client's `launch` request + # to name one, so a path here would be silently dead and would imply + # that connecting alone starts the dataplane. The editor supplies + # `program` instead; see .github/workflows/README.md. + ]; + Env = [ "HOME=/tmp" ]; + ExposedPorts = { + "4711/tcp" = { }; + }; + }; + }).overrideAttrs + source-volatile; + debug-tools = pkgs: [ diff --git a/justfile b/justfile index 408eb0be5f..07f78247ec 100644 --- a/justfile +++ b/justfile @@ -115,6 +115,7 @@ oci_name := "githedgehog/dataplane" oci_frr_prefix := "githedgehog/dataplane/frr" oci_image_dataplane := oci_repo + "/" + oci_name + ":" + version oci_image_dataplane_core_viewer := oci_repo + "/" + oci_name + "/core-viewer:" + version +oci_image_dataplane_dev_debugger := oci_repo + "/" + oci_name + "/dev-debugger:" + version oci_image_dataplane_validator := oci_repo + "/" + oci_name + "/validator:" + version oci_image_frr_dataplane := oci_repo + "/" + oci_frr_prefix + ":" + version oci_image_frr_host := oci_repo + "/" + oci_frr_prefix + "-host:" + version @@ -284,6 +285,11 @@ build-container target="dataplane" *args: (build (if target == "dataplane" { "da docker tag "ghcr.io/githedgehog/dataplane/core-viewer:{{version}}" "{{oci_image_dataplane_core_viewer}}" echo "imported {{ oci_image_dataplane_core_viewer }}" ;; + "dataplane-dev-debugger") + docker load < ./results/containers.dataplane-dev-debugger + docker tag "ghcr.io/githedgehog/dataplane/dev-debugger:{{version}}" "{{oci_image_dataplane_dev_debugger}}" + echo "imported {{ oci_image_dataplane_dev_debugger }}" + ;; "debug-tools") # Uses nix only to produce a base image with the runtime closure (glibc, bash, etc.) # then layers locally-compiled cargo binaries on top via Dockerfile. @@ -389,6 +395,9 @@ push-container target="dataplane" *args: (build-container target args) && versio "dataplane-core-viewer") push_image "{{ oci_image_dataplane_core_viewer }}" ;; + "dataplane-dev-debugger") + push_image "{{ oci_image_dataplane_dev_debugger }}" + ;; "debug-tools") >&2 echo "do not push the debug tools!" exit 1 diff --git a/nix/overlays/dataplane-dev.nix b/nix/overlays/dataplane-dev.nix index c8829018f2..7b0074591a 100644 --- a/nix/overlays/dataplane-dev.nix +++ b/nix/overlays/dataplane-dev.nix @@ -21,6 +21,15 @@ in opengrep = final.callPackage ../pkgs/opengrep { src = sources.opengrep; }; + # cargoDeps must be fetched from the overridden source too. + bugstalker = prev.bugstalker.overrideAttrs (orig: { + version = final.lib.removePrefix "v" sources.bugstalker.version; + src = sources.bugstalker; + cargoDeps = prev.rustPlatform.fetchCargoVendor { + src = sources.bugstalker; + hash = "sha256-GGi5hnrK5WpvnXHNckpsBch/SJ4lDvH7peSlrCdk218="; + }; + }); cargo-bolero = prev.cargo-bolero.override { inherit (override-packages) rustPlatform; }; cargo-deny = prev.cargo-deny.override { inherit (override-packages) rustPlatform; }; cargo-edit = prev.cargo-edit.override { inherit (override-packages) rustPlatform; }; diff --git a/npins/sources.json b/npins/sources.json index 369e701a56..fde579c3b7 100644 --- a/npins/sources.json +++ b/npins/sources.json @@ -16,6 +16,22 @@ "url": "https://api.github.com/repos/KaTeX/KaTeX/tarball/refs/tags/v0.18.3", "hash": "sha256-FZpiUhKFI2GAZmr667gA5yHFezAoRcAADoS0+NrNsJQ=" }, + "bugstalker": { + "type": "GitRelease", + "repository": { + "type": "GitHub", + "owner": "godzie44", + "repo": "BugStalker" + }, + "pre_releases": false, + "version_upper_bound": null, + "release_prefix": null, + "submodules": false, + "version": "v0.4.7", + "revision": "9c18e546eca6a1d68ef69da340a6fb0f2bc1bab7", + "url": "https://api.github.com/repos/godzie44/BugStalker/tarball/refs/tags/v0.4.7", + "hash": "sha256-AAeSvy/rvyylPH2jTVBGN95QIc6gumREQYuruhRo2ZI=" + }, "crane": { "type": "GitRelease", "repository": { From 43ad40118ac8bfbd2724f6e2fa7c6a5c0f3416a1 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 14 Aug 2026 21:38:36 -0600 Subject: [PATCH 04/30] feat(debug): add a syscall tracer image Some failures need a record of the dataplane's kernel interactions rather than an interactive debugger. A small, repeatable tracing environment is easier to deploy and feed into existing log analysis than a general-purpose toolbox. Package lurk around the matching release binaries and follow the worker threads where the dataplane does its work. Because syscall tracing needs no symbols, this image can stay much smaller than the debugger images. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit c00aff1a493d0ac7ce8840e277ffc09fc5df20d4) --- .github/workflows/README.md | 22 +++++++++++++++- .github/workflows/dev.yml | 2 ++ default.nix | 47 ++++++++++++++++++++++++++++++++++ justfile | 9 +++++++ nix/overlays/dataplane-dev.nix | 27 +++++++++++++++++++ 5 files changed, 106 insertions(+), 1 deletion(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 9fcce2c5cf..a036099f08 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -103,7 +103,7 @@ If those queue failures stop being rare, the phasing is worth revisiting. - Checks: `debug` by default; `release` and `fuzz` on deep runs - Coverage: `debug` by default; `fuzz` on deep runs - Miri: required on deep runs; opt-in on pull requests with `ci:+miri` -- Containers: debug/release for dataplane, its two debug images, and FRR; +- Containers: debug/release for dataplane, its three debug images, and FRR; release for validator - VLAB configurations: spine-leaf fabric mode, L2VNI/L3VNI VPC modes, with gateway enabled @@ -169,6 +169,26 @@ If those queue failures stop being rare, the phasing is worth revisiting. } ``` +- `ghcr.io/githedgehog/dataplane/syscall-tracer` records what the dataplane + asks the kernel for, as JSON, using lurk. + It carries the same stripped binaries the release image ships, since nothing + here symbolizes, so it is smaller than the other two -- though not by as much + as that suggests: like them it ships the source tree, which the entrypoint + makes the working directory. Only the debug symbols and the debuggers + themselves are absent. + + ```console + docker run --rm ghcr.io/githedgehog/dataplane/syscall-tracer:TAG > trace.jsonl + ``` + + The stream is one JSON object per line, except that tracing child threads + makes lurk announce each one with a bare `Attaching to child ` line. + Filter those out if the consumer needs strict JSONL: + + ```console + jq -R 'fromjson? // empty' < trace.jsonl + ``` + - Coverage reports from each `coverage/` job, kept for 7 days: - `coverage-html-.tar.gz` - `llvm-cov` HTML report, including the per-branch counts that Codecov does not render. Unpack and open diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 5d4f972c06..915f57be6a 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -432,6 +432,8 @@ jobs: - dataplane-core-viewer # Live debugging of the same build, driven from an editor over DAP. - dataplane-dev-debugger + # Syscall trace of the same build, as JSON. + - dataplane-syscall-tracer - validator # TODO: enable cfi and safe-stack on release when possible profile: "${{ fromJSON(needs.plan.outputs.container_profiles) }}" diff --git a/default.nix b/default.nix index 74ad7468d3..6f6345a8f3 100644 --- a/default.nix +++ b/default.nix @@ -1085,6 +1085,53 @@ let }).overrideAttrs source-volatile; + # Traces the release binaries' syscalls as JSON with lurk. + containers.dataplane-syscall-tracer = + (pkgs.dockerTools.buildLayeredImage { + name = "ghcr.io/githedgehog/dataplane/syscall-tracer"; + inherit tag; + contents = + (pkgs.buildEnv { + name = "dataplane-syscall-tracer-env"; + pathsToLink = [ + "/bin" + "/etc" + "/var" + "/lib" + ]; + paths = [ + pkgs.pkgsBuildHost.lurk + pkgs.pkgsHostHost.dockerTools.fakeNss + pkgs.pkgsHostHost.busybox + pkgs.pkgsHostHost.dockerTools.usrBinEnv + workspace.cli + workspace.dataplane + workspace.init + ]; + }).overrideAttrs + source-volatile; + extraCommands = '' + # Point `src-prefix` at the sources this image ships. Referencing ${src} + # here is also what keeps it in the image closure: with the remap no + # longer naming a store path, nothing else retains it. + mkdir -p ".$(dirname "${src-prefix}")" + ln -s "${src}" ".${src-prefix}" + mkdir -p tmp + chmod 1777 tmp + ''; + config = { + Entrypoint = [ + "/bin/lurk" + "--json" + # Include the worker threads where the dataplane does its work. + "--follow-forks" + "/bin/dataplane" + ]; + Env = [ "HOME=/tmp" ]; + }; + }).overrideAttrs + source-volatile; + debug-tools = pkgs: [ diff --git a/justfile b/justfile index 07f78247ec..63d9ac2cd9 100644 --- a/justfile +++ b/justfile @@ -116,6 +116,7 @@ oci_frr_prefix := "githedgehog/dataplane/frr" oci_image_dataplane := oci_repo + "/" + oci_name + ":" + version oci_image_dataplane_core_viewer := oci_repo + "/" + oci_name + "/core-viewer:" + version oci_image_dataplane_dev_debugger := oci_repo + "/" + oci_name + "/dev-debugger:" + version +oci_image_dataplane_syscall_tracer := oci_repo + "/" + oci_name + "/syscall-tracer:" + version oci_image_dataplane_validator := oci_repo + "/" + oci_name + "/validator:" + version oci_image_frr_dataplane := oci_repo + "/" + oci_frr_prefix + ":" + version oci_image_frr_host := oci_repo + "/" + oci_frr_prefix + "-host:" + version @@ -290,6 +291,11 @@ build-container target="dataplane" *args: (build (if target == "dataplane" { "da docker tag "ghcr.io/githedgehog/dataplane/dev-debugger:{{version}}" "{{oci_image_dataplane_dev_debugger}}" echo "imported {{ oci_image_dataplane_dev_debugger }}" ;; + "dataplane-syscall-tracer") + docker load < ./results/containers.dataplane-syscall-tracer + docker tag "ghcr.io/githedgehog/dataplane/syscall-tracer:{{version}}" "{{oci_image_dataplane_syscall_tracer}}" + echo "imported {{ oci_image_dataplane_syscall_tracer }}" + ;; "debug-tools") # Uses nix only to produce a base image with the runtime closure (glibc, bash, etc.) # then layers locally-compiled cargo binaries on top via Dockerfile. @@ -398,6 +404,9 @@ push-container target="dataplane" *args: (build-container target args) && versio "dataplane-dev-debugger") push_image "{{ oci_image_dataplane_dev_debugger }}" ;; + "dataplane-syscall-tracer") + push_image "{{ oci_image_dataplane_syscall_tracer }}" + ;; "debug-tools") >&2 echo "do not push the debug tools!" exit 1 diff --git a/nix/overlays/dataplane-dev.nix b/nix/overlays/dataplane-dev.nix index 7b0074591a..345466c1d8 100644 --- a/nix/overlays/dataplane-dev.nix +++ b/nix/overlays/dataplane-dev.nix @@ -30,6 +30,33 @@ in hash = "sha256-GGi5hnrK5WpvnXHNckpsBch/SJ4lDvH7peSlrCdk218="; }; }); + # lurk disables ASLR in the tracee before exec, and treats failure as fatal. + # Docker's default seccomp profile answers personality(ADDR_NO_RANDOMIZE) with + # EPERM, so under a plain `docker run` the traced program never starts -- and + # lurk still exits 0 after emitting a well-formed JSON trace of its own child + # failing, which the `jq -R 'fromjson? // empty'` filter we document accepts + # without complaint. + # + # Nothing in the tracer image symbolizes an address, so a fixed layout buys us + # nothing. Make it advisory rather than telling users to pass + # `--security-opt seccomp=unconfined`, which drops confinement on a container + # whose whole job is ptracing a process. + # + # Two single-line substitutions rather than one spanning both: nix strips the + # common indentation from an indented string, so a multi-line search pattern + # would not match the source's own indentation. + lurk = prev.lurk.overrideAttrs (orig: { + postPatch = (orig.postPatch or "") + '' + substituteInPlace src/lib.rs \ + --replace-fail \ + 'personality::set(Persona::ADDR_NO_RANDOMIZE)' \ + 'let _ = personality::set(Persona::ADDR_NO_RANDOMIZE);' \ + --replace-fail \ + '.map_err(|_| anyhow!("Unable to set ADDR_NO_RANDOMIZE"))?;' \ + "" + ''; + }); + cargo-bolero = prev.cargo-bolero.override { inherit (override-packages) rustPlatform; }; cargo-deny = prev.cargo-deny.override { inherit (override-packages) rustPlatform; }; cargo-edit = prev.cargo-edit.override { inherit (override-packages) rustPlatform; }; From 9b9b9bf81ddfa5238a5d8584798d0e1300c6014e Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 14 Aug 2026 21:39:43 -0600 Subject: [PATCH 05/30] ci: publish debug images on a deliberate cadence The diagnostic images are useful only when they match the build being investigated, but building roughly 850 MB of extra images for every pull request would undermine the runner-load reduction this CI rework is meant to achieve. Build them automatically for pushes, the merge queue, and manual runs, with an explicit label available for debugging a pull request. Publish all three beside tagged releases so the matching tools remain available when a deployed build needs investigation. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 9621607312bc7124e0539d47a966c10e2f828e57) --- .github/workflows/README.md | 10 ++++++++-- .github/workflows/dev.yml | 22 ++++++++++++---------- justfile | 10 ++++++++-- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index a036099f08..4f8fd6d86d 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -63,6 +63,11 @@ Production artifacts are produced via nix builds in a separate CI workflow. - `ci:+miri` - Run Miri checks - `ci:+wasm` - Run the WASM build check - `ci:+concurrency` - Run Shuttle and Loom tests +- `ci:+debug-images` - Also build and push the core viewer, DAP debugger, and + syscall tracer images. They are built on main, in the merge queue, and on + dispatch regardless, and `ci:+merge-ready` turns them on too, since + `ci-gate` treats that label as enabling every gate; this is for when the + build itself needs debugging - `ci:+cross` - Build all cross-platform containers - `ci:+cross/full` - Also run the workspace test suite under qemu-user, on the two aarch64 musl legs. Gated like every other job, so the merge queue and @@ -103,8 +108,9 @@ If those queue failures stop being rare, the phasing is worth revisiting. - Checks: `debug` by default; `release` and `fuzz` on deep runs - Coverage: `debug` by default; `fuzz` on deep runs - Miri: required on deep runs; opt-in on pull requests with `ci:+miri` -- Containers: debug/release for dataplane, its three debug images, and FRR; - release for validator +- Containers: debug/release for dataplane and FRR; release for validator +- Debug images (core viewer, DAP debugger, syscall tracer): deep runs only, + or on a pull request with `ci:+debug-images` - VLAB configurations: spine-leaf fabric mode, L2VNI/L3VNI VPC modes, with gateway enabled diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 915f57be6a..547d3e9b33 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -68,6 +68,7 @@ jobs: outputs: container_profiles: "${{ steps.container-profiles.outputs.value }}" parallel: "${{ steps.parallel.outputs.value }}" + container_targets: "${{ steps.container-targets.outputs.value }}" profiles: "${{ steps.profiles.outputs.value }}" concurrency: "${{ steps.concurrency.outputs.value }}" cross: "${{ steps.cross.outputs.value }}" @@ -132,6 +133,16 @@ jobs: on-value: '["debug", "release", "fuzz"]' off-value: '["debug"]' + # Debug images are opt-in on pull requests because of their size. + - id: "container-targets" + uses: *gate + with: + labels: "debug-images" + # Keep this on one line: `ci-gate` writes the value to GITHUB_OUTPUT + # with a plain printf, which a multi-line value would corrupt. + on-value: '["frr.dataplane", "dataplane", "dataplane-core-viewer", "dataplane-dev-debugger", "dataplane-syscall-tracer", "validator"]' + off-value: '["frr.dataplane", "dataplane", "validator"]' + # Lab jobs require release images but not other release/fuzz checks. - id: "container-profiles" uses: *gate @@ -425,16 +436,7 @@ jobs: fail-fast: false max-parallel: ${{ fromJSON(needs.plan.outputs.parallel) }} matrix: - nix-target: - - frr.dataplane - - dataplane - # Must match the build that produced the core. - - dataplane-core-viewer - # Live debugging of the same build, driven from an editor over DAP. - - dataplane-dev-debugger - # Syscall trace of the same build, as JSON. - - dataplane-syscall-tracer - - validator + nix-target: "${{ fromJSON(needs.plan.outputs.container_targets) }}" # TODO: enable cfi and safe-stack on release when possible profile: "${{ fromJSON(needs.plan.outputs.container_profiles) }}" exclude: diff --git a/justfile b/justfile index 63d9ac2cd9..b2f04c564f 100644 --- a/justfile +++ b/justfile @@ -439,8 +439,14 @@ push-container target="dataplane" *args: (build-container target args) && versio [script] push: {{ _just_debuggable_ }} - # The core viewer must match the release it inspects. - for container in dataplane dataplane-core-viewer frr.dataplane validator; do + # Debug images must match the release they inspect. + for container in \ + dataplane \ + dataplane-core-viewer \ + dataplane-dev-debugger \ + dataplane-syscall-tracer \ + frr.dataplane \ + validator; do if [ "${container}" = "validator" ]; then platform="wasm32-wasip1" else From c666922d3ac7414c3aec307d8ce5db0e1944305a Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 15 Aug 2026 21:36:02 -0600 Subject: [PATCH 06/30] test(debug): exercise the debug images instead of only building them All three images built green while two of them could not do what the README documented. Building proves an image links; it says nothing about whether its entrypoint runs. `smoke-container` runs each one the way the README tells a user to: - the tracer under a plain `docker run`, with no seccomp relaxation, and requires an `execve` in the trace. Its failure mode is the reason this exists: lurk emitted eight well-formed JSON lines recording its own child failing to start, then exited 0, which the `jq -R 'fromjson? // empty'` filter we document accepts without complaint. Both guards fire against the pre-fix image. - the core viewer through its own entrypoint rather than by invoking gdb directly, since the `--directory` and `source` flags that register the printers live in that entrypoint. Checks that the printer set is registered, not merely that gdb started. - the debugger only for coming up and listening. Driving a real DAP session from CI means carrying a protocol client in-tree, and the contract it would pin is exercised better by pointing an editor at the image. It also would not have caught the defect on that image, which was in the documentation rather than the runtime. The trace goes to a file rather than a shell variable: at a few megabytes it overruns the here-string limit, and every grep against it then fails with E2BIG, which reads exactly like a failed trace. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 3c76aec407ae893ab3c1b80cf1150c69e8325b5f) --- .github/workflows/dev.yml | 11 ++++ ci.just | 4 ++ justfile | 105 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+) diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 547d3e9b33..acd5c20e4f 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -451,6 +451,17 @@ jobs: with: recipe: "ci::push-container" recipe_args: "${{ matrix.nix-target }} ${{ matrix.profile }} ${{ needs.version.outputs.version }}" + + # A debug image that builds is not a debug image that works. Two of + # these shipped with entrypoints that could not do what the README + # documents, and building them said nothing about it. + - name: "smoke" + if: "${{ startsWith(matrix.nix-target, 'dataplane-') }}" + uses: *just + with: + recipe: "ci::smoke-container" + recipe_args: "${{ matrix.nix-target }} ${{ matrix.profile }}" + - *verify-clean-tree - *tmate diff --git a/ci.just b/ci.just index f4c5406f8f..7f659c5ecc 100644 --- a/ci.just +++ b/ci.just @@ -78,6 +78,10 @@ cross platform libc +args: cross-test platform libc: NEXTEST_PROFILE=cross-qemu just {{ _lab }} platform={{ platform }} libc={{ libc }} profile=debug test +# Verify a debug image's entrypoint actually does its job. +smoke-container target profile: + just {{ _lab }} profile={{ profile }} platform=x86-64-v3 smoke-container {{ target }} + # Publish both content-derived and discoverable per-commit tags. [script] push-container target profile version: diff --git a/justfile b/justfile index b2f04c564f..fb7ad50d2f 100644 --- a/justfile +++ b/justfile @@ -258,6 +258,111 @@ setup-roots *args: {{ args }} done +# Check that the debug images actually do what the README says they do. +# +# Building an image proves it links; it does not prove the entrypoint runs. +# Both of these shipped broken: the tracer's documented `docker run` produced a +# well-formed JSON trace of its own child failing to start, and still exited 0. +[script] +smoke-container target: (build-container (if target == "dataplane-core-viewer" { target } else if target == "dataplane-dev-debugger" { target } else if target == "dataplane-syscall-tracer" { target } else { error("smoke-container: no smoke test for '" + target + "'; expected dataplane-core-viewer, dataplane-dev-debugger, or dataplane-syscall-tracer") })) + {{ _just_debuggable_ }} + declare -xr DOCKER_HOST="${DOCKER_HOST:-unix://{{ docker_sock }}}" + case "{{ target }}" in + "dataplane-syscall-tracer") + # A trace runs to megabytes, so keep it in a file rather than a + # variable: `declare -x` would put it in the environment, and every + # child process then fails to exec with E2BIG, which reads exactly + # like a failed trace. Here-strings are fine at this size -- bash + # backs them with a pipe or temp file -- and the checks below use + # them on captured output. + declare trace + trace="$(mktemp)" + declare -r trace + # Name the container and remove it by name. `timeout` signals the + # docker CLI, and the daemon -- not the CLI -- owns the container's + # lifetime, so on the timeout path `--rm` alone can leave a `lurk` + # tracing something for as long as the runner lives. + declare cid + cid="smoke-syscall-tracer-$$" + declare -r cid + trap 'rm -f -- "${trace}"; docker rm -f "${cid}" >/dev/null 2>&1 || true' EXIT + # No seccomp relaxation on purpose: this is the documented command. + timeout 60 docker run --rm --name "${cid}" \ + "{{ oci_image_dataplane_syscall_tracer }}" \ + >"${trace}" 2>&1 || true + if grep -q "Unable to set ADDR_NO_RANDOMIZE" "${trace}"; then + >&2 echo "::error::lurk could not disable ASLR, so the tracee never ran" + exit 1 + fi + # The tracee has to actually execute, not merely be attached to. + if ! grep -q '"syscall":"execve"' "${trace}"; then + >&2 echo "::error::no execve in the trace: the traced program never started" + >&2 head -20 "${trace}" + exit 1 + fi + printf 'syscall-tracer: traced %s syscalls\n' "$(grep -c '"type":"SYSCALL"' "${trace}")" + ;; + "dataplane-dev-debugger") + # Only that it comes up and listens. Driving a DAP session from + # CI means carrying a protocol client for a contract better + # exercised by pointing a real editor at `just debug bugstalker`. + declare cid + # Let docker pick the host port and read it back. A fixed one + # collides: these run on a shared daemon, and `container_profiles` + # can put two of these jobs on the same node at once, where the + # second bind fails and reads as "the listener never came up". + cid="$(docker run -d --rm -p 127.0.0.1::4711 "{{ oci_image_dataplane_dev_debugger }}")" + declare -r cid + trap 'docker kill "${cid}" >/dev/null 2>&1 || true' EXIT + declare -i waited=0 + declare port + port="$(docker port "${cid}" 4711/tcp | head -1)" + port="${port##*:}" + declare -r port + if [ -z "${port}" ]; then + >&2 echo "::error::docker published no host port for 4711/tcp" + exit 1 + fi + until timeout 1 bash -c "/dev/null; do + if [ "${waited}" -ge 60 ]; then + >&2 echo "::error::dev-debugger never listened on 4711" + >&2 docker logs "${cid}" 2>&1 | tail -20 + exit 1 + fi + sleep 1 + waited=$(( waited + 1 )) + done + echo "dev-debugger: DAP listener up" + ;; + "dataplane-core-viewer") + # The Rust pretty-printers are the reason this image exists, and + # what registers them is the entrypoint's own `source` flag -- so + # drive the real entrypoint rather than invoking gdb directly, + # which would only test a copy of it. + declare out + out="$(printf 'info pretty-printer\nquit\n' \ + | timeout 120 docker run --rm -i "{{ oci_image_dataplane_core_viewer }}" 2>&1)" + if grep -qiE "traceback|no module named" <<<"${out}"; then + >&2 echo "::error::gdb could not load the rust pretty-printers" + >&2 printf '%s\n' "${out}" + exit 1 + fi + # A registered printer set, not merely a clean start. + for want in StdString StdVec StdHashMap; do + if ! grep -q "${want}" <<<"${out}"; then + >&2 echo "::error::rust pretty-printer ${want} is not registered" + exit 1 + fi + done + echo "core-viewer: rust pretty-printers registered" + ;; + *) + # Unreachable: the dependency above rejects anything else first. + >&2 echo "::error::no smoke test defined for {{ target }}" + exit 1 + ;; + esac + # Build the dataplane container image [script] build-container target="dataplane" *args: (build (if target == "dataplane" { "dataplane.tar" } else if target == "validator" { "workspace.validator" } else { "containers." + target }) args) From 06c0ccaa36aac36ad1b06e2eaa5e698a85fcb1a0 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 15 Aug 2026 21:36:22 -0600 Subject: [PATCH 07/30] feat(debug): run a binary or a single test inside the debug images The published images debug what CI built. Debugging what you are building meant either rebuilding an image by hand or falling back to a system gdb, which is exactly the case where symbols do not line up. `just debug ` builds the image that carries the tool at the current profile, platform, instrumentation, and sanitizer, and runs the target inside it: - `lurk` traces syscalls and streams JSON until the program exits. - `gdb` runs gdbserver and waits, printing the `target remote` line. - `bugstalker` waits for a DAP client, printing the `program` and `args` for the launch request -- in remote-DAP mode it takes those from the client rather than from its own command line. `target` is either one of the binaries the images already carry, in which case nothing needs mounting, or a nextest filter. For a test the archive is built and unpacked, and that binary was built outside the image, so the store comes along read-only and it runs with its package directory as the working directory the way nextest runs it. Naming a target exactly is the tedious part -- test paths are long and nobody remembers them -- so leaving it out offers everything through skim, and a filter matching several offers those. Resolution stays unambiguous: exactly one match runs without asking, no match is an error, and several with no terminal to ask at is an error listing them rather than a guess. That last case is what keeps this safe to call from a script. `just debug-list` prints the same list without running anything, and `just inspect-core` opens a core file in a gdb built from the same settings. Randomization stays enabled under gdbserver. Docker's default seccomp answers personality(ADDR_NO_RANDOMIZE) with EPERM, and while gdbserver treats that as non-fatal -- unlike lurk, which is why lurk is patched -- it otherwise opens with a warning that reads like a real failure. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 057b27dec27edac56786e95ae57994253772a567) --- .github/workflows/README.md | 47 ++++++++ default.nix | 38 +++--- justfile | 228 ++++++++++++++++++++++++++++++++++++ 3 files changed, 298 insertions(+), 15 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 4f8fd6d86d..3c0dc0ba7b 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -205,6 +205,53 @@ If those queue failures stop being rare, the phasing is worth revisiting. Both upload unarchived, so they download as the named file rather than wrapped in a zip. +### Debugging locally + +The published images debug what CI built. To debug what you are building, `just +debug` builds the matching image and runs a workspace binary or a single test +inside it, at whatever `profile`, `platform`, `instrument`, and `sanitize` you +pass. Symbols only line up when those match the build the problem appeared in, +which is the whole reason to go through the image rather than a system gdb. + +```console +just debug # pick from a list +just debug bugstalker # pick, then wait for an editor +just debug lurk dataplane # trace syscalls, streams JSON, runs to exit +just debug gdb dataplane # gdbserver on 2345, waits for a client +just profile=fuzz debug gdb test_parse_interface args +just debug-list # print the same list without running anything +``` + +Name nothing and everything is offered through `skim`, which the dev shell +provides. Name a filter matching one test and it runs without asking; name one +matching several and those are offered. A filter matching nothing is an error +rather than a guess, and so is an ambiguous one when there is no terminal to +ask at, which is what makes this safe to call from a script. + +The third argument narrows which archive is searched, so +`just debug gdb some_test args` builds only `args`' tests. It is worth passing: +the default builds every test in the workspace, which is a long wait if all you +wanted was to pick from a short list. + +`gdb` and `bugstalker` block until you disconnect and interrupt them; that is +the point. `gdb` prints the `target remote` line to use. `bugstalker` prints a +`.zed/debug.json` entry ready to paste, because in remote-DAP mode it takes the +program from the client's launch request rather than from its own command line, +so connecting an editor is only half of it. The `tcp_connection` field in that +entry is what stops the editor spawning a second debugger of its own. + +A test runs with its package directory as the working directory, the way +nextest runs it, so relative paths behave the same as under `just test`. + +To open a core file: + +```console +just inspect-core /path/to/core.1234 +``` + +Pass the same build settings that produced the binary that dumped +(`just profile=release inspect-core ...`), for the same reason. + --- ## Linting and Validation Workflows for Pull Requests diff --git a/default.nix b/default.nix index 6f6345a8f3..1b80c72e94 100644 --- a/default.nix +++ b/default.nix @@ -182,6 +182,7 @@ let qemu-user rust-toolchain shellcheck + skim # the `just debug` picker skopeo wasmtime wget @@ -1014,15 +1015,17 @@ let source-volatile; # gdb needs a writable HOME for logs and its index cache. extraCommands = '' - # Point `src-prefix` at the sources this image ships. Referencing ${src} - # here is also what keeps it in the image closure: with the remap no - # longer naming a store path, nothing else retains it. - mkdir -p ".$(dirname "${src-prefix}")" - ln -s "${src}" ".${src-prefix}" + # The remapped prefix is relative, so a debugger resolves source against + # its working directory rather than an absolute path. Ship the tree at + # /src and start there. Referencing ${src} is also what keeps it in the + # image closure: with the remap no longer naming a store path, nothing + # else retains it. + ln -s "${src}" src mkdir -p tmp chmod 1777 tmp ''; config = { + WorkingDir = "/src"; Entrypoint = [ "/bin/gdb" "--directory=/lib/rustlib/etc" @@ -1056,15 +1059,17 @@ let source-volatile; # bugstalker needs a writable HOME for its keymap and history. extraCommands = '' - # Point `src-prefix` at the sources this image ships. Referencing ${src} - # here is also what keeps it in the image closure: with the remap no - # longer naming a store path, nothing else retains it. - mkdir -p ".$(dirname "${src-prefix}")" - ln -s "${src}" ".${src-prefix}" + # The remapped prefix is relative, so a debugger resolves source against + # its working directory rather than an absolute path. Ship the tree at + # /src and start there. Referencing ${src} is also what keeps it in the + # image closure: with the remap no longer naming a store path, nothing + # else retains it. + ln -s "${src}" src mkdir -p tmp chmod 1777 tmp ''; config = { + WorkingDir = "/src"; Entrypoint = [ "/bin/bs" # Bind the published interface rather than container-local loopback. @@ -1111,15 +1116,17 @@ let }).overrideAttrs source-volatile; extraCommands = '' - # Point `src-prefix` at the sources this image ships. Referencing ${src} - # here is also what keeps it in the image closure: with the remap no - # longer naming a store path, nothing else retains it. - mkdir -p ".$(dirname "${src-prefix}")" - ln -s "${src}" ".${src-prefix}" + # The remapped prefix is relative, so a debugger resolves source against + # its working directory rather than an absolute path. Ship the tree at + # /src and start there. Referencing ${src} is also what keeps it in the + # image closure: with the remap no longer naming a store path, nothing + # else retains it. + ln -s "${src}" src mkdir -p tmp chmod 1777 tmp ''; config = { + WorkingDir = "/src"; Entrypoint = [ "/bin/lurk" "--json" @@ -1146,6 +1153,7 @@ let # pkgs.wireshark-cli pkgs.bashInteractive + pkgs.bugstalker pkgs.coreutils pkgs.curl pkgs.debianutils diff --git a/justfile b/justfile index fb7ad50d2f..720fdec7ef 100644 --- a/justfile +++ b/justfile @@ -258,6 +258,234 @@ setup-roots *args: {{ args }} done +# Ports the debug helpers listen on. Override for a second session. +gdb_port := "2345" + +bs_port := "4711" + +# Workspace binaries the debug images already carry, spelled as the images +# spell them. The nix attribute is keyed by directory (`workspace.init`) but +# the binary it installs is `dataplane-init`, and it is the binary name that has +# to appear here -- `debug` passes it straight through as `/bin/`. +[private] +_debug_binaries := "dataplane cli dataplane-init" + +# Build settings to forward when a recipe has to re-enter `just` rather than +# depend on it. A debug session is only useful against the same build the +# problem showed up in. +[private] +_forward := "jobs=" + jobs + " cores=" + cores + " debug_justfile=" + debug_justfile \ + + " profile=" + profile + " platform=" + platform + " libc=" + libc \ + + " features=" + features + " default_features=" + default_features \ + + " instrument=" + instrument + " sanitize=" + sanitize + " nightly=" + nightly + +# List what `just debug` can run at the current profile: the workspace binaries +# the images carry, and every test in a nextest archive. +[script] +debug-list package="tests.all": (build (if package == "tests.all" { "tests.all" } else { "tests.pkg." + package })) + {{ _just_debuggable_ }} + declare -r suite="{{ if package == "tests.all" { "tests.all" } else { "tests.pkg." + package } }}" + echo "binaries:" + for b in {{ _debug_binaries }}; do printf ' %s\n' "${b}"; done + echo "tests:" + cargo nextest list --archive-file results/"${suite}"/*.tar.zst --workspace-remap "$(pwd)" + +# Run a workspace binary or a single test under a debug helper, in the image +# that carries it, built at the current profile and instrumentation. +# +# just debug # pick from a list +# just debug bugstalker # pick, then wait for an editor +# just debug lurk dataplane # trace syscalls, streams JSON +# just debug gdb dataplane # gdbserver, waits for gdb +# just profile=fuzz debug gdb test_parse_interface args +# +# `target` is one of the workspace binaries, or a nextest filter. Leave it out +# and everything is offered; give a filter matching one test and it is used +# without asking; give one matching several and those are offered. `package` +# narrows which archive is built to find them, so naming one is much quicker +# than the default of building every test in the workspace. gdb and bugstalker +# wait for a client instead of running to completion. +# +# The remapped source prefix is relative, so a client shows source only if its +# working directory is the root of the tree the binary was built from. This +# passes `-w` for that in every case; a local `gdb` needs you to be standing in +# the right place yourself. +[script] +debug tool="gdb" target="" package="tests.all" *args: (build-container (if tool == "gdb" { "dataplane-core-viewer" } else if tool == "bugstalker" { "dataplane-dev-debugger" } else if tool == "lurk" { "dataplane-syscall-tracer" } else { error("debug: unknown tool '" + tool + "'; expected gdb, bugstalker, or lurk") })) + {{ _just_debuggable_ }} + declare -xr DOCKER_HOST="${DOCKER_HOST:-unix://{{ docker_sock }}}" + + declare image + case "{{ tool }}" in + gdb) image="{{ oci_image_dataplane_core_viewer }}" ;; + bugstalker) image="{{ oci_image_dataplane_dev_debugger }}" ;; + lurk) image="{{ oci_image_dataplane_syscall_tracer }}" ;; + *) + # Unreachable: the dependency above rejects anything else before + # this body runs. Kept so `set -u` cannot meet an unset `image`. + >&2 echo "debug: unknown tool '{{ tool }}'" + exit 1 + ;; + esac + declare -r image + + # Resolve the target to a program, its arguments, and a working directory. + declare program workdir + declare -a program_args=() mounts=() + if [[ -n '{{ target }}' && " {{ _debug_binaries }} " == *" {{ target }} "* ]]; then + # The images carry these already, so nothing needs mounting. `/src` is + # where they ship the sources, and the remapped prefix is relative, so + # the debugger only resolves source if it starts there. + program="/bin/{{ target }}" + workdir="/src" + else + # A test, or nothing yet. Either way the archive has to exist before + # there is anything to name or to choose between. + declare -r suite="{{ if package == "tests.all" { "tests.all" } else { "tests.pkg." + package } }}" + just {{ _forward }} build "${suite}" + declare -r extract="${PWD}/results/debug-extract" + rm -rf -- "${extract}" + mkdir -p -- "${extract}" + declare errors candidates + errors="$(mktemp)" + candidates="$(mktemp)" + declare -r errors candidates + trap 'rm -f -- "${errors}" "${candidates}"' EXIT + # Keep nextest's diagnostics: it writes progress to stderr and JSON to + # stdout, and discarding the former turns "cargo is not on PATH" into + # an empty result that looks like "no such test". + declare listing + if ! listing="$(cargo nextest list --archive-file results/"${suite}"/*.tar.zst \ + --workspace-remap "$(pwd)" --extract-to "${extract}" --extract-overwrite \ + --message-format json \ + {{ if target == "" { "" } else { "-E 'test(/" + target + "/)'" } }} \ + 2>"${errors}")"; then + >&2 echo "::error::could not list the tests in ${suite}" + >&2 cat -- "${errors}" + exit 1 + fi + declare -r listing + + # label \t binary \t workdir \t test, so a picker can show the label + # and the caller can read the rest back off the same line. + if [ -z '{{ target }}' ]; then + for b in {{ _debug_binaries }}; do + printf '%s (binary)\t/bin/%s\t/\t\n' "${b}" "${b}" + done >>"${candidates}" + fi + jq -r ' + ."rust-suites" | to_entries[] | .value as $s + | ($s.testcases // {} | to_entries[] + | select(."value"."filter-match"."status" == "matches") | .key) as $t + | "\($s."binary-id") \($t)\t\($s."binary-path")\t\($s.cwd)\t\($t)" + ' <<<"${listing}" >>"${candidates}" + + declare -i found + found="$(wc -l <"${candidates}")" + declare chosen + if [ "${found}" -eq 0 ]; then + >&2 echo "::error::nothing in ${suite} matches '{{ target }}'" + exit 1 + elif [ "${found}" -eq 1 ]; then + chosen="$(cat -- "${candidates}")" + elif [ -t 0 ]; then + # Several matches and someone to ask. Showing only the label keeps + # the store paths out of the list without losing them. + if ! command -v sk >/dev/null 2>&1; then + >&2 echo "::error::sk (skim) is not on PATH; use the dev shell, or name a target exactly" + exit 1 + fi + chosen="$(sk --delimiter '\t' --with-nth 1 \ + --prompt "{{ tool }} > " --height 40% --reverse <"${candidates}")" + if [ -z "${chosen}" ]; then + >&2 echo "debug: nothing picked" + exit 1 + fi + else + if [ -n '{{ target }}' ]; then + >&2 echo "::error::'{{ target }}' matches ${found} in ${suite}; name one exactly, or pick from a terminal:" + else + >&2 echo "::error::not a terminal, so nothing to ask; name one of:" + fi + >&2 cut -f1 -- "${candidates}" + exit 1 + fi + declare -r chosen + + program="$(cut -f2 <<<"${chosen}")" + # nextest gives a test its package root as the working directory, and + # anything reading a relative path depends on that. + workdir="$(cut -f3 <<<"${chosen}")" + declare test_name + test_name="$(cut -f4 <<<"${chosen}")" + declare -r test_name + if [ -n "${test_name}" ]; then + program_args=(--exact "${test_name}" --nocapture) + # Built outside the image, so it still resolves its loader and its + # libraries through the store; the store has to come along. + mounts=(-v /nix/store:/nix/store:ro -v "${extract}":"${extract}":ro -v "${PWD}":"${PWD}":ro) + fi + fi + declare -r program workdir + + case "{{ tool }}" in + lurk) + docker run --rm -i "${mounts[@]}" -w "${workdir}" --entrypoint /bin/lurk \ + "${image}" --json --follow-forks "${program}" "${program_args[@]}" {{ args }} + ;; + gdb) + >&2 echo "gdbserver on {{ gdb_port }}. Connect with:" + >&2 echo " gdb -ex 'target remote 127.0.0.1:{{ gdb_port }}' '${program}'" + # Randomization stays on: docker's default seccomp answers + # personality(ADDR_NO_RANDOMIZE) with EPERM, and gdbserver would + # otherwise open with an alarming warning about a benign failure. + # Loopback, not `0.0.0.0`: gdbserver runs whatever a client asks + # it to, with no authentication, and a bare `-p` would offer that + # to anything that can reach this machine. + docker run --rm -i -p "127.0.0.1:{{ gdb_port }}:{{ gdb_port }}" "${mounts[@]}" -w "${workdir}" \ + --entrypoint /bin/gdbserver "${image}" --no-disable-randomization \ + ":{{ gdb_port }}" "${program}" "${program_args[@]}" {{ args }} + ;; + bugstalker) + # It takes the program from the client's launch request, not from + # here, so connecting an editor to this is only half of it. Print + # the other half ready to paste into .zed/debug.json: `tcp_connection` + # is what stops the editor spawning a `bs` of its own. + >&2 echo "bugstalker DAP on {{ bs_port }}. Give the editor:" + >&2 jq -n --arg l "container: {{ tool }} {{ target }}" --arg p "${program}" \ + --arg w "${workdir}" --argjson port "{{ bs_port }}" \ + '{label: $l, adapter: "bugstalker-dap", request: "launch", + program: $p, args: $ARGS.positional, cwd: $w, + tcp_connection: {host: "127.0.0.1", port: $port}}' \ + --args "${program_args[@]}" {{ args }} + # The container end is 4711 whatever `bs_port` says: the image's + # entrypoint hardcodes `--dap-remote=0.0.0.0:4711`, so publishing + # `bs_port:bs_port` only worked while `bs_port` was 4711. Loopback + # for the same reason as gdbserver -- a DAP `launch` request runs + # an arbitrary program. + docker run --rm -i -p "127.0.0.1:{{ bs_port }}:4711" "${mounts[@]}" -w "${workdir}" "${image}" + ;; + esac + +# Open a core file in a gdb matched to the build that produced it. +# +# Symbols only line up with the exact version and profile that dumped, so pass +# the same build settings that produced the binary. +[script] +inspect-core core *args: (build-container "dataplane-core-viewer") + {{ _just_debuggable_ }} + declare -xr DOCKER_HOST="${DOCKER_HOST:-unix://{{ docker_sock }}}" + if [ ! -r '{{ core }}' ]; then + >&2 echo "inspect-core: cannot read '{{ core }}'" + exit 1 + fi + declare core_dir core_file + core_dir="$(cd "$(dirname -- '{{ core }}')" && pwd)" + core_file="$(basename -- '{{ core }}')" + declare -r core_dir core_file + docker run --rm -it -v "${core_dir}":/cores:ro \ + "{{ oci_image_dataplane_core_viewer }}" "/cores/${core_file}" {{ args }} + # Check that the debug images actually do what the README says they do. # # Building an image proves it links; it does not prove the entrypoint runs. From 1f4612172c766f4cdac3e46cded72f14eb827d1f Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 25 Aug 2026 20:50:09 -0600 Subject: [PATCH 08/30] docs: Record what a commit message carries Written down because the convention is not self-evident from `git log` alone: a reviewer can see that messages are short without seeing that the brevity is deliberate. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 62e214e5fb..9116ee28da 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,3 +4,21 @@ Follow the [development guide](development/README.md). If you make a design decision or do a code review, try to cite the section of the development guide you are following. + +## Commit messages + +A commit message signposts the diff; it does not restate it. Rationale belongs where it +constrains the code -- in the code, its doc comments, or the development guide -- because a +second copy in the message is a second thing to keep correct, and the two will drift. + +A message carries what the diff cannot: + +- the "why" of the commit, +- non-obvious process facts, + +It does not re-summarize the change, narrate how it was made, or repeat an argument the +commit already puts in the tree. For a docs commit, the file is the content; the message +points at it. + +Keep `git log` scannable: a few lines by default, longer only where the diff is opaque and +the message is what makes the commit reviewable. From d2e63fe016b5f2625b933ea01ad7d791825ccc67 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 25 Aug 2026 20:50:09 -0600 Subject: [PATCH 09/30] build(just): Write an lcov report beside the html one The html report is for reading; lcov is what external coverage tooling ingests, and regenerating it separately means running the whole instrumented suite again. Co-Authored-By: Claude Opus 5 (1M context) --- justfile | 1 + 1 file changed, 1 insertion(+) diff --git a/justfile b/justfile index 720fdec7ef..05e7e268f8 100644 --- a/justfile +++ b/justfile @@ -1105,6 +1105,7 @@ coverage *args: cargo llvm-cov --no-report --branch nextest {{ args }} mkdir -p "${out}" cargo llvm-cov report --branch --html --output-dir="${out}" + cargo llvm-cov report --branch --lcov --output-path="${out}/lcov.info" cargo llvm-cov report --branch --codecov --output-path="${out}/codecov.json" cargo llvm-cov report --branch --summary-only From c99490e8643fcdc17ab4bd4b2b11dd425c2e8144 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 25 Aug 2026 20:50:20 -0600 Subject: [PATCH 10/30] build(nix): Add cargo-expand, cargo-mutants and cargo-show-asm to the dev shell Co-Authored-By: Claude Opus 5 (1M context) --- default.nix | 3 +++ nix/overlays/dataplane-dev.nix | 3 +++ 2 files changed, 6 insertions(+) diff --git a/default.nix b/default.nix index 1b80c72e94..96928496c0 100644 --- a/default.nix +++ b/default.nix @@ -159,8 +159,11 @@ let cargo-deny cargo-depgraph cargo-edit + cargo-expand cargo-llvm-cov + cargo-mutants cargo-nextest + cargo-show-asm commitlint-rs direnv gateway-crd diff --git a/nix/overlays/dataplane-dev.nix b/nix/overlays/dataplane-dev.nix index 345466c1d8..6e746d089c 100644 --- a/nix/overlays/dataplane-dev.nix +++ b/nix/overlays/dataplane-dev.nix @@ -60,6 +60,9 @@ in cargo-bolero = prev.cargo-bolero.override { inherit (override-packages) rustPlatform; }; cargo-deny = prev.cargo-deny.override { inherit (override-packages) rustPlatform; }; cargo-edit = prev.cargo-edit.override { inherit (override-packages) rustPlatform; }; + cargo-expand = prev.cargo-expand.override { inherit (override-packages) rustPlatform; }; + cargo-show-asm = prev.cargo-show-asm.override { inherit (override-packages) rustPlatform; }; + cargo-mutants = prev.cargo-mutants.override { inherit (override-packages) rustPlatform; }; cargo-llvm-cov = (prev.cargo-llvm-cov.override override-packages).overrideAttrs (orig: { # the test suite is very impractical in our CI (fails on nightly for spurious reasons), and has nothing to do with # our project. From d1398e30184a5d6fbd0ff59de68977e4e5244953 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 25 Aug 2026 20:50:28 -0600 Subject: [PATCH 11/30] feat(debug): Build gdb and perf to run where there is no nix store Both are meant to be copied into a VM or an image that has no store to resolve an interpreter against. Why configure flags cannot get you there, and why perf needs a different route from gdb, is recorded at each derivation -- the reasoning constrains the argument lists, so it lives beside them. Co-Authored-By: Claude Opus 5 (1M context) --- nix/overlays/dataplane-dev.nix | 120 ++++++++++++++++++++++++++++++--- 1 file changed, 109 insertions(+), 11 deletions(-) diff --git a/nix/overlays/dataplane-dev.nix b/nix/overlays/dataplane-dev.nix index 6e746d089c..defb4311eb 100644 --- a/nix/overlays/dataplane-dev.nix +++ b/nix/overlays/dataplane-dev.nix @@ -82,15 +82,113 @@ in destination = "/src/fabric/${p}"; }; - gdb' = prev.gdb.overrideAttrs (orig: { - CFLAGS = "-Os -flto"; - CXXFLAGS = "-Os -flto"; - LDFLAGS = "-flto -Wl,--as-needed,--gc-sections -static-libstdc++ -static-libgcc"; - buildInputs = (orig.buildInputs or [ ]); - configureFlags = (orig.configureFlags or [ ]) ++ [ - "--enable-static" - "--disable-inprocess-agent" - "--disable-source-highlight" # breaks static compile - ]; - }); + # A gdb that can be copied into a VM or an image that has no nix store, and + # still run: musl, no interpreter, no shared libraries at all. + # + # Configure flags cannot get you here. `--enable-static` and + # `--disable-shared` -- which nixpkgs already passes -- only decide whether + # the libbfd, libopcodes, and libctf that this tree builds are archives; they + # say nothing about how the gdb executable links against readline, ncurses, + # expat, or python, and those have no static outputs in the default package + # set. Hence pkgsStatic, which rebuilds the dependencies rather than the + # link line. + # + # pkgsStatic gates python support on host == build, so this gdb configures + # `--without-python` and cannot load the Rust pretty printers. It is a + # bare-metal debugger, not a replacement for the ordinary `gdb` that + # `containers.dataplane-core-viewer` ships with `rust-gdb-printers`. + gdb' = final.pkgsStatic.gdb.override { + # dejagnu is a buildInput only so that gdb's own test suite can run, and we + # never run it. It also cannot be built here: expect resolves `tclStubsPtr` + # from tcl's stub library, which exists to be filled in by a dynamic loader, + # so a static link leaves it undefined. + dejagnu = final.pkgsStatic.emptyDirectory; + }; + + # A perf that can be copied into a VM or an image with no nix store, for the + # same reason as `gdb'`. + # + # Unlike gdb this cannot come from pkgsStatic: elfutils carries + # `badPlatforms = isStatic` because its Makefile builds libelf.so + # unconditionally, and a static toolchain cannot emit a shared object at all + # (`crtbeginT.o: relocation R_X86_64_32 against hidden symbol __TMC_END__`). + # perf without libelf/libdw is not worth shipping, so build against ordinary + # glibc packages -- whose elfutils already installs libelf.a and libdw.a -- + # and make only the final link static. + perf' = + let + # Every dependency below is either unusable in a static binary or not + # worth its transitive static closure. Dropping them at the argument + # layer keeps them out of the build; the NO_* flags tell perf's own + # configure-equivalent the same thing, so the two cannot disagree. + none = final.emptyDirectory; + in + (final.perf.override { + stdenv = final.stdenvAdapters.makeStaticBinaries final.stdenv; + withPython = false; + withLibcap = false; + newt = none; + slang = none; + babeltrace = none; + libunwind = none; + libpfm = none; + numactl = none; + openssl = none; + libopcodes = none; + libtraceevent = none; + systemtap-unwrapped = none; + }).overrideAttrs + (orig: { + # dlfilters are dlopen-ed plugins, which a static perf could not load + # even if the toolchain could build them. + postPatch = orig.postPatch + '' + substituteInPlace Makefile.perf \ + --replace-fail \ + 'DLFILTERS := dlfilter-test-api-v0.so dlfilter-test-api-v2.so dlfilter-show-cycles.so' \ + 'DLFILTERS :=' \ + --replace-fail '$(INSTALL) $(DLFILTERS) ' 'true ' + ''; + # perf keys off -static in LDFLAGS to add `-lelf -lz -llzma -lbz2 -ldl` + # to the libdw link. Without it the libdw probe fails and perf builds + # with DWARF support silently off -- it still links, so this is only + # visible in `perf version --build-options`. Pass it in the + # environment, not via makeFlags, so Makefile.config's own `LDFLAGS +=` + # still appends. + env = orig.env // { + LDFLAGS = "-static"; + }; + # A static link does not follow a library's own dependencies, so + # libelf.a and libdw.a's compression backends have to be named here, + # as archives rather than the shared objects the normal outputs carry. + buildInputs = orig.buildInputs ++ [ + final.zlib.static + (final.zstd.override { static = true; }) + (final.xz.override { enableStatic = true; }) + (final.bzip2.override { enableStatic = true; }) + ]; + makeFlags = orig.makeFlags ++ [ + "NO_LIBPYTHON=1" + "NO_LIBPERL=1" + "NO_SLANG=1" + "NO_NEWT=1" + "NO_LIBBABELTRACE=1" + "NO_LIBNUMA=1" + "NO_LIBAUDIT=1" + "NO_LIBBPF=1" + "NO_LIBPFM4=1" + "NO_LIBCRYPTO=1" + "NO_JVMTI=1" + "NO_LIBUNWIND=1" + "NO_LIBDEBUGINFOD=1" + "NO_LIBTRACEEVENT=1" + "NO_SDT=1" + # Only C++ demangling. perf's Rust v0 demangler is built in and + # unaffected, which is what matters for a Rust dataplane. + "NO_DEMANGLE=1" + ]; + # wrapProgram would replace the binary with a shell script, defeating + # the point of a static build. It only put objdump on PATH for + # `perf annotate`, which needs a toolchain on the target regardless. + preFixup = ""; + }); } From 9016291054e76b785d4d822a4a133615d74b6c11 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 25 Aug 2026 20:50:28 -0600 Subject: [PATCH 12/30] feat(debug): Dump a core from a running process without ending it Co-Authored-By: Claude Opus 5 (1M context) --- scripts/dump-core.sh | 82 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100755 scripts/dump-core.sh diff --git a/scripts/dump-core.sh b/scripts/dump-core.sh new file mode 100755 index 0000000000..e9c398a362 --- /dev/null +++ b/scripts/dump-core.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: Apache-2.0 +# Copyright Open Network Fabric Authors + +# Dump a core from a running process without ending it. +# +# Attaching stops the process, so the window between attach and detach is time +# the dataplane is not forwarding. Keep the command list to the dump itself. + +set -euo pipefail + +if [ "$#" -lt 1 ] || [ "$#" -gt 2 ]; then + >&2 echo "usage: ${0##*/} [core-file]" + exit 2 +fi + +declare -r pid="$1" +declare -r core="${2:-/tmp/dataplane.core}" + +if [[ ! "${pid}" =~ ^[0-9]+$ ]]; then + >&2 echo "${0##*/}: '${pid}' is not a pid" + exit 2 +fi + +if [ ! -d "/proc/${pid}" ]; then + >&2 echo "${0##*/}: no process ${pid}" + exit 1 +fi + +# ptrace across uids needs CAP_SYS_PTRACE, and a dataplane does not run as you. +# Checking here turns a confusing gdb error into a clear one. +if [ "$(id -u)" -ne 0 ]; then + >&2 echo "${0##*/}: must run as root to attach to ${pid}" + exit 1 +fi + +# The static gdb is the point of this script: prefer one sitting beside it, so +# that scp'ing the pair to a machine with no nix store is enough. +declare gdb="${GDB:-}" +if [ -z "${gdb}" ]; then + if [ -x "$(dirname -- "$(readlink -f -- "$0")")/gdb" ]; then + gdb="$(dirname -- "$(readlink -f -- "$0")")/gdb" + else + gdb="$(command -v gdb || true)" + fi +fi +declare -r gdb +if [ -z "${gdb}" ]; then + >&2 echo "${0##*/}: no gdb; set GDB, or put one next to this script" + exit 1 +fi + +# `--nx` because an operator's ~/.gdbinit must not decide what a core contains, +# and `auto-load off` because the only thing gdb would auto-load here is +# libthread_db, which it does not need: threads are enumerated from /proc, and +# a static gdb cannot dlopen it anyway. Without this it prints a paragraph of +# safe-path advice on every run. +"${gdb}" --nx --quiet --batch \ + -iex 'set auto-load off' \ + -ex "generate-core-file ${core}" \ + -ex detach \ + -p "${pid}" + +if [ ! -s "${core}" ]; then + >&2 echo "${0##*/}: gdb wrote no core to ${core}" + exit 1 +fi + +# `detach` resumes, but say so from the process's own state rather than from +# gdb's exit status: a core you can open is worthless if the dataplane is still +# sitting in ptrace-stop. 't' is TASK_TRACED. +# The comm field is parenthesised and may contain spaces, so cut past the last +# ')' rather than counting whitespace-separated fields. +declare state +state="$(sed -e 's/^.*) //' -e 's/ .*//' "/proc/${pid}/stat" 2>/dev/null || echo gone)" +declare -r state +case "${state}" in + t) >&2 echo "${0##*/}: ${pid} is still stopped; detach it by hand"; exit 1 ;; + gone) >&2 echo "${0##*/}: ${pid} did not survive"; exit 1 ;; +esac + +echo "${0##*/}: ${core} ($(stat -c %s -- "${core}") bytes), ${pid} running (${state})" From 3a27f88e6835d8d4418cb6b936d128fde8479616 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Tue, 18 Aug 2026 20:49:54 -0600 Subject: [PATCH 13/30] build(nix): Add duvet to the dev toolchain duvet parses a specification into its individual requirements and matches them against citations left in the source -- `//= ` followed by the requirement's text -- so a requirement with no implementation, or an implementation with no test, becomes visible. It is the third leg. Bolero says a property holds. cargo-mutants says enough properties exist that nothing goes unasserted. Neither can say the properties are the ones the specification asked for, and the RFC 4884 defect fixed in the previous commit is what that gap looks like: a length check expressed in bits where the values were octets, refusing seven of every eight conforming messages, with a test that had been written to make the deviation pass. Mutation testing would have rewarded closing those mutants against the code, cementing it. The citations that fix already carries are in duvet's format, so a report run has something to find on day one. ## Taken from the crate, not the tag duvet embeds `www/public/script.js` with `include_str!`, and that file is a JavaScript build product: absent from the git tree, present in the published crate. Building from the tag would mean carrying a node toolchain and a second lockfile to produce a file the crate already ships. The crate also carries the `Cargo.lock` the git tree omits. Building from the tag first, I had to generate a lockfile and keep it beside the expression -- ours to maintain, and drifting from whatever upstream tested against. Taking the crate hands that back to upstream. The cost is that the pin is a plain URL rather than a GitHub release, so npins cannot discover new versions: bumping duvet means editing the version in npins/sources.json by hand. That is written down in the package expression next to the pin it constrains. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit e274d99bd6c8f7771fcfc974ba9d31ea343ec58b) --- default.nix | 1 + nix/overlays/dataplane-dev.nix | 6 ++++++ nix/pkgs/duvet/default.nix | 36 ++++++++++++++++++++++++++++++++++ npins/sources.json | 6 ++++++ 4 files changed, 49 insertions(+) create mode 100644 nix/pkgs/duvet/default.nix diff --git a/default.nix b/default.nix index 96928496c0..4fb8b88446 100644 --- a/default.nix +++ b/default.nix @@ -166,6 +166,7 @@ let cargo-show-asm commitlint-rs direnv + duvet gateway-crd gettext jq diff --git a/nix/overlays/dataplane-dev.nix b/nix/overlays/dataplane-dev.nix index defb4311eb..e318617b71 100644 --- a/nix/overlays/dataplane-dev.nix +++ b/nix/overlays/dataplane-dev.nix @@ -12,6 +12,12 @@ let }; in { + # callPackage rather than a bare import: duvet links against OpenSSL and so needs pkg-config and + # the library from the package set, while still taking our overridden rust toolchain. + duvet = final.callPackage ../pkgs/duvet { + src = sources.duvet; + inherit (override-packages) rustPlatform; + }; kopium = import ../pkgs/kopium ( override-packages // { diff --git a/nix/pkgs/duvet/default.nix b/nix/pkgs/duvet/default.nix new file mode 100644 index 0000000000..14377edd96 --- /dev/null +++ b/nix/pkgs/duvet/default.nix @@ -0,0 +1,36 @@ +# duvet: specification compliance coverage. +# +# Parses a specification into its individual requirements and matches them against citations left +# in the source -- `//= ` followed by the requirement's text, quoted -- so that a requirement +# with no implementation, or an implementation with no test, becomes visible. +# +# It answers the question neither bolero nor cargo-mutants can. Bolero says a property holds; +# cargo-mutants says enough properties exist that nothing goes unasserted; neither can say the +# properties are the ones the specification asked for. See development/code/mutation-testing.md. +# +# Taken from the published crate rather than the git tag, deliberately. duvet embeds +# `www/public/script.js` with `include_str!`, and that file is a JavaScript build product: it is +# absent from the git tree and present in the crate. Building from the tag would mean carrying a +# node toolchain and a second lockfile to produce a file the crate already ships. The crate also +# carries the `Cargo.lock` the git tree omits, so the dependency set is pinned upstream instead of +# by us. +# +# The cost of that choice is that the pin is a plain URL, so npins cannot discover new versions: +# bumping duvet means editing the version in npins/sources.json by hand. +{ + src, + rustPlatform, + pkg-config, + openssl, + ... +}: +rustPlatform.buildRustPackage (final: { + pname = "duvet"; + version = "0.4.2"; + src = src.outPath; + cargoLock.lockFile = "${final.src}/Cargo.lock"; + nativeBuildInputs = [ pkg-config ]; + buildInputs = [ openssl ]; + # Upstream's tests reach the network to fetch the specifications they exercise. + doCheck = false; +}) diff --git a/npins/sources.json b/npins/sources.json index fde579c3b7..722332c937 100644 --- a/npins/sources.json +++ b/npins/sources.json @@ -87,6 +87,12 @@ "url": "https://github.com/githedgehog/dplane-rpc/archive/e8fc33db10e1d00785f2a2b90cbadcad7900f200.tar.gz", "hash": "sha256-tjN4qSbKrWfosOV3wt2AnQxmVL0BPZYBjAHG3X00+aM=" }, + "duvet": { + "type": "Url", + "url": "https://static.crates.io/crates/duvet/duvet-0.4.2.crate", + "unpack": true, + "hash": "sha256-ey5eGuJv65ARtv+q+LDYMRunfOliNGHCTJ24tMgXbjs=" + }, "fabric": { "type": "GitRelease", "repository": { From 90a2468709f8a1d54105461be7bfe65eaac2738a Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 20 Aug 2026 15:11:15 -0600 Subject: [PATCH 14/30] build(nix): Add deno to the dev toolchain For `scripts/spec-interlock.ts`. TypeScript over Python because more of the team reads it. (cherry picked from commit 0b1f665d510ada4ae2b68b152ae54b576ed869bc) --- default.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/default.nix b/default.nix index 4fb8b88446..6eaefdc632 100644 --- a/default.nix +++ b/default.nix @@ -165,6 +165,7 @@ let cargo-nextest cargo-show-asm commitlint-rs + deno direnv duvet gateway-crd From 1888d67225e25609902016158608cb835869e261 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 20 Aug 2026 15:11:21 -0600 Subject: [PATCH 15/30] feat(spec): Cross-check a test citation against its implementation citation duvet checks that a `type=test` citation exists, not that it tests anything. Both citations are comments, so a refactor separates them silently and the requirement still reports green. Two things the tool has to work around are not visible in the diff: - cargo-mutants applies neither `--re` nor `--exclude-re` nor `.cargo/mutants.toml` to StructField-genre mutants, so its own filters cannot be trusted as the selector; the results are filtered here instead. - A cited test that has been renamed makes the nextest filter match nothing, which would report every mutant as surviving. Hence the `stale` outcome and the `cargo nextest list` check before any mutant runs. (cherry picked from commit 347c260d57e0b8810c09d42db5fcc56a05c0b40a) --- scripts/spec-interlock.ts | 487 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 487 insertions(+) create mode 100755 scripts/spec-interlock.ts diff --git a/scripts/spec-interlock.ts b/scripts/spec-interlock.ts new file mode 100755 index 0000000000..d1a8f00664 --- /dev/null +++ b/scripts/spec-interlock.ts @@ -0,0 +1,487 @@ +#!/usr/bin/env -S deno run --allow-read --allow-run --allow-write +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +/** + * Check that a `type=test` citation actually tests its `type=implementation` citation. + * + * duvet checks that a citation *exists*. It cannot check that the test named by one says + * anything about the code named by the other: both are comments, and a refactor can separate + * them without either changing. That gap is what makes a citation decorative, and it is + * invisible in every report the three tools produce on their own. + * + * This closes it by making the tools check each other. For each requirement duvet has matched + * to both an implementation and a test, mutate *only* the cited implementation region and run + * *only* the cited tests. A mutant that survives is a change to the code that claims to + * implement the requirement which the test claiming to check it does not notice. + * + * The unit is a (requirement, implementation, test) triple rather than a file, because that is + * what the claim is about. `cargo mutants -f ` would answer a weaker question -- "is this + * file tested" -- and would drown the signal in mutants belonging to requirements nobody cited. + */ + +// No imports, deliberately. `jsr:@std/...` would be fetched on first run, which puts this tool +// behind the network in exactly the situation it is most wanted -- a CI or nix sandbox with +// none -- for a path join and an argument parse. The same reasoning vendors the specifications +// under `.duvet/`; see development/code/spec-compliance.md. + +const join = (...parts: string[]) => parts.join("/").replaceAll(/\/+/g, "/"); +const REPO = new URL("..", import.meta.url).pathname.replace(/\/$/, ""); + +/** + * duvet emits no `type` key for the default annotation kind, which is the implementation + * citation. Spelling it here keeps the defaulting in one place. + */ +const IMPLEMENTATION = "CITATION"; +const TEST = "TEST"; + +/** A duvet citation line: `//= `, `//= type=`, or the quoted text `//# ...`. */ +const CITATION_LINE = /^\s*\/\/[=#]/; +const ATTRIBUTE_LINE = /^\s*#\[/; +/** `fn name(`, `pub fn name(`, `async fn name(`. The name is what nextest filters on. */ +const FN_LINE = /\bfn\s+([A-Za-z_][A-Za-z0-9_]*)/; +/** A mutant as cargo-mutants names it in `caught.txt` and friends: `path:line:col: what`. */ +const MUTANT_LINE = /^([^:]+):(\d+):(\d+): /; + +/** A half-open line range `[start, end)`, 1-indexed, in `path`. */ +interface Region { + path: string; + start: number; + end: number; +} + +const showRegion = (r: Region) => `${r.path}:${r.start}-${r.end - 1}`; + +/** One requirement, the code cited as implementing it, and the tests cited as checking it. */ +interface Triple { + index: string; + spec: string; + section: string; + implementations: Region[]; + tests: string[]; + testSites: Region[]; +} + +interface Annotation { + source: string; + target_path: string; + target_section: string; + line: number; + type?: string; +} + +interface Status { + citation?: number; + test?: number; + related?: number[]; +} + +interface Report { + annotations: Annotation[]; + statuses: Record; +} + +async function run( + cmd: string, + args: string[], +): Promise<{ code: number; stdout: string }> { + const output = await new Deno.Command(cmd, { + args, + cwd: REPO, + stdout: "piped", + stderr: "piped", + }).output(); + return { code: output.code, stdout: new TextDecoder().decode(output.stdout) }; +} + +/** + * Run `duvet report` and read back its JSON. + * + * Always regenerated rather than cached: a stale report would silently check the previous + * commit's citations, which is the failure this tool exists to catch. + */ +async function duvetReport(jsonPath: string): Promise { + const { code } = await run("duvet", ["report", "--json", jsonPath]); + if (code !== 0) throw new Error("duvet report failed"); + return JSON.parse(await Deno.readTextFile(jsonPath)); +} + +/** + * The first index at or after `start` that is not part of a citation comment. + * + * Ordinary `//` comments between the citation and the code it annotates are prose about the + * citation -- every existing site has some -- so they are skipped too, as are the attributes + * and doc comments that precede an item. + */ +function skipCitationBlock(lines: string[], start: number): number { + let i = start; + while (i < lines.length) { + const line = lines[i]; + const stripped = line.trim(); + if ( + stripped === "" || CITATION_LINE.test(line) || + stripped.startsWith("//") || ATTRIBUTE_LINE.test(line) + ) { + i += 1; + continue; + } + break; + } + return i; +} + +/** + * The line index just past the item or statement beginning at `start`. + * + * Brace matching rather than a Rust parser: the cited construct is a statement or a single + * item, and the alternative -- taking the whole enclosing function -- would attribute mutants + * to a requirement that does not cover them. + */ +function itemExtent(lines: string[], start: number): number { + let depth = 0; + let opened = false; + let i = start; + while (i < lines.length) { + // Strings and char literals containing braces would break this. None of the cited sites + // has one, and a miscount shows up as a region that fails to bound a mutant, not as a + // silently wrong verdict. + const code = lines[i].split("//")[0]; + const opens = (code.match(/\{/g) ?? []).length; + depth += opens - (code.match(/\}/g) ?? []).length; + if (opens > 0) opened = true; + i += 1; + if (opened && depth <= 0) return i; + if (!opened && code.trimEnd().endsWith(";")) return i; + } + return i; +} + +async function readLines(path: string): Promise { + return (await Deno.readTextFile(path)).split("\n"); +} + +/** The code a `type=implementation` citation at `line` annotates. */ +async function implementationRegion( + source: string, + line: number, +): Promise { + const lines = await readLines(join(REPO, source)); + const start = skipCitationBlock(lines, line - 1); + return { path: source, start: start + 1, end: itemExtent(lines, start) + 1 }; +} + +/** The name of the test function a `type=test` citation at `line` annotates. */ +async function testName( + source: string, + line: number, +): Promise<[string, Region]> { + const lines = await readLines(join(REPO, source)); + const start = skipCitationBlock(lines, line - 1); + for (let i = start; i < Math.min(start + 5, lines.length); i += 1) { + const match = FN_LINE.exec(lines[i]); + if (match) { + return [match[1], { + path: source, + start: i + 1, + end: itemExtent(lines, i) + 1, + }]; + } + } + throw new Error(`${source}:${line}: a type=test citation does not precede a function`); +} + +/** Every requirement duvet has matched to both an implementation and a test. */ +async function collect(report: Report): Promise { + const triples: Triple[] = []; + for (const [index, status] of Object.entries(report.statuses)) { + if (status.citation === undefined || status.test === undefined) continue; + let triple: Triple | undefined; + for (const j of status.related ?? []) { + const annotation = report.annotations[j]; + triple ??= { + index, + spec: annotation.target_path, + section: annotation.target_section, + implementations: [], + tests: [], + testSites: [], + }; + const kind = annotation.type ?? IMPLEMENTATION; + if (kind === IMPLEMENTATION) { + triple.implementations.push( + await implementationRegion(annotation.source, annotation.line), + ); + } else if (kind === TEST) { + const [name, site] = await testName(annotation.source, annotation.line); + triple.tests.push(name); + triple.testSites.push(site); + } + } + if (triple && triple.implementations.length && triple.tests.length) { + triples.push(triple); + } + } + return triples; +} + +/** The cargo package owning a workspace-relative source path. */ +async function packageOf(path: string): Promise { + const manifest = join(REPO, path.split("/")[0], "Cargo.toml"); + for (const line of (await Deno.readTextFile(manifest)).split("\n")) { + if (line.startsWith("name")) { + return line.split("=")[1].trim().replaceAll('"', ""); + } + } + throw new Error(`${manifest}: no package name`); +} + +/** + * A `cargo mutants --re` pattern narrowing the run towards `regions`. + * + * Best effort only, for cost. It cannot be trusted as the selector: cargo-mutants applies + * neither `--re` nor `--exclude-re` nor `.cargo/mutants.toml` to `StructField`-genre mutants, + * so a pattern matching nothing still yields every "delete field X from struct Y" in the + * package. `selectRegions` is what actually decides which mutants count. + */ +function mutantFilter(regions: Region[]): string { + const byFile = new Map>(); + for (const region of regions) { + const lines = byFile.get(region.path) ?? new Set(); + for (let n = region.start; n < region.end; n += 1) lines.add(n); + byFile.set(region.path, lines); + } + const parts = [...byFile.entries()].sort().map(([path, lines]) => { + const escaped = path.replaceAll(".", "\\."); + return `${escaped}:(${[...lines].sort((a, b) => a - b).join("|")}):`; + }); + return `^(?:${parts.join("|")})`; +} + +/** + * The mutants that fall inside a cited implementation region. + * + * The verdict is computed here rather than delegated to cargo-mutants' own filters, because + * those leak (see `mutantFilter`). A mutant outside every cited region says nothing about the + * citation under test and is discarded rather than counted either way. + */ +function selectRegions(mutants: string[], regions: Region[]): string[] { + return mutants.filter((mutant) => { + const match = MUTANT_LINE.exec(mutant); + if (!match) return false; + const [, path, line] = match; + return regions.some((r) => + r.path === path && r.start <= Number(line) && Number(line) < r.end + ); + }); +} + +/** + * Whether the cited test names match at least one test nextest will run. + * + * Without this the tool's central failure mode is silent: a renamed test makes the filter match + * nothing, nextest exits 0 having run nothing, every mutant survives, and a citation that is + * merely stale is reported as decorative. + */ +async function selectsATest(pkg: string, tests: string[]): Promise { + const { code, stdout } = await run("cargo", [ + "nextest", + "list", + "-p", + pkg, + ...tests, + ]); + return code === 0 && tests.some((name) => stdout.includes(name)); +} + +interface Result { + outcome: "held" | "decorative" | "stale" | "unsupported" | "no-mutants"; + detail?: string; + caught?: string[]; + missed?: string[]; + unviable?: string[]; + timeout?: string[]; +} + +/** Mutate the cited implementation, run only the cited tests, and report survivors. */ +async function runTriple( + triple: Triple, + output: string, + jobs: number, +): Promise { + const pkg = await packageOf(triple.implementations[0].path); + const testPackages = new Set( + await Promise.all(triple.testSites.map((s) => packageOf(s.path))), + ); + if (testPackages.size !== 1 || !testPackages.has(pkg)) { + return { + outcome: "unsupported", + detail: `implementation in ${pkg}, tests in ${[...testPackages].join(", ")}`, + }; + } + if (!await selectsATest(pkg, triple.tests)) { + return { + outcome: "stale", + detail: `no test in ${pkg} matches ${triple.tests.join(", ")}`, + }; + } + + const outDir = join(output, `requirement-${triple.index}`); + const files = [...new Set(triple.implementations.map((r) => r.path))].sort(); + await run("cargo", [ + "mutants", + "--package", + pkg, + "--test-tool", + "nextest", + ...files.flatMap((path) => ["--file", path]), + "--re", + mutantFilter(triple.implementations), + "--output", + outDir, + "--jobs", + String(jobs), + "--no-times", + "--", + ...triple.tests, + ]); + + const read = async (name: string): Promise => { + const path = join(outDir, "mutants.out", `${name}.txt`); + let text: string; + try { + text = await Deno.readTextFile(path); + } catch { + return []; + } + return selectRegions( + text.split("\n").filter((l) => l.trim() !== ""), + triple.implementations, + ); + }; + + const [caught, missed, unviable, timeout] = await Promise.all( + ["caught", "missed", "unviable", "timeout"].map(read), + ); + // A region whose every mutant is unviable or timed out is not evidence either way: nothing + // ran that the test could have noticed. Reporting that as "held" would credit the citation + // for a check that never happened, which is the exact failure this tool exists to catch. + if (!caught.length && !missed.length) { + const why = unviable.length || timeout.length + ? `${unviable.length} unviable and ${timeout.length} timed out, so none could be tested` + : "generated no mutants"; + return { + outcome: "no-mutants", + detail: `the cited region ${ + triple.implementations.map(showRegion).join(", ") + } ${why}; the citation cannot be checked this way`, + unviable, + timeout, + }; + } + return { + outcome: missed.length ? "decorative" : "held", + caught, + missed, + unviable, + timeout, + }; +} + +/** `--flag value` and `--flag`, which is all this tool needs. */ +function parseArgs(argv: string[]) { + const args = { + list: false, + help: false, + only: [] as string[], + jobs: 4, + output: join(REPO, "target", "spec-interlock"), + json: "/tmp/duvet-interlock.json", + }; + for (let i = 0; i < argv.length; i += 1) { + const flag = argv[i]; + const value = () => { + const next = argv[++i]; + if (next === undefined) throw new Error(`${flag} needs a value`); + return next; + }; + switch (flag) { + case "--list": args.list = true; break; + case "--help": args.help = true; break; + case "--only": args.only.push(value()); break; + case "--jobs": args.jobs = Number(value()); break; + case "--output": args.output = value(); break; + case "--json": args.json = value(); break; + default: throw new Error(`unknown argument ${flag}`); + } + } + return args; +} + +async function main(): Promise { + const args = parseArgs(Deno.args); + if (args.help) { + console.log( + "usage: spec-interlock.ts [--list] [--only ]... [--jobs N]", + ); + return 0; + } + + const report = await duvetReport(args.json); + let triples = await collect(report); + if (args.only.length) triples = triples.filter((t) => args.only.includes(t.index)); + + if (args.list) { + for (const triple of triples) { + console.log(`${triple.spec}#${triple.section} (requirement ${triple.index})`); + for (const region of triple.implementations) { + console.log(` implementation ${showRegion(region)}`); + } + triple.tests.forEach((name, i) => { + console.log(` test ${name} [${showRegion(triple.testSites[i])}]`); + }); + console.log(); + } + console.log( + `${triples.length} requirements carry both an implementation and a test`, + ); + return 0; + } + + await Deno.mkdir(args.output, { recursive: true }); + let failures = 0; + for (const triple of triples) { + console.log(`==> ${triple.spec}#${triple.section} (requirement ${triple.index})`); + const result = await runTriple(triple, args.output, Number(args.jobs)); + if (result.outcome === "held") { + console.log( + ` held: ${result.caught!.length} mutants in the cited region, all caught by ${ + triple.tests.join(", ") + }`, + ); + } else if (result.outcome === "decorative") { + failures += 1; + console.log( + ` DECORATIVE: ${result.missed!.length} mutants survive ${triple.tests.join(", ")}`, + ); + for (const mutant of result.missed!) console.log(` ${mutant}`); + } else { + failures += 1; + console.log(` ${result.outcome.toUpperCase()}: ${result.detail}`); + } + // Counted but not listed: an unviable mutant is one that did not compile, which says + // nothing about the test. The count is kept because a region that is *entirely* unviable + // is the `no-mutants` case above, and that distinction is worth being able to see. + const skipped = (result.unviable?.length ?? 0) + (result.timeout?.length ?? 0); + if (skipped && result.outcome !== "no-mutants") { + console.log(` (${skipped} unviable or timed out, not counted either way)`); + } + } + + console.log(); + console.log( + `${triples.length - failures}/${triples.length} requirements hold their citations`, + ); + return failures ? 1 : 0; +} + +if (import.meta.main) Deno.exit(await main()); From fa4dca5f4a0b45f4ef8432acdd9780a529c94aaf Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 20 Aug 2026 15:11:26 -0600 Subject: [PATCH 16/30] build(just): Recipe-ise duvet, cargo-mutants, and the interlock `duvet-check` is a gate; the other three are not. duvet report is 76ms and bit-for-bit deterministic, which is what makes gating it affordable, and the snapshot having already drifted is what makes it necessary. (cherry picked from commit c787026d0d45237d59ab6ac8aa3fbfbca66fbecd) --- justfile | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/justfile b/justfile index 05e7e268f8..45cb7d752b 100644 --- a/justfile +++ b/justfile @@ -1109,6 +1109,45 @@ coverage *args: cargo llvm-cov report --branch --codecov --output-path="${out}/codecov.json" cargo llvm-cov report --branch --summary-only +# Report specification compliance. See development/code/spec-compliance.md +[script] +duvet *args: + {{ _just_debuggable_ }} + duvet report {{ args }} + +# Fail if the compliance snapshot is out of date +[script] +duvet-check: + {{ _just_debuggable_ }} + # `duvet report` takes milliseconds and is bit-for-bit deterministic, so unlike mutation + # testing this can be a gate, and it is the cheapest correctness check in the repo. It is + # one because the snapshot had already drifted two commits after being introduced: a + # regenerate-by-hand rule is one nobody runs. + duvet report + if ! git diff --quiet -- .duvet/snapshot.txt; then + echo "error: .duvet/snapshot.txt is stale; run \`just duvet\` and commit the result" >&2 + git --no-pager diff -- .duvet/snapshot.txt >&2 + exit 1 + fi + +# Mutation-test a crate or a diff. See development/code/mutation-testing.md +[script] +mutants *args: + {{ _just_debuggable_ }} + # Deliberately not a gate, and deliberately not the whole workspace by default: a full + # sweep is hours, and the product is the list of survivors rather than the score. Scope + # it, as in `just mutants -p dataplane-nat` or `just mutants --in-diff <(git diff main)`. + cargo mutants --test-tool nextest {{ args }} + +# Check that each `type=test` citation tests its `type=implementation` citation +[script] +spec-interlock *args: + {{ _just_debuggable_ }} + # The cross-check duvet cannot do alone: mutate only the cited implementation region, run + # only the cited tests, and report a citation whose test notices nothing as decorative. + # See development/code/spec-compliance.md. + ./scripts/spec-interlock.ts {{ args }} + # Use Nix-built archives so local and CI coverage report the same binaries. [script] coverage-archive package="tests.all" *args: From 931636e01a39db1dd7fac420adcdbcbff41aa835 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 11:45:21 -0600 Subject: [PATCH 17/30] feat(spec): Let the interlock classify a mutant instead of only killing it RFC 4787 REQ-2's surviving mutant is equivalent: `reuse_allocated_ip` can only return `NoFreeIp` from a well-formed pool, so forcing the exhaustion guard to `true` changes nothing. The whole nat suite passes with it applied. Without somewhere to record that, the tool reports it as decorative forever and the cheapest way to clear it is a test asserting whatever the code already does -- the entrenchment `development/code/mutation-testing.md` warns about, which is the failure this whole procedure exists to avoid. An accept is printed on every run and must still match a live mutant, so the list cannot quietly become a way of not looking. (cherry picked from commit 54a842bee32f80c5fc6c892bac7c52ba8469b9bc) --- scripts/spec-interlock.ts | 88 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 2 deletions(-) diff --git a/scripts/spec-interlock.ts b/scripts/spec-interlock.ts index d1a8f00664..8685fc98c8 100755 --- a/scripts/spec-interlock.ts +++ b/scripts/spec-interlock.ts @@ -43,6 +43,49 @@ const FN_LINE = /\bfn\s+([A-Za-z_][A-Za-z0-9_]*)/; /** A mutant as cargo-mutants names it in `caught.txt` and friends: `path:line:col: what`. */ const MUTANT_LINE = /^([^:]+):(\d+):(\d+): /; +/** + * A mutant that survives its requirement's cited tests, and why that is the correct outcome. + * + * `development/code/mutation-testing.md` asks for every mutant to be *classified*, not killed: + * some are equivalent, and the cheapest way to turn one green is to assert whatever the code + * already does, which entrenches the behaviour instead of checking it. Without somewhere to + * record that judgement the interlock reports an equivalent mutant as decorative forever, and + * the pressure is to write the entrenching test. + * + * Kept here rather than in a data file for the reason `.cargo/mutants.toml` gives for holding + * cargo-mutants' exclusions: the reasoning stays in one place, next to what acts on it. + */ +interface Accepted { + /** The requirement, so an accept cannot silently cover a different citation. */ + requirement: string; + /** cargo-mutants' own stable name -- `: `, with no line or column, so that the + * entry survives the function being moved or reformatted. */ + mutant: string; + reason: string; +} + +const ACCEPTED: Accepted[] = [ + { + requirement: "https://www.rfc-editor.org/rfc/rfc4787#section-4.1", + mutant: + "nat/src/masquerade/apalloc/alloc.rs: replace match guard e.is_exhaustion() with true in IpAllocator::allocate", + reason: + "Equivalent under the allocator's invariants. `reuse_allocated_ip` skips `NoFreePort` " + + "and loops, so the only error it can return from a well-formed pool is `NoFreeIp`, " + + "which is exhaustion; the non-exhaustion arm is defensive depth against an internal " + + "inconsistency. The whole 210-test nat suite passes with the guard forced to `true`. " + + "The guard is kept because a future allocator that can fail for a non-exhaustion reason " + + "must not draw a second public address for a host that already holds one.", + }, +]; + +/** cargo-mutants' stable name for a mutant: its `: `, dropping line and column. */ +function stableName(mutant: string): string { + const match = MUTANT_LINE.exec(mutant); + if (!match) return mutant; + return `${match[1]}: ${mutant.slice(match[0].length)}`; +} + /** A half-open line range `[start, end)`, 1-indexed, in `path`. */ interface Region { path: string; @@ -298,6 +341,7 @@ interface Result { detail?: string; caught?: string[]; missed?: string[]; + accepted?: Accepted[]; unviable?: string[]; timeout?: string[]; } @@ -307,6 +351,7 @@ async function runTriple( triple: Triple, output: string, jobs: number, + used: Set, ): Promise { const pkg = await packageOf(triple.implementations[0].path); const testPackages = new Set( @@ -359,9 +404,25 @@ async function runTriple( ); }; - const [caught, missed, unviable, timeout] = await Promise.all( + const [caught, survived, unviable, timeout] = await Promise.all( ["caught", "missed", "unviable", "timeout"].map(read), ); + + // Split the survivors into the ones somebody has judged equivalent and the ones nobody has. + // Only the second kind is a finding. + const accepted: Accepted[] = []; + const missed = survived.filter((mutant) => { + const entry = ACCEPTED.find((a) => + a.requirement === `${triple.spec}#${triple.section}` && + a.mutant === stableName(mutant) + ); + if (entry) { + accepted.push(entry); + used.add(entry); + return false; + } + return true; + }); // A region whose every mutant is unviable or timed out is not evidence either way: nothing // ran that the test could have noticed. Reporting that as "held" would credit the citation // for a check that never happened, which is the exact failure this tool exists to catch. @@ -382,6 +443,7 @@ async function runTriple( outcome: missed.length ? "decorative" : "held", caught, missed, + accepted, unviable, timeout, }; @@ -448,10 +510,11 @@ async function main(): Promise { } await Deno.mkdir(args.output, { recursive: true }); + const used = new Set(); let failures = 0; for (const triple of triples) { console.log(`==> ${triple.spec}#${triple.section} (requirement ${triple.index})`); - const result = await runTriple(triple, args.output, Number(args.jobs)); + const result = await runTriple(triple, args.output, Number(args.jobs), used); if (result.outcome === "held") { console.log( ` held: ${result.caught!.length} mutants in the cited region, all caught by ${ @@ -475,6 +538,27 @@ async function main(): Promise { if (skipped && result.outcome !== "no-mutants") { console.log(` (${skipped} unviable or timed out, not counted either way)`); } + // Printed on every run, not hidden. An accepted mutant is a judgement somebody made, and it + // should be as visible as the finding it replaced -- otherwise the list only ever grows. + for (const entry of result.accepted ?? []) { + console.log(` accepted: ${entry.mutant}`); + console.log(` ${entry.reason}`); + } + } + + // An accept that matches nothing is worse than no accept: it reads as a considered judgement + // while silently covering a mutant that no longer exists, and it would go on hiding whatever + // takes that name next. + const stale = ACCEPTED.filter((entry) => !used.has(entry)); + const checkedAll = !args.only.length; + if (stale.length && checkedAll) { + failures += stale.length; + console.log(); + for (const entry of stale) { + console.log(`STALE ACCEPT: no surviving mutant matches`); + console.log(` requirement ${entry.requirement}`); + console.log(` mutant ${entry.mutant}`); + } } console.log(); From 8b75611ec040444dbd88d47da7218f930535adc3 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 12:53:07 -0600 Subject: [PATCH 18/30] feat(spec): Say why a mutant survived, using coverage A surviving mutant has two possible causes needing opposite fixes, and the mutation run cannot tell them apart: the test never reached the line, or it ran straight through and did not care. Splitting the ten survivors on RFC 4787 REQ-3 by hand took longer than the run that found them. Coverage answers it directly, and is cheap enough to run first: a cited test that executes none of the cited region fails outright, with no mutants built at all. It must not become a threshold. A caught mutant was necessarily executed, so coverage adds nothing wherever mutation already succeeds -- only zero is decisive, and only as an error. Failure to collect it returns null rather than an empty map, because an empty map reads as "nothing was executed" and would relabel every survivor as unreached. (cherry picked from commit dc24092ead58d399a80e9125d30c38b57f86b353) --- scripts/spec-interlock.ts | 151 +++++++++++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 4 deletions(-) diff --git a/scripts/spec-interlock.ts b/scripts/spec-interlock.ts index 8685fc98c8..83dc35c3b1 100755 --- a/scripts/spec-interlock.ts +++ b/scripts/spec-interlock.ts @@ -127,14 +127,96 @@ interface Report { async function run( cmd: string, args: string[], -): Promise<{ code: number; stdout: string }> { + env: Record = {}, +): Promise<{ code: number; stdout: string; stderr: string }> { const output = await new Deno.Command(cmd, { args, cwd: REPO, + env, stdout: "piped", stderr: "piped", }).output(); - return { code: output.code, stdout: new TextDecoder().decode(output.stdout) }; + const decode = (b: Uint8Array) => new TextDecoder().decode(b); + return { + code: output.code, + stdout: decode(output.stdout), + stderr: decode(output.stderr), + }; +} + +/** + * How many times the cited tests executed each line of the files they are cited against. + * + * Cheap next to mutation -- one instrumented run rather than one build and test cycle per + * mutant -- and it answers a question mutation cannot separate out on its own: a mutant that + * survives because the test never reached it needs a different input, while one that survives + * although the test ran straight through it needs a different assertion. + * + * The instrumented build gets its own target directory. Sharing the default one would make + * every ordinary `cargo test` afterwards rebuild from scratch, because the coverage flags + * differ. + */ +async function lineCounts( + pkg: string, + tests: string[], + paths: string[], + outDir: string, + sharedTarget: string, +): Promise | null> { + // Added to the parent environment rather than replacing it: `Deno.Command` inherits unless + // `clearEnv` is set, which keeps PATH and the toolchain selection intact without this tool + // needing `--allow-env`. + const env = { + LLVM_COV: join(REPO, "devroot", "bin", "llvm-cov"), + LLVM_PROFDATA: join(REPO, "devroot", "bin", "llvm-profdata"), + // Shared across requirements, not per-requirement: the instrumented build is the slow part + // and is identical for every citation in the same package. + CARGO_TARGET_DIR: sharedTarget, + }; + // `--output-path` will not create the directory it writes into. + await Deno.mkdir(outDir, { recursive: true }); + + // Returning null rather than an empty map matters: an empty map is indistinguishable from + // "nothing was executed", and would relabel every survivor as unreached -- a confident wrong + // answer, which is worse than no answer. + const step = async (args: string[]) => { + const { code, stderr } = await run("cargo", args, env); + if (code !== 0) { + console.log(` warning: coverage step \`${args.join(" ")}\` failed`); + const tail = stderr.trim().split("\n").slice(-3).join("\n "); + if (tail) console.log(` ${tail}`); + } + return code === 0; + }; + + // Only the profiles, not the build: the build is the slow part and is reusable. + const jsonPath = join(outDir, "coverage.json"); + const ok = + await step(["llvm-cov", "clean", "--workspace", "--profraw-only"]) && + await step(["llvm-cov", "--no-report", "--branch", "nextest", "-p", pkg, ...tests]) && + await step(["llvm-cov", "report", "--json", "--output-path", jsonPath]); + if (!ok) return null; + + const counts = new Map(); + let report; + try { + report = JSON.parse(await Deno.readTextFile(jsonPath)); + } catch (e) { + console.log(` warning: coverage report unreadable: ${e}`); + return null; + } + for (const file of report.data?.[0]?.files ?? []) { + const match = paths.find((p) => file.filename.endsWith(p)); + if (!match) continue; + // A segment is [line, column, count, hasCount, ...]. One line can carry several, so the + // largest wins: the question here is only whether execution ever got there. + for (const [line, _col, count, hasCount] of file.segments ?? []) { + if (!hasCount) continue; + const key = `${match}:${line}`; + counts.set(key, Math.max(counts.get(key) ?? 0, count)); + } + } + return counts; } /** @@ -337,11 +419,21 @@ async function selectsATest(pkg: string, tests: string[]): Promise { } interface Result { - outcome: "held" | "decorative" | "stale" | "unsupported" | "no-mutants"; + outcome: + | "held" + | "decorative" + | "uncovered" + | "stale" + | "unsupported" + | "no-mutants"; detail?: string; caught?: string[]; missed?: string[]; accepted?: Accepted[]; + /** Survivors on a line the cited tests never executed: the test needs a different input. */ + unreached?: string[]; + /** Survivors on a line they did execute: the test needs a different assertion. */ + tolerated?: string[]; unviable?: string[]; timeout?: string[]; } @@ -372,6 +464,30 @@ async function runTriple( const outDir = join(output, `requirement-${triple.index}`); const files = [...new Set(triple.implementations.map((r) => r.path))].sort(); + + // Coverage first, because it is cheap and can settle the question outright. A cited test that + // never executes a single line of the cited region cannot be testing the requirement, whatever + // the mutants would have said, and skipping them saves the build-and-test cycle per mutant. + const counts = await lineCounts( + pkg, + triple.tests, + files, + outDir, + join(output, "cov-target"), + ); + const regionLines = triple.implementations.flatMap((r) => + Array.from({ length: r.end - r.start }, (_, i) => `${r.path}:${r.start + i}`) + ); + const executable = regionLines.filter((key) => counts?.has(key)); + const executed = executable.filter((key) => (counts?.get(key) ?? 0) > 0); + if (counts && executable.length && !executed.length) { + return { + outcome: "uncovered", + detail: `${triple.tests.join(", ")} never executes ${ + triple.implementations.map(showRegion).join(", ") + }; the citation cannot be testing this requirement`, + }; + } await run("cargo", [ "mutants", "--package", @@ -439,11 +555,21 @@ async function runTriple( timeout, }; } + // Split the survivors by whether the cited tests got to them at all. The two need opposite + // fixes, and telling them apart by hand meant reading a coverage report anyway. + // With no coverage there is no basis to split them, and guessing would relabel every + // survivor as unreached. Leave both buckets empty; the reporter then lists them plainly. + const reached = (mutant: string) => { + const match = MUTANT_LINE.exec(mutant); + return match ? (counts!.get(`${match[1]}:${match[2]}`) ?? 0) > 0 : false; + }; return { outcome: missed.length ? "decorative" : "held", caught, missed, accepted, + unreached: counts ? missed.filter((m) => !reached(m)) : [], + tolerated: counts ? missed.filter(reached) : [], unviable, timeout, }; @@ -526,7 +652,24 @@ async function main(): Promise { console.log( ` DECORATIVE: ${result.missed!.length} mutants survive ${triple.tests.join(", ")}`, ); - for (const mutant of result.missed!) console.log(` ${mutant}`); + if (result.unreached?.length) { + console.log( + ` unreached (${result.unreached.length}) -- the test never runs these lines, so`, + ); + console.log(` closing them means changing what it feeds, not what it asserts:`); + for (const mutant of result.unreached) console.log(` ${mutant}`); + } + if (result.tolerated?.length) { + console.log( + ` tolerated (${result.tolerated.length}) -- the test runs these lines and passes`, + ); + console.log(` anyway, so either an assertion is missing or the mutant is equivalent:`); + for (const mutant of result.tolerated) console.log(` ${mutant}`); + } + if (!result.unreached?.length && !result.tolerated?.length) { + console.log(` (unclassified -- coverage was not collected)`); + for (const mutant of result.missed!) console.log(` ${mutant}`); + } } else { failures += 1; console.log(` ${result.outcome.toUpperCase()}: ${result.detail}`); From 0ea0d48a892bcd7f7d5d8fef81381b623ae1e8dc Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 13:16:29 -0600 Subject: [PATCH 19/30] fix(spec): Accept the second-half boundary the block guard makes unreachable The tenth survivor. `allocate_port` enters the bitmap only when the block is not full, so on the second-half branch the first half is already full and a non-full block must leave a zero in the second -- `ones == 128`, the only value `<` and `<=` disagree on, cannot occur. The same mutant on the first half is caught, and should be: a block with the first half full and the second free is ordinary, so there `ones == 128` is reachable and the shift overflows. The asymmetry is the argument that this is an invariant rather than a gap. (cherry picked from commit 47f4b2e7a6c536518662e14f1c51c01b889ef8da) --- scripts/spec-interlock.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/scripts/spec-interlock.ts b/scripts/spec-interlock.ts index 83dc35c3b1..cf4dc07323 100755 --- a/scripts/spec-interlock.ts +++ b/scripts/spec-interlock.ts @@ -77,6 +77,22 @@ const ACCEPTED: Accepted[] = [ "The guard is kept because a future allocator that can fail for a non-exhaustion reason " + "must not draw a second public address for a host that already holds one.", }, + // The same code answers to both specifications, so the judgement is recorded against each. + ...["https://www.rfc-editor.org/rfc/rfc4787#section-4.2.1", + "https://www.rfc-editor.org/rfc/rfc5382#section-8"].map((requirement) => ({ + requirement, + mutant: + "nat/src/masquerade/apalloc/port_alloc.rs: replace < with <= in Bitmap256::allocate_port_from_bitmap", + reason: + "Equivalent on the second half, and unreachable rather than untested. `allocate_port` " + + "only enters the bitmap when `!is_full()`, and `bitmap_full()` is both halves at " + + "`u128::MAX`. Reaching the second-half branch means the first half is already full, so a " + + "block that is not full has a zero in the second half and `trailing_ones() < 128` always " + + "holds; `ones == 128`, the only value the two operators disagree on, cannot occur. The " + + "whole 210-test nat suite passes with `<=` applied. Note the same mutant on the *first* " + + "half is caught, and correctly: a block with the first half full and the second free is " + + "ordinary, so `ones == 128` is reachable there and `1u128 << 128` overflows.", + })), ]; /** cargo-mutants' stable name for a mutant: its `: `, dropping line and column. */ From 5be2ae562034724209d1a741b30edfa7dd4f7606 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 7 Aug 2026 00:44:16 -0600 Subject: [PATCH 20/30] fix(net): Store the checksum an incremental update computes `Checksum::increment_update_checksum` works out the new checksum and hands it back. It does not store it, despite taking `&mut self`. `EmbeddedTransport::update_checksum` called it and dropped the answer on the floor, under a comment about ignoring errors on a truncated header -- but there is no error to ignore; the return value is the checksum. So no quoted transport header has ever had its checksum updated. NAT rewrites the ports of the packet quoted inside an ICMP error and calls this to keep the checksum in step, and the call has been doing nothing. The only correct uses of the trait method in the tree are in a test, which is presumably how the shape survived. Setting it can fail, on a header too truncated to hold a checksum -- but that is a header the caller could not have read a checksum out of either, and every caller reads one first. Hence discarding that error rather than the value, which is what the comment was reaching for. update_checksum_for_address is new, and folds a change of one of the quoted packet's addresses in. TCP, UDP and ICMPv6 are checksummed over a pseudo-header built from the source and destination addresses, so rewriting one leaves the quoted checksum describing an address that is no longer there. ICMPv4 has no pseudo-header and is left alone. Incremental for the same reason as the rest: a quote is usually truncated, so there is no payload to compute over from scratch. The tests build the one case where both routes are open -- a full header over a known payload -- and hold the fold against a computation from scratch, over two words for v4 and eight for v6. The checksum starts out correct on purpose: an RFC 1624 update is exact given a correct starting value and says nothing given a wrong one. Both fail against the discarded update; the ICMPv4 one passes either way, which is the point of it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 858897f18e62e1d2d49a54c0f916ed0e10429087) --- net/src/headers/embedded.rs | 171 ++++++++++++++++++++++++++++++++++-- 1 file changed, 163 insertions(+), 8 deletions(-) diff --git a/net/src/headers/embedded.rs b/net/src/headers/embedded.rs index 8017c2231f..c98128d78b 100644 --- a/net/src/headers/embedded.rs +++ b/net/src/headers/embedded.rs @@ -22,6 +22,7 @@ use crate::udp::{TruncatedUdp, UdpChecksum, UdpPort}; use arrayvec::ArrayVec; use core::fmt::Debug; use derive_builder::Builder; +use std::net::IpAddr; use std::num::NonZero; #[cfg(any(test, feature = "bolero"))] @@ -609,42 +610,87 @@ impl EmbeddedTransport { } } + /// Fold the change of a 16-bit field into the header's checksum. + /// + /// `increment_update_checksum` computes the new checksum and hands it back; it does not store + /// it, despite taking `&mut self`. Setting it is the point of this method, and it is what makes + /// a translated quote's checksum agree with the bytes around it. + /// + /// Setting fails only on a header too truncated to hold a checksum, which is a header the + /// caller could not have read one out of either, so there is nothing to do about it here. pub fn update_checksum(&mut self, current_checksum: u16, old_value: u16, new_value: u16) { match self { EmbeddedTransport::Tcp(tcp) => { - // Silently ignore errors if transport header is truncated - let _ = tcp.increment_update_checksum( + let updated = tcp.increment_update_checksum( TcpChecksum::new(current_checksum), old_value, new_value, ); + let _ = tcp.set_checksum(updated); } EmbeddedTransport::Udp(udp) => { - // Silently ignore errors if transport header is truncated - let _ = udp.increment_update_checksum( + let updated = udp.increment_update_checksum( UdpChecksum::new(current_checksum), old_value, new_value, ); + let _ = udp.set_checksum(updated); } EmbeddedTransport::Icmp4(icmp) => { - // Silently ignore errors if transport header is truncated - let _ = icmp.increment_update_checksum( + let updated = icmp.increment_update_checksum( Icmp4Checksum::new(current_checksum), old_value, new_value, ); + let _ = icmp.set_checksum(updated); } EmbeddedTransport::Icmp6(icmp) => { - // Silently ignore errors if transport header is truncated - let _ = icmp.increment_update_checksum( + let updated = icmp.increment_update_checksum( Icmp6Checksum::new(current_checksum), old_value, new_value, ); + let _ = icmp.set_checksum(updated); } } } + + /// Fold a change to one of the quoted packet's IP addresses into this header's checksum. + /// + /// TCP, UDP and `ICMPv6` are checksummed over a pseudo-header built from the source and + /// destination addresses, so rewriting either address of a quoted packet leaves the quoted + /// transport checksum describing an address that is no longer there. `ICMPv4` has no + /// pseudo-header and is left alone. + /// + /// Incremental, because a quote is usually truncated: there is no payload to compute a + /// checksum over from scratch, only a delta to apply to the one already there. + pub fn update_checksum_for_address(&mut self, old: IpAddr, new: IpAddr) { + if matches!(self, EmbeddedTransport::Icmp4(_)) { + return; + } + // Translating between address families would change the shape of the pseudo-header rather + // than a value inside it, so there would be no delta to fold. + if old.is_ipv4() != new.is_ipv4() { + return; + } + for (old_word, new_word) in address_words(old).into_iter().zip(address_words(new)) { + let Some(current) = self.checksum() else { + return; + }; + self.update_checksum(current, old_word, new_word); + } + } +} + +// The 16-bit words of an address, in the order a checksum sums them. +fn address_words(addr: IpAddr) -> ArrayVec { + match addr { + IpAddr::V4(addr) => { + let [a, b, c, d] = addr.octets(); + ArrayVec::from_iter([u16::from_be_bytes([a, b]), u16::from_be_bytes([c, d])]) + } + IpAddr::V6(addr) => ArrayVec::from(addr.segments()), + } } impl DeParse for EmbeddedTransport { @@ -1378,6 +1424,115 @@ mod tests { // Before calling check_full_payload, should be false assert!(!headers.is_full_payload()); } + + // Checksum folding for a rewritten address in a quoted packet. + // + // A quote is usually truncated, so folding a delta into the checksum already there is the only + // option: there is no payload to compute one over. These build the one case where both routes + // are open -- a full header over a known payload -- and hold the fold against a computation + // from scratch. + // + // The checksum starts out correct on purpose. An RFC 1624 update is exact given a correct + // starting value and says nothing at all given a wrong one, so only a correct start tests + // anything. + mod address_folding { + use super::*; + use crate::icmp4::{Icmp4, Icmp4EchoRequest, Icmp4Type}; + use crate::ip::NextHeader; + use crate::ipv4::UnicastIpv4Addr; + use crate::ipv6::UnicastIpv6Addr; + use crate::tcp::{Tcp, TcpChecksumPayload}; + use crate::udp::{UdpChecksumPayload, UdpPort}; + use std::net::{Ipv4Addr, Ipv6Addr}; + + const PAYLOAD: [u8; 4] = [0xde, 0xad, 0xbe, 0xef]; + + fn v4_net(source: Ipv4Addr, next_header: NextHeader) -> Net { + let mut ip = Ipv4::default(); + ip.set_source(UnicastIpv4Addr::new(source).unwrap_or_else(|_| unreachable!())) + .set_destination(Ipv4Addr::new(192, 168, 1, 2)) + .set_next_header(next_header); + Net::Ipv4(ip) + } + + fn v6_net(source: Ipv6Addr, next_header: NextHeader) -> Net { + let mut ip = Ipv6::default(); + ip.set_source(UnicastIpv6Addr::new(source).unwrap_or_else(|_| unreachable!())) + .set_destination(Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 2)) + .set_next_header(next_header); + Net::Ipv6(ip) + } + + /// Two words to fold, over a TCP pseudo-header. + #[test] + fn a_v4_address_change_matches_a_fresh_tcp_checksum() { + let (old, new) = (Ipv4Addr::new(192, 168, 1, 1), Ipv4Addr::new(10, 11, 12, 13)); + + let mut tcp = Tcp::new(123.try_into().unwrap(), 456.try_into().unwrap()); + tcp.update_checksum(&TcpChecksumPayload::new( + &v4_net(old, NextHeader::TCP), + &PAYLOAD, + )) + .unwrap_or_else(|()| unreachable!()); + + let mut quoted = EmbeddedTransport::Tcp(TruncatedTcp::FullHeader(tcp.clone())); + quoted.update_checksum_for_address(IpAddr::V4(old), IpAddr::V4(new)); + + let expected = tcp + .compute_checksum(&TcpChecksumPayload::new( + &v4_net(new, NextHeader::TCP), + &PAYLOAD, + )) + .unwrap_or_else(|()| unreachable!()); + assert_eq!(quoted.checksum(), Some(u16::from(expected))); + } + + /// Eight words to fold, over a UDP pseudo-header. + #[test] + fn a_v6_address_change_matches_a_fresh_udp_checksum() { + let old = Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 1); + let new = Ipv6Addr::new(0x2001, 0xdb8, 0xdead, 0xbeef, 1, 2, 3, 4); + + let mut udp = Udp::new( + UdpPort::new_checked(123).unwrap(), + UdpPort::new_checked(456).unwrap(), + ); + udp.update_checksum(&UdpChecksumPayload::new( + &v6_net(old, NextHeader::UDP), + &PAYLOAD, + )) + .unwrap_or_else(|()| unreachable!()); + + let mut quoted = EmbeddedTransport::Udp(TruncatedUdp::FullHeader(udp.clone())); + quoted.update_checksum_for_address(IpAddr::V6(old), IpAddr::V6(new)); + + let expected = udp + .compute_checksum(&UdpChecksumPayload::new( + &v6_net(new, NextHeader::UDP), + &PAYLOAD, + )) + .unwrap_or_else(|()| unreachable!()); + assert_eq!(quoted.checksum(), Some(u16::from(expected))); + } + + /// `ICMPv4` is checksummed over itself alone, so the addresses around it are none of its + /// business and folding one must not disturb it. + #[test] + fn a_v4_address_change_leaves_an_icmpv4_quote_alone() { + let mut icmp = + Icmp4::with_type(Icmp4Type::EchoRequest(Icmp4EchoRequest { id: 18, seq: 2 })); + icmp.update_checksum(&PAYLOAD) + .unwrap_or_else(|()| unreachable!()); + + let mut quoted = EmbeddedTransport::Icmp4(TruncatedIcmp4::FullHeader(icmp)); + let before = quoted.checksum(); + quoted.update_checksum_for_address( + IpAddr::V4(Ipv4Addr::new(192, 168, 1, 1)), + IpAddr::V4(Ipv4Addr::new(10, 11, 12, 13)), + ); + assert_eq!(quoted.checksum(), before); + } + } } #[cfg(any(test, feature = "bolero"))] From 88c36962b2934ee83b65ad9b567b45604ba3824f Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 14:46:06 -0600 Subject: [PATCH 21/30] test(net): Check the HeadersView unsafe boundary against the safe matcher `view.rs` was the most sensitive uncovered code in the tree, and its coverage number understated the risk. [`HeadersView`] buys zero-cost extraction with `unwrap_unchecked`: `Look::look` repeats the `ViewStep::step` chain that `sealed::Sealed::matches` already ran, and tells the compiler the `None` arms cannot happen. **Soundness rests entirely on those two chains agreeing** -- and the macro writes them out separately for every arity, eight-odd hand-written pairs, each threading the VLAN and extension cursors through by hand. That is the same shape as all six defects this campaign found in `routing`: an invariant enforced at a distance by a different function from the one relying on it. Only here a transposed cursor is not a wrong answer, it is undefined behaviour. `view.rs` sat at 42% line coverage. ## The generator was the reason, not missing tests `CommonHeaders` -- the sunny-day generator every packet test reaches for -- has six construction sites and **all six** set `vlan: ArrayVec::default()` and `net_ext: ArrayVec::default()`. It never produces a VLAN tag or an IPv6 extension header. Those are exactly the two things the view and matcher semantics are *about*: a tag the shape does not mention is a miss, extension headers are skipped silently until the shape enters the extension region and then `ExtGapCheck` turns strict. No existing generator could reach either. The tests were there; the inputs were not. Hence `ShapedHeaders`, which varies the structure: 0..=MAX_VLANS tags, and 0..=MAX_NET_EXTENSIONS extension headers of the variants that belong to the address family. Structural on purpose -- `step` walks in-memory layers with cursors, so whether `next_header` agrees with what follows it is a different property's business, and coupling the two would shrink the space this explores. Measured: **80% of generated packets carry a VLAN tag, 75% an extension header, and 20% match the shape under test.** Not vacuous. ## The oracle is the other implementation `Matcher` decides the same question safely and returns an `Option`, over the same `Within` graph and the same `ExtGapCheck`. Comparing the two is a differential test between implementations that both already exist, rather than against a third transcription of the rules. Layers are compared by **address**: two VLAN tags with equal contents pass an `assert_eq!` and are a bug if the two sides chose different ones. A shape starting at `Net` was tried and does not compile -- `Net` has no `Within<()>` -- so that half of the contract is enforced at compile time and needs no property. Left as a comment so the next person does not retry it. ## Verified, including what the verification cannot see The break test needed two attempts, which is the argument for always running it: making `matches` stricter in the **arity-1** arm changed nothing, because these shapes are arity 3 and 4. Patching the arity-3 arm fails in half a second with a shrunk packet. Both properties also run clean under miri, which is the only thing that can see the *unsound* direction -- a test that has already reached `unwrap_unchecked` on a `None` cannot report it. Recorded honestly at the property: bolero manages 5 cases a second under miri against ~35,000 native, and the miri recipe spawns its own `nix-shell` so the caller's `BOLERO_RANDOM_TEST_TIME_MS` never arrives, capping the run at 25 cases per property. A smoke test of the unsafe path, not a proof. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit c3ce02d8878040b2020c7dd505a8f8e6573ef7be) --- net/src/headers/mod.rs | 67 +++++++++++++++++++++++- net/src/headers/view.rs | 113 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 179 insertions(+), 1 deletion(-) diff --git a/net/src/headers/mod.rs b/net/src/headers/mod.rs index 22fb5237ca..cf409cee4d 100644 --- a/net/src/headers/mod.rs +++ b/net/src/headers/mod.rs @@ -1194,7 +1194,7 @@ where mod contract { use crate::eth::ethtype::CommonEthType; use crate::eth::{Eth, GenWithEthType}; - use crate::headers::{Headers, Net, Transport}; + use crate::headers::{Headers, MAX_NET_EXTENSIONS, MAX_VLANS, Net, NetExt, Transport}; use crate::icmp4::Icmp4; use crate::icmp6::Icmp6; use crate::ipv4; @@ -1205,6 +1205,7 @@ mod contract { use crate::vxlan::Vxlan; use arrayvec::ArrayVec; use bolero::{Driver, TypeGenerator, ValueGenerator}; + use std::ops::Bound; impl TypeGenerator for Headers { /// Generate a completely arbitrary value of [`Headers`]. @@ -1250,6 +1251,70 @@ mod contract { } } + /// Draws [`Headers`] whose **layer structure** varies: VLAN tags, IPv6 extension headers, and + /// every combination of the two. + /// + /// [`CommonHeaders`] deliberately does not. All six of its construction sites set + /// `vlan: ArrayVec::default()` and `net_ext: ArrayVec::default()`, so it never produces a VLAN tag + /// or an extension header at all -- reasonable for the "sunny-day" packet processing it was written + /// for, and useless for the code whose whole subject is those two things: + /// + /// * [`HeadersView`](crate::headers::view::HeadersView) exists to decide whether a packet's + /// structure matches a type-level shape, and its contract is stated in terms of VLAN tags that + /// are not mentioned in the shape and extension regions the shape may or may not enter; + /// * [`Matcher`](crate::headers::pat::Matcher) threads the same `ExtGapCheck`. + /// + /// Neither semantics could be reached by any existing generator, which is a better explanation of + /// `view.rs` sitting at 42% line coverage than "nobody wrote tests" -- the tests are there. + /// + /// This is structural on purpose. `ViewStep::step` walks the in-memory layers with VLAN and + /// extension cursors, so what matters here is which slots are populated and how many, not whether + /// `next_header` agrees with the layer that follows it. A generator that also kept the wire fields + /// consistent would be the right tool for a parse round-trip property, and the wrong one for this: + /// it would couple a structural test to a byte-level invariant and shrink the space it explores. + #[allow(dead_code)] // constructed through `.with_generator()` + #[repr(transparent)] + pub struct ShapedHeaders; + + impl ValueGenerator for ShapedHeaders { + type Output = Headers; + + fn generate(&self, driver: &mut D) -> Option { + // Start from a common stack so the base layers are realistic, then vary the structure the + // view and matcher semantics actually turn on. + let mut headers = CommonHeaders.generate(driver)?; + + let vlans = driver.gen_usize(Bound::Included(&0), Bound::Included(&MAX_VLANS))?; + for _ in 0..vlans { + headers.vlan.push(driver.produce()?); + } + + // Extension headers hang off the net layer, so there is nothing to attach them to without + // one. The IPv4 authentication header is the only one that belongs on a v4 packet. + let ipv4 = matches!(headers.net, Some(Net::Ipv4(_))); + if headers.net.is_some() { + let exts = + driver.gen_usize(Bound::Included(&0), Bound::Included(&MAX_NET_EXTENSIONS))?; + for _ in 0..exts { + let ext = if ipv4 { + NetExt::Ipv4Auth(driver.produce()?) + } else { + match driver.gen_u8(Bound::Included(&0), Bound::Included(&4))? { + 0 => NetExt::HopByHop(driver.produce()?), + 1 => NetExt::DestOpts(driver.produce()?), + 2 => NetExt::Routing(driver.produce()?), + 3 => NetExt::Fragment(driver.produce()?), + _ => NetExt::Ipv6Auth(driver.produce()?), + } + }; + headers.net_ext.push(ext); + } + } + + Some(headers) + } + } + #[allow(dead_code)] // rustc not able to infer we construct this through .with_generator() #[repr(transparent)] pub struct CommonHeaders; diff --git a/net/src/headers/view.rs b/net/src/headers/view.rs index 516f60ff28..abb0479b89 100644 --- a/net/src/headers/view.rs +++ b/net/src/headers/view.rs @@ -2575,3 +2575,116 @@ mod tests { assert_eq!(v.vid(), vid_updated); } } + +/// The `unsafe` boundary, checked against the safe implementation of the same semantics. +/// +/// [`HeadersView`] earns its zero-cost extraction with `unwrap_unchecked`: [`Look::look`] repeats the +/// [`ViewStep::step`] chain that [`sealed::Sealed::matches`] already ran and tells the compiler the +/// `None` arms cannot happen. **The soundness of that rests entirely on the two chains agreeing**, and +/// they are written out separately for every arity by the macro above -- eight-odd hand-written pairs, +/// each threading the VLAN and extension cursors through by hand. A transposed cursor in one of them +/// is not a wrong answer, it is undefined behaviour. +/// +/// That is the same shape as every defect this campaign found in `routing`: an invariant enforced at a +/// distance by a different function from the one relying on it. The difference is the consequence. +/// +/// The oracle is [`Matcher`](super::pat::Matcher), which decides the same question safely and returns +/// an `Option`. Comparing the two is a differential test between two existing implementations rather +/// than against a third transcription of the rules -- if they disagree, either `matches` admits a +/// packet `look` cannot extract from (undefined behaviour) or `Matcher` mis-matches in the datapath. +/// Both are worth knowing. +/// +/// References are compared by **address**, not by value: two VLAN tags with identical contents are a +/// pass under `assert_eq!` and a bug if the two implementations picked different ones. +/// +/// # What this cannot catch +/// +/// Both `matches` and `look` call the same [`ViewStep::step`], so a bug *inside* `step` is invisible +/// here -- it moves both sides together. What is checked is the hand-written *chaining* around it, +/// which is where the duplication is. +/// +/// And it can only observe the safe direction of a divergence. If `matches` were ever too **strict**, +/// the matcher accepts where `as_view` refuses and this fails with a counterexample. If it were too +/// **permissive**, `look` reaches `unwrap_unchecked` on a `None`, and a test that has already invoked +/// undefined behaviour is in no position to report it. **That direction is what miri is for**, and +/// these run clean under `just miri test -p dataplane-net view_properties`. +/// +/// Note what that costs: bolero manages 5 cases a second under miri against roughly 35,000 native, and +/// the miri recipe spawns its own `nix-shell`, so `BOLERO_RANDOM_TEST_TIME_MS` from the caller's +/// environment does not reach it and the run stops at **25 cases per property**. That is a smoke test +/// of the unsafe path, not a proof. Raising it means setting the budget inside `miri.just`. +#[cfg(test)] +mod view_properties { + use crate::eth::Eth; + use crate::headers::view::Look; + use crate::headers::{Headers, Net, ShapedHeaders, Transport}; + use crate::vlan::Vlan; + + /// `as_view` and `Matcher` must agree on whether the packet has the shape, and on which layers. + #[test] + fn eth_net_transport_agrees_with_the_matcher() { + bolero::check!() + .with_generator(ShapedHeaders) + .for_each(|h: &Headers| { + let matched = h.pat().eth().net().transport().done(); + match h.as_view::<(&Eth, &Net, &Transport)>() { + None => assert!( + matched.is_none(), + "the matcher accepted a shape as_view refused: {h:?}" + ), + Some(view) => { + let Some((m_eth, m_net, m_transport)) = matched else { + panic!("as_view accepted a shape the matcher refused: {h:?}"); + }; + let (v_eth, v_net, v_transport) = view.look(); + assert!(std::ptr::eq(v_eth, m_eth), "eth differs: {h:?}"); + assert!(std::ptr::eq(v_net, m_net), "net differs: {h:?}"); + assert!( + std::ptr::eq(v_transport, m_transport), + "transport differs: {h:?}" + ); + } + } + }); + } + + /// The same, for a shape that names a VLAN tag. + /// + /// This is where the interesting half of the contract lives: a tag the shape does not mention is a + /// miss, so the two implementations have to agree about *how many* tags were consumed, not merely + /// that some were. + #[test] + fn eth_vlan_net_transport_agrees_with_the_matcher() { + bolero::check!() + .with_generator(ShapedHeaders) + .for_each(|h: &Headers| { + let matched = h.pat().eth().vlan().net().transport().done(); + match h.as_view::<(&Eth, &Vlan, &Net, &Transport)>() { + None => assert!( + matched.is_none(), + "the matcher accepted a vlan shape as_view refused: {h:?}" + ), + Some(view) => { + let Some((m_eth, m_vlan, m_net, m_transport)) = matched else { + panic!("as_view accepted a vlan shape the matcher refused: {h:?}"); + }; + let (v_eth, v_vlan, v_net, v_transport) = view.look(); + assert!(std::ptr::eq(v_eth, m_eth), "eth differs: {h:?}"); + assert!( + std::ptr::eq(v_vlan, m_vlan), + "the two implementations consumed different vlan tags: {h:?}" + ); + assert!(std::ptr::eq(v_net, m_net), "net differs: {h:?}"); + assert!( + std::ptr::eq(v_transport, m_transport), + "transport differs: {h:?}" + ); + } + } + }); + } + + // A shape starting at `Net` rather than `Eth` was tried here and does not compile: `Net` does not + // satisfy `Within<()>`, so the adjacency graph forbids a shape that begins mid-stack. That half of + // the contract is enforced at compile time and needs no property. +} From 6edcf934135439da5e40dd1cf77dba3878100b40 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 14:55:36 -0600 Subject: [PATCH 22/30] docs(net): Correct where the HeadersView soundness check actually fires The previous commit claimed the unsound direction of a `matches`/`look` divergence could only be seen under miri, on the reasoning that a test which has already reached `unwrap_unchecked` on a `None` is in no position to report it. That is wrong, and Daniel caught it. `unwrap_unchecked` bottoms out in `hint::unreachable_unchecked`, whose `assert_unsafe_precondition!` is gated on `ub_checks`, which follows `-Cdebug-assertions` -- and `profile.fuzz` sets that **on**. Measured rather than reasoned this time: unsafe precondition(s) violated: hint::unreachable_unchecked must never be reached thread caused non-unwinding panic. aborting. ... (signal: 6, SIGABRT: process abort signal) So `profile=fuzz` already detects it, and that is where most of the assurance comes from. Miri is still worth having for what `ub_checks` does not model -- aliasing and provenance across the `as_ref_unchecked` boundary -- but it is not the only thing standing between this and undefined behaviour. Both guards are now documented in the order they fire, each demonstrated by breaking the arity-3 arm deliberately: 1. **the differential, first.** The `Matcher` comparison runs *before* `look`, so a divergence in either direction fails with a shrunk counterexample instead of invoking undefined behaviour. Over-strict and accept-everything both fail here in under a second. 2. **the standard library's check, as a backstop**, for a divergence that slipped past guard 1 -- if `Matcher` carried the same bug. Verified by calling `look` on an over-permissive `matches` with the differential removed: `SIGABRT`, fuzz profile, no miri. Worth recording that guard 2 is a *non-unwinding* panic, so bolero cannot catch it and the process dies. Under libfuzzer that is the right outcome -- a saved `crash-*` artifact rather than a silent pass -- but it does mean the failure surfaces as an abort rather than a counterexample, which is why guard 1 running first is a design choice and not an accident. No behaviour change; comments only. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 217664138af0f621a005e8aa8271270e01a37d05) --- net/src/headers/view.rs | 42 +++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/net/src/headers/view.rs b/net/src/headers/view.rs index abb0479b89..1aa999690f 100644 --- a/net/src/headers/view.rs +++ b/net/src/headers/view.rs @@ -2597,22 +2597,44 @@ mod tests { /// References are compared by **address**, not by value: two VLAN tags with identical contents are a /// pass under `assert_eq!` and a bug if the two implementations picked different ones. /// -/// # What this cannot catch +/// # Two lines of defence, and where each one fires /// /// Both `matches` and `look` call the same [`ViewStep::step`], so a bug *inside* `step` is invisible /// here -- it moves both sides together. What is checked is the hand-written *chaining* around it, /// which is where the duplication is. /// -/// And it can only observe the safe direction of a divergence. If `matches` were ever too **strict**, -/// the matcher accepts where `as_view` refuses and this fails with a counterexample. If it were too -/// **permissive**, `look` reaches `unwrap_unchecked` on a `None`, and a test that has already invoked -/// undefined behaviour is in no position to report it. **That direction is what miri is for**, and -/// these run clean under `just miri test -p dataplane-net view_properties`. +/// Against a divergence in that chaining there are two independent guards, and both were demonstrated +/// by breaking the arity-3 arm on purpose: /// -/// Note what that costs: bolero manages 5 cases a second under miri against roughly 35,000 native, and -/// the miri recipe spawns its own `nix-shell`, so `BOLERO_RANDOM_TEST_TIME_MS` from the caller's -/// environment does not reach it and the run stops at **25 cases per property**. That is a smoke test -/// of the unsafe path, not a proof. Raising it means setting the budget inside `miri.just`. +/// 1. **This differential, which fires first.** The `Matcher` comparison happens *before* `look` is +/// called, so a divergence in either direction fails with a shrunk counterexample rather than by +/// invoking undefined behaviour. Making `matches` stricter, or making it accept everything, both +/// fail here in under a second. +/// 2. **The standard library's own check, as a backstop.** `unwrap_unchecked` bottoms out in +/// `hint::unreachable_unchecked`, whose `assert_unsafe_precondition!` is gated on `ub_checks` -- +/// which follows `-Cdebug-assertions`, and `profile.fuzz` sets that **on**. So if a divergence ever +/// slipped past guard 1 -- if `Matcher` carried the same bug, say -- reaching `look` aborts: +/// +/// ```text +/// unsafe precondition(s) violated: hint::unreachable_unchecked must never be reached +/// thread caused non-unwinding panic. aborting. +/// ``` +/// +/// Verified by calling `look` on a deliberately over-permissive `matches` with the differential +/// removed: `SIGABRT`, in the fuzz profile, no miri required. Note it is a *non-unwinding* panic, so +/// bolero cannot catch it and the process dies -- which under libfuzzer is exactly right, a saved +/// `crash-*` artifact rather than a silent pass. +/// +/// The practical consequence is that **`just fuzz` on the fuzz profile already detects the unsound +/// direction**, and that is where the assurance mostly comes from. Miri remains useful for what +/// `ub_checks` does not model -- aliasing and provenance across the `as_ref_unchecked` boundary -- but +/// it is not the only thing standing between this and undefined behaviour. +/// +/// These do run clean under `just miri test -p dataplane-net view_properties`. Worth knowing what that +/// is worth, though: bolero manages 5 cases a second under miri against roughly 35,000 native, and the +/// miri recipe spawns its own `nix-shell`, so `BOLERO_RANDOM_TEST_TIME_MS` from the caller's +/// environment never arrives and the run stops at **25 cases per property**. A smoke test, not a proof. +/// Raising it means setting the budget inside `miri.just`. #[cfg(test)] mod view_properties { use crate::eth::Eth; From 83f48b513f9914b2feae368e5d74def40d635b50 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 15:25:07 -0600 Subject: [PATCH 23/30] test(net): Check the mutable half of the HeadersView boundary, and make miri usable The read path was the easy half. `Look::look` and `sealed::Sealed::matches` at least walk the stack the same way -- both chain `ViewStep::step`. `look_mut` does not: it builds a `MatcherMut` from `pat_mut()` and chains `ViewStepMut::chain` over it, then calls `unreachable_unchecked` if that returns `None`. **So the invariant is established by one traversal and consumed by a different one.** Nothing makes `ViewStep::step` and `ViewStepMut::chain` agree except that they were written to. Where the read path risks a mis-threaded cursor between two copies of one walk, this risks two walks disagreeing outright. Worth saying what is *not* tested: `look_mut` against `MatcherMut`. `look_mut` **is** `MatcherMut` plus an `unreachable_unchecked`, so comparing them is the implementation against itself. The question worth asking is whether `matches` -- which licensed the unchecked call -- agrees with the walk that has to deliver on it, and that is checked without calling `look_mut` at all, so a divergence is a counterexample rather than undefined behaviour. `look_mut` hands back several `&mut` into one `Headers`, pre-split through `Fields`. If that split ever aliased, two references would point at the same layer -- and the `ub_checks` backstop cannot see it. It checks the `unreachable_unchecked` precondition and nothing about aliasing. Only miri sees that, and only with stacked borrows on. Two obstacles, both now fixed in `miri.just`: - **the budget.** The recipe launches its own `nix-shell`, which does not inherit the caller's environment, so `BOLERO_RANDOM_TEST_TIME_MS` never arrived and every property stopped at bolero's one-second default -- about 25 cases under miri. Enough to prove the harness runs and nothing else. - **`stacked_borrow_check` was unreachable.** `just` will not override a *module's* variables from the command line: `just miri stacked_borrow_check=... test` parses as a recipe name, and `--set` is refused as "not present in justfile". The knob existed and could only be changed by editing the file. Both now read `env()`, so `STACKED_BORROW_CHECK=enabled just miri test ...` works. Under miri bolero manages about five cases a second, and the cost is wall-clock. Raising the budget buys cases linearly on one core; sharding buys them across cores for free. The aliasing property -- the expensive one and the one that matters most -- is instantiated as sixteen shards, each seeded from the OS so they explore independent streams, and nextest runs them concurrently. Sixteen rather than sixty because miri's per-process memory footprint is large and the other properties want cores too. Result: **963 cases across 24 properties under miri with stacked borrows enabled, no undefined behaviour** -- against 25 per property with stacked borrows off before this. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 0b8372bca9c6fbe51deb88994b3e8ba13af3a33e) --- miri.just | 16 +++- net/src/headers/view.rs | 170 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 1 deletion(-) diff --git a/miri.just b/miri.just index 091da10355..db51d93875 100644 --- a/miri.just +++ b/miri.just @@ -25,7 +25,20 @@ export target := cpu + "-unknown-linux-gnu" export provenance := "permissive" export schedule_seed := choose('5', "0123456789") export seeds := "1" -export stacked_borrow_check := "disabled" +# Overridable from the environment because `just` will not override a *module's* variables from the +# command line: `just miri stacked_borrow_check=enabled test` is parsed as a recipe name, and +# `just --set stacked_borrow_check enabled miri test` is refused outright ("not present in justfile"). +# Without `env()` these knobs can only be changed by editing this file, which is how +# `stacked_borrow_check` came to be effectively unreachable. +export stacked_borrow_check := env("STACKED_BORROW_CHECK", "disabled") + +# How long each bolero property gets, in milliseconds. +# +# Needed because this recipe launches its own `nix-shell`, which does not inherit the caller's +# environment: exporting `BOLERO_RANDOM_TEST_TIME_MS` before `just miri test` has no effect, and every +# property silently stops at bolero's one-second default. Under miri that is about 25 cases -- enough to +# prove the harness runs and nothing else. +export bolero_test_time_ms := env("BOLERO_TEST_TIME_MS", "30000") export preemption_rate := "0.10" export weak_failure_rate := "0.05" export randomize_struct_layout := "enabled" @@ -63,6 +76,7 @@ test *args="": # Umbrella cfg shared with the qemu-user path in nix/profiles.nix. RUSTFLAGS+="--cfg=emulated" declare -rx RUSTFLAGS + declare -rx BOLERO_RANDOM_TEST_TIME_MS="${bolero_test_time_ms}" declare -a cmd=("nice" "-n" "19" "cargo" "miri" "nextest" "run" "--profile=miri" "--target=${target}") if [ "${cores}" != "0" ]; then # nextest defaults --test-threads to the core count; the miri profile diff --git a/net/src/headers/view.rs b/net/src/headers/view.rs index 1aa999690f..2d3173367b 100644 --- a/net/src/headers/view.rs +++ b/net/src/headers/view.rs @@ -2710,3 +2710,173 @@ mod view_properties { // satisfy `Within<()>`, so the adjacency graph forbids a shape that begins mid-stack. That half of // the contract is enforced at compile time and needs no property. } + +/// The **mutable** half of the unsafe boundary, which is the more delicate one. +/// +/// [`Look::look`] and [`sealed::Sealed::matches`] at least walk the stack the same way: both chain +/// [`ViewStep::step`]. [`LookMut::look_mut`] does not. It builds a [`MatcherMut`](super::pat::MatcherMut) +/// from [`Headers::pat_mut`] and chains [`ViewStepMut::chain`] over it, then calls +/// `unreachable_unchecked` if that comes back `None`. +/// +/// So the invariant is established by one traversal and consumed by a **different** one. Nothing makes +/// `ViewStep::step` and `ViewStepMut::chain` agree except that they were written to; where the read path +/// risks a mis-threaded cursor between two copies of the same walk, this risks two walks disagreeing +/// outright. +/// +/// Note what is *not* worth testing here: `look_mut` against `MatcherMut` directly. `look_mut` **is** +/// `MatcherMut` plus an `unreachable_unchecked`, so that comparison is the implementation against +/// itself. The question worth asking is whether `matches` -- the `ViewStep` walk that licensed the +/// unchecked call -- agrees with the `ViewStepMut` walk that has to deliver on it. +/// +/// # Aliasing, and why `ub_checks` cannot help +/// +/// `look_mut` hands back several `&mut` into one [`Headers`], pre-split through +/// [`Fields`](super::pat::Fields). If that split ever aliased, two of those references would point at +/// the same layer -- undefined behaviour of a kind the `ub_checks` backstop does **not** model. It +/// checks the `unreachable_unchecked` precondition, nothing about aliasing. +/// +/// Only miri sees that, and only with stacked borrows *on*, which this repo turns off by default: +/// +/// ```text +/// STACKED_BORROW_CHECK=enabled just miri test -p dataplane-net view_mut_properties +/// ``` +/// +/// Through the environment, not `just`'s command line: a *module's* variables cannot be overridden +/// there. `just miri stacked_borrow_check=enabled test` parses as a recipe name and +/// `just --set stacked_borrow_check enabled miri test` is refused, which is why `miri.just` reads both +/// knobs via `env()`. +#[cfg(test)] +mod view_mut_properties { + use crate::eth::Eth; + use crate::headers::view::LookMut; + use crate::headers::{Headers, Net, ShapedHeaders, Transport}; + use crate::vlan::Vlan; + + /// Whatever licensed the unchecked call must be deliverable by the walk that has to deliver it. + /// + /// Checked without calling `look_mut`, so a divergence is a clean counterexample rather than + /// undefined behaviour: `as_view_mut` consults `matches`, and the chain below is the same one + /// `look_mut` would run, but safely. + #[test] + fn what_matches_licenses_the_mutable_walk_can_deliver() { + bolero::check!() + .with_generator(ShapedHeaders) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + let licensed = owned.as_view_mut::<(&Eth, &Net, &Transport)>().is_some(); + let deliverable = owned.pat_mut().eth().net().transport().done().is_some(); + assert_eq!( + licensed, deliverable, + "`matches` and the mutable walk disagree, so `look_mut` would reach \ + `unreachable_unchecked`: {h:?}" + ); + }); + } + + /// The same for a shape naming a VLAN tag, where the walks have to agree on how many were consumed. + #[test] + fn what_matches_licenses_the_mutable_vlan_walk_can_deliver() { + bolero::check!() + .with_generator(ShapedHeaders) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + let licensed = owned + .as_view_mut::<(&Eth, &Vlan, &Net, &Transport)>() + .is_some(); + let deliverable = owned + .pat_mut() + .eth() + .vlan() + .net() + .transport() + .done() + .is_some(); + assert_eq!( + licensed, deliverable, + "`matches` and the mutable vlan walk disagree: {h:?}" + ); + }); + } + + /// Exercise the multi-`&mut` split itself: write through every reference and read the writes back. + /// + /// The assertions are almost beside the point. What matters is that the references are *created and + /// written through*, so that miri with stacked borrows enabled can judge whether + /// [`Fields`](super::pat::Fields) handed out two paths to the same layer. Nothing else in the suite + /// does that. + fn exercise_the_mutable_split() { + bolero::check!() + .with_generator(ShapedHeaders) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + let Some(view) = owned.as_view_mut::<(&Eth, &Net, &Transport)>() else { + return; + }; + let (eth, net, transport) = view.look_mut(); + + // A write through each reference, then a read back through the same one. If two of + // these aliased, the writes would interfere and miri would object to the borrow stack + // long before the values did. + let want_src = + crate::eth::mac::SourceMac::try_from(crate::eth::mac::Mac([2, 0, 0, 0, 0, 1])) + .unwrap_or_else(|_| { + unreachable!("a locally-administered unicast mac is a valid source") + }); + eth.set_source(want_src); + let seen_net = net.dst_addr(); + let seen_transport = transport.dst_port(); + + assert_eq!( + eth.source(), + want_src, + "the write through eth did not stick" + ); + assert_eq!(net.dst_addr(), seen_net, "net changed under a write to eth"); + assert_eq!( + transport.dst_port(), + seen_transport, + "transport changed under a write to eth" + ); + }); + } + + /// Shards of [`exercise_the_mutable_split`], so the machine can be used. + /// + /// Under miri this property is the expensive one and the one that matters most, and its cost is + /// wall-clock: bolero manages about five cases a second. Raising the time budget buys cases + /// linearly, but only on one core. + /// + /// Each shard seeds itself from the OS, so `N` shards explore `N` independent streams and nextest + /// runs them concurrently -- turning a machine with cores to spare into more cases for the same + /// wall time, which is the only lever that does not cost patience. Sixteen rather than sixty: miri + /// carries a large memory footprint per process, and the other properties want cores too. + macro_rules! split_shards { + ($($name:ident),* $(,)?) => { + $( + #[test] + fn $name() { + exercise_the_mutable_split(); + } + )* + }; + } + + split_shards!( + the_mutable_split_hands_out_distinct_layers, + split_shard_02, + split_shard_03, + split_shard_04, + split_shard_05, + split_shard_06, + split_shard_07, + split_shard_08, + split_shard_09, + split_shard_10, + split_shard_11, + split_shard_12, + split_shard_13, + split_shard_14, + split_shard_15, + split_shard_16, + ); +} From 1ef005fdf440317af1732b696bb5936330dd80ab Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 16:19:21 -0600 Subject: [PATCH 24/30] test(net): Extend the HeadersView differential to every arity the oracle reaches `matches`, `look` and `look_mut` are generated separately for each arity by a macro, with the VLAN and extension cursors threaded through by hand every time. Testing two arities tested two of eight copies, which is why the last commit moved `view.rs` coverage by two points and no more. Both walks are now checked at arities one through seven, read and mutable, from one macro so a new arity costs one line. ## Seven, not eight, and the reason is the oracle `Matcher`'s vocabulary is `eth`, `vlan`, `net`, `transport`, `vxlan`, `embedded`. The last two cannot be reached in a builder chain: `Vxlan: Within` and the embedded header sits under `Icmp4`/`Icmp6` -- both *concrete* layers -- and `Matcher` has no concrete-layer methods. No `.udp()`, no `.tcp()`. So its longest expressible chain is `Eth`, four VLAN tags (`MAX_VLANS`), `Net`, `Transport`. Two gaps follow, and both belong to the oracle rather than the code: - the **arity-8** arm is generated and stays unchecked, because nothing `Matcher` can say is eight elements long; - shapes entering the **IPv6 extension region** cannot be expressed at all, and that is the more interesting loss. `ExtGapCheck` is the subtlest part of the contract and the part the module documentation spends most of its words on, and it has no oracle. Closing it needs extension-header methods on `Matcher`, or a different oracle. Recorded at the call site so the next person does not have to rediscover why the list stops where it does. ## Every arity proves it is not vacuous A shape the generator never produces makes its property pass for the wrong reason, and the higher arities are exactly where that would happen quietly: arity 7 needs a packet carrying *exactly* four VLAN tags, since a tag the shape does not name is a miss. So each property reports its hit rate and fails if it never matched. Measured, and pleasingly uniform: arity 1 matches everything, and arities 2 through 7 each match about 20% -- which is `P(exactly N tags)` for a uniform 0..=4 draw. Every arity is exercised at roughly the same rate rather than the long shapes being starved. ## Verification 1,386 tests green. Under miri with stacked borrows enabled -- the configuration that can see an aliasing fault in the `Fields` split, which the `ub_checks` backstop cannot -- **4,560 cases across 31 properties, no undefined behaviour.** That is up from 963 before the arity work and from 25 per property before the miri budget was reachable at all. Coverage-guided runs on the two differentials, 60 workers: roughly 1.7 billion executions each, no crashes. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit edf8bb34b26eeee9e7b05493c65f36cb152882b8) --- net/src/headers/view.rs | 203 ++++++++++++++++++++++++++++++++-------- 1 file changed, 163 insertions(+), 40 deletions(-) diff --git a/net/src/headers/view.rs b/net/src/headers/view.rs index 2d3173367b..ee480085ca 100644 --- a/net/src/headers/view.rs +++ b/net/src/headers/view.rs @@ -2752,52 +2752,175 @@ mod view_mut_properties { use crate::headers::{Headers, Net, ShapedHeaders, Transport}; use crate::vlan::Vlan; - /// Whatever licensed the unchecked call must be deliverable by the walk that has to deliver it. + /// Both walks, at every arity the oracle can express. /// - /// Checked without calling `look_mut`, so a divergence is a clean counterexample rather than - /// undefined behaviour: `as_view_mut` consults `matches`, and the chain below is the same one - /// `look_mut` would run, but safely. - #[test] - fn what_matches_licenses_the_mutable_walk_can_deliver() { - bolero::check!() - .with_generator(ShapedHeaders) - .for_each(|h: &Headers| { - let mut owned = h.clone(); - let licensed = owned.as_view_mut::<(&Eth, &Net, &Transport)>().is_some(); - let deliverable = owned.pat_mut().eth().net().transport().done().is_some(); - assert_eq!( - licensed, deliverable, - "`matches` and the mutable walk disagree, so `look_mut` would reach \ - `unreachable_unchecked`: {h:?}" + /// The decision is what soundness turns on: if `matches` says yes where the walk that must deliver + /// says no, `look`/`look_mut` reach `unreachable_unchecked`. So this compares decisions across the + /// whole family, for the read path and the mutable path both. + /// + /// Arity matters because `matches`, `look` and `look_mut` are generated separately for each one, by + /// a macro, with the VLAN and extension cursors threaded through by hand every time. Testing two + /// arities tested two of eight copies. + /// + /// Pointer identity -- did the two implementations pick the *same* layer, not merely agree that one + /// exists -- is checked separately, at arities 3 and 4, where the tuple can be destructured + /// concretely. That is a correctness question rather than a soundness one, so it is checked deeply + /// at representative arities rather than shallowly at all of them. + macro_rules! arity_agrees { + ($read:ident, $mutable:ident, $shape:ty, $($layer:ident),+) => { + #[test] + fn $read() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static SEEN: AtomicUsize = AtomicUsize::new(0); + static HIT: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ShapedHeaders) + .for_each(|h: &Headers| { + SEEN.fetch_add(1, Ordering::Relaxed); + let licensed = h.as_view::<$shape>().is_some(); + if licensed { + HIT.fetch_add(1, Ordering::Relaxed); + } + let deliverable = h.pat()$(.$layer())+.done().is_some(); + assert_eq!( + licensed, deliverable, + concat!( + "`matches` and the read walk disagree for ", + stringify!($shape), + ", so `look` would reach `unreachable_unchecked`: {:?}" + ), + h + ); + }); + agreement_is_not_vacuous( + stringify!($shape), + SEEN.load(Ordering::Relaxed), + HIT.load(Ordering::Relaxed), ); - }); - } + } - /// The same for a shape naming a VLAN tag, where the walks have to agree on how many were consumed. - #[test] - fn what_matches_licenses_the_mutable_vlan_walk_can_deliver() { - bolero::check!() - .with_generator(ShapedHeaders) - .for_each(|h: &Headers| { - let mut owned = h.clone(); - let licensed = owned - .as_view_mut::<(&Eth, &Vlan, &Net, &Transport)>() - .is_some(); - let deliverable = owned - .pat_mut() - .eth() - .vlan() - .net() - .transport() - .done() - .is_some(); - assert_eq!( - licensed, deliverable, - "`matches` and the mutable vlan walk disagree: {h:?}" + #[test] + fn $mutable() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static SEEN: AtomicUsize = AtomicUsize::new(0); + static HIT: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ShapedHeaders) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + SEEN.fetch_add(1, Ordering::Relaxed); + let licensed = owned.as_view_mut::<$shape>().is_some(); + if licensed { + HIT.fetch_add(1, Ordering::Relaxed); + } + let deliverable = owned.pat_mut()$(.$layer())+.done().is_some(); + assert_eq!( + licensed, deliverable, + concat!( + "`matches` and the mutable walk disagree for ", + stringify!($shape), + ", so `look_mut` would reach `unreachable_unchecked`: {:?}" + ), + h + ); + }); + agreement_is_not_vacuous( + stringify!($shape), + SEEN.load(Ordering::Relaxed), + HIT.load(Ordering::Relaxed), ); - }); + } + }; + } + + /// A shape the generator never produces makes its property pass for the wrong reason. + /// + /// Two implementations agreeing that nothing matches is not agreement worth having, and the higher + /// arities are where it would happen quietly: arity 7 needs a packet carrying exactly four VLAN + /// tags, since a tag the shape does not name is a miss. So every arity reports its own hit rate and + /// fails if it never matched at all. + fn agreement_is_not_vacuous(shape: &str, seen: usize, hit: usize) { + println!("{shape}: matched {hit} of {seen}"); + // The default run is a second long, which is plenty here -- these properties manage tens of + // thousands of cases a second -- but a short sample should say so rather than fail. + if seen > 500 { + assert!( + hit > 0, + "{shape} never matched in {seen} packets: the two walks agree only because the \ + generator cannot produce this shape" + ); + } } + // Every arity from one to seven, which is as far as the oracle reaches. + // + // `Matcher`'s vocabulary is `eth`, `vlan`, `net`, `transport`, `vxlan` and `embedded`. The last two + // are unreachable in a builder chain: `Vxlan: Within` and the embedded header sits under + // `Icmp4`/`Icmp6`, both *concrete* layers, and `Matcher` has no concrete-layer methods -- no + // `.udp()`, no `.tcp()`. So the longest chain it can express is `Eth`, four VLAN tags + // (`MAX_VLANS`), `Net`, `Transport`: seven. + // + // Two gaps follow, and both are about the oracle rather than the code: + // + // * the **arity-8** arm of the macro is generated and goes unchecked here, because nothing the + // oracle can say is eight elements long; + // * shapes entering the **IPv6 extension region** cannot be expressed at all, and that is the + // more interesting loss -- `ExtGapCheck` is the subtlest part of the contract, the part the + // module documentation spends most of its words on, and it has no oracle. Closing it means + // either extension-header methods on `Matcher` or a different oracle. + arity_agrees!(read_1, mutable_1, (&Eth,), eth); + arity_agrees!(read_2, mutable_2, (&Eth, &Net), eth, net); + arity_agrees!( + read_3, + mutable_3, + (&Eth, &Net, &Transport), + eth, + net, + transport + ); + arity_agrees!( + read_4, + mutable_4, + (&Eth, &Vlan, &Net, &Transport), + eth, + vlan, + net, + transport + ); + arity_agrees!( + read_5, + mutable_5, + (&Eth, &Vlan, &Vlan, &Net, &Transport), + eth, + vlan, + vlan, + net, + transport + ); + arity_agrees!( + read_6, + mutable_6, + (&Eth, &Vlan, &Vlan, &Vlan, &Net, &Transport), + eth, + vlan, + vlan, + vlan, + net, + transport + ); + arity_agrees!( + read_7, + mutable_7, + (&Eth, &Vlan, &Vlan, &Vlan, &Vlan, &Net, &Transport), + eth, + vlan, + vlan, + vlan, + vlan, + net, + transport + ); + /// Exercise the multi-`&mut` split itself: write through every reference and read the writes back. /// /// The assertions are almost beside the point. What matters is that the references are *created and From bcaf281281738e42910070ba5ecf867706a097ee Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 17:40:07 -0600 Subject: [PATCH 25/30] test(net): Reach the quoted packet inside an ICMP error, and the extension region `embedded_view.rs` sat at 33.9% line coverage, and the reason was not that nobody had written tests for it. Its hand-written tests are careful and thorough. Every one of them builds its packet through `HeaderStack` or `header_chain`, which pin the shape at `(Eth, Ipv4, Icmp4)` outside and `(Ipv4, Tcp)` inside and offer no way to attach an extension header to either side. Six of the eight `as_embedded` arities and the whole embedded extension region were unreachable by construction. The generators were the binding constraint, again. All six construction sites of `CommonHeaders` set `embedded_ip: None`, so neither it nor `ShapedHeaders`, which builds on it, can produce an ICMP error carrying a quoted packet at all. `ShapedIcmpError` does: it varies the outer VLAN tags and extension headers so the outer arity spans the range of `as_embedded` impls, varies whether the quote is present, and varies the quoted packet's network layer, extensions and truncated transport. The quoted family usually follows the quoting family, because that is what a real ICMP error looks like, but not always -- a mismatch is where two independent structural walks are most likely to disagree. What the new differential properties compare is the pairing soundness rests on, which is not the obvious one. `as_embedded_mut` *decides* with `Sealed::matches`, walking `EmbeddedHeaders` through `EmbeddedStep`. `look_mut` then *delivers* through `EmbeddedMatcherMut`, a separate implementation with its own pre-split fields and its own gap check, and unwraps that chain with `unreachable_unchecked` on the strength of the first one's answer. If they disagree the result is undefined behaviour, not a wrong answer. Every arity of both is now checked against the `pat()` oracle, read path and mutable path. Two claims in `view.rs` were wrong and are corrected here. `Matcher` does have concrete-layer and extension-header methods -- `matcher_net!`, `matcher_ext!` and `matcher_transport!` give it `.ipv4()`, `.hop_by_hop()`, `.tcp()` and the rest -- so neither gap that comment recorded was real. The arity-8 arm is now checked, and so is the extension region, where `ExtGapCheck` switches from skipping extensions silently to requiring all of them consumed. That is the subtlest part of the contract and the part the module documentation spends most of its words on, and it had no oracle at all. Hit rates are measured and asserted rather than hoped for, which mattered twice. A shape naming three extensions in sequence matched 2 packets in 36,000 -- honest, and useless -- until `ext_run` learned to follow RFC 8200's recommended order a quarter of the time; it now matches about 200. And naming one extension inside a quoted packet compounds six conditions, including `P(no VLAN tags) = 1/5`, which put `(&Ipv6, &DestOpts, &TruncatedTcp)` at 6 hits in 23,910. `ShapedQuote` produces that shape every time and fuzzes the contents instead: 32,000 hits. The division is deliberate -- a property comparing hit against miss needs both, a property asking only which layer was selected gets nothing from a packet it skips. Verified by breaking the code three ways. An off-by-one on the embedded extension cursor fails exactly the four extension-region differentials, in one second, with a counterexample carrying exactly one extension -- the case where a stalled cursor makes the gap check see `len 1 != ec 0`. Dropping the shape check from arity 7's `as_embedded` fails exactly `read_outer_7`, in 87ms, while `mutable_outer_7` correctly stays green. Reading one extension slot too far fails the six `same_layers_ext_*` properties through the vacuity guard, which reports that the shape was never produced -- the same class of defect this campaign has now found five times. Coverage: `embedded_view.rs` 33.9% -> 86.0%, `pat.rs` 55.4% -> 63.2% without a test written for it, `view.rs` 50.1% -> 53.8%, `net/` 66.8% -> 69.8%. The 49 lines still uncovered are all unreachable: three `unreachable_unchecked` arms, which is the point of them, and 44 in the arity-1 and arity-2 `as_embedded` arms, which cannot be instantiated -- asking for either is a compile error, so they could be deleted. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit fb394a8bcb5db413ed8b368fb74c444caf0bf5b7) --- net/src/headers/embedded_view.rs | 646 +++++++++++++++++++++++++++++++ net/src/headers/mod.rs | 277 ++++++++++++- net/src/headers/view.rs | 108 +++++- 3 files changed, 997 insertions(+), 34 deletions(-) diff --git a/net/src/headers/embedded_view.rs b/net/src/headers/embedded_view.rs index fe6d5eba0e..ad52e28bf7 100644 --- a/net/src/headers/embedded_view.rs +++ b/net/src/headers/embedded_view.rs @@ -1790,3 +1790,649 @@ mod tests { } } } + +// =========================================================================== +// Differential properties +// =========================================================================== + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] // fine to unwrap in tests +mod embedded_view_properties { + use super::*; + use crate::eth::Eth; + use crate::headers::{Headers, ShapedIcmpError, ShapedQuote}; + use crate::icmp4::Icmp4; + use crate::icmp6::Icmp6; + use crate::vlan::Vlan; + + /// Both structural walks over a quoted packet, across the range of outer and inner shapes. + /// + /// The pairing this checks is the one soundness rests on, and it is not the obvious one. + /// `as_embedded`/`as_embedded_mut` *decide* with [`embedded_sealed::Sealed::matches`], which + /// walks [`EmbeddedHeaders`] through [`EmbeddedStep`]. [`EmbeddedLookMut::look_mut`] then + /// *delivers* through [`EmbeddedMatcherMut`], a completely separate implementation with its own + /// pre-split [`EmbeddedFields`](super::super::pat::EmbeddedFields) and its own gap check, and it + /// unwraps that chain with `unreachable_unchecked` on the strength of the first one's answer. + /// Two implementations, one of them licensing the other to skip its own `None` branch. If they + /// ever disagree the result is undefined behaviour, not a wrong answer. + /// + /// So the oracle here is `pat()`/`pat_mut()`'s `.embedded()` chain, which is the same machinery + /// `look_mut` uses -- making this differential test exactly the invariant `look_mut` assumes. + /// + /// Arity matters twice over, because `matches`, `look` and `look_mut` are generated per *inner* + /// arity while `as_embedded`/`as_embedded_mut` are generated per *outer* arity, each threading + /// the extension cursor by hand. Before this the suite reached one outer arity and one inner + /// arity, of eight and three. + macro_rules! embedded_agrees { + ( + $read:ident, $mutable:ident, + ($outer:ty, $($ol:ident),+), + ($inner:ty, $($il:ident),+) + ) => { + #[test] + fn $read() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static SEEN: AtomicUsize = AtomicUsize::new(0); + static HIT: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ShapedIcmpError) + .for_each(|h: &Headers| { + SEEN.fetch_add(1, Ordering::Relaxed); + let licensed = h + .as_view::<$outer>() + .is_some_and(|w| w.as_embedded::<$inner>().is_some()); + if licensed { + HIT.fetch_add(1, Ordering::Relaxed); + } + let deliverable = + h.pat()$(.$ol())+.embedded()$(.$il())+.done().is_some(); + assert_eq!( + licensed, deliverable, + concat!( + "`matches` and the read walk disagree for ", + stringify!($inner), + " inside ", + stringify!($outer), + ", so `look` would reach `unreachable_unchecked`: {:?}" + ), + h + ); + }); + agreement_is_not_vacuous( + concat!(stringify!($inner), " in ", stringify!($outer)), + SEEN.load(Ordering::Relaxed), + HIT.load(Ordering::Relaxed), + ); + } + + #[test] + fn $mutable() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static SEEN: AtomicUsize = AtomicUsize::new(0); + static HIT: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ShapedIcmpError) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + SEEN.fetch_add(1, Ordering::Relaxed); + // `as_embedded_mut`'s reference cannot outlive the `and_then` closure it + // would be produced in, so this asks the question without keeping it. + let licensed = match owned.as_view_mut::<$outer>() { + Some(w) => w.as_embedded_mut::<$inner>().is_some(), + None => false, + }; + if licensed { + HIT.fetch_add(1, Ordering::Relaxed); + } + let deliverable = + owned.pat_mut()$(.$ol())+.embedded()$(.$il())+.done().is_some(); + assert_eq!( + licensed, deliverable, + concat!( + "`matches` and the mutable walk disagree for ", + stringify!($inner), + " inside ", + stringify!($outer), + ", so `look_mut` would reach `unreachable_unchecked`: {:?}" + ), + h + ); + }); + agreement_is_not_vacuous( + concat!(stringify!($inner), " in ", stringify!($outer)), + SEEN.load(Ordering::Relaxed), + HIT.load(Ordering::Relaxed), + ); + } + }; + } + + /// A shape the generator never produces makes its property pass for the wrong reason. + /// + /// Two shapes compound here -- an outer arity and an inner one -- so a property can starve much + /// more quietly than in [`view`](super::super::view): demanding four VLAN tags *and* a + /// particular extension inside the quoted packet multiplies two small probabilities. Each pair + /// reports its own hit rate, and the pairs are chosen to vary one dimension at a time for + /// exactly that reason. + fn agreement_is_not_vacuous(shape: &str, seen: usize, hit: usize) { + println!("{shape}: matched {hit} of {seen}"); + // Twenty thousand rather than [`view`](super::super::view)'s five hundred, because + // compounding costs an order of magnitude: the widest pair here matches about one packet in + // twenty, and `(&Ipv6, &HopByHop, &TruncatedTcp)` inside an ICMPv6 error matches about one + // in a thousand. Five hundred cases at that rate would fail this check outright half the + // time it ran. A default one-second run manages twenty-five thousand, so the guard still + // bites in CI; a deliberately short run should say nothing rather than say something false. + if seen > 20_000 { + assert!( + hit > 0, + "{shape} never matched in {seen} packets: the two walks agree only because the \ + generator cannot produce this shape" + ); + } + } + + type O4V4 = (&'static Eth, &'static Ipv4, &'static Icmp4); + type O4V6 = (&'static Eth, &'static Ipv6, &'static Icmp6); + + // ---- Inner shapes, at the one outer arity the builders could already express ---------- + // + // Varying the inner shape alone: network layer, both enum forms, every truncated transport, + // and the extension region. + embedded_agrees!( + read_inner_v4_tcp, + mutable_inner_v4_tcp, + (O4V4, eth, ipv4, icmp4), + ((&Ipv4, &TruncatedTcp), ipv4, tcp) + ); + embedded_agrees!( + read_inner_v6_udp, + mutable_inner_v6_udp, + (O4V6, eth, ipv6, icmp6), + ((&Ipv6, &TruncatedUdp), ipv6, udp) + ); + embedded_agrees!( + read_inner_net_only, + mutable_inner_net_only, + (O4V4, eth, ipv4, icmp4), + ((&Net,), net) + ); + embedded_agrees!( + read_inner_v6_only, + mutable_inner_v6_only, + (O4V6, eth, ipv6, icmp6), + ((&Ipv6,), ipv6) + ); + // An ICMP error quoting an ICMP packet -- a `TruncatedIcmp4` inside an `Icmp4`. Legitimate: + // an unreachable in response to a ping quotes the echo request. + embedded_agrees!( + read_inner_icmp_in_icmp4, + mutable_inner_icmp_in_icmp4, + (O4V4, eth, ipv4, icmp4), + ((&Ipv4, &TruncatedIcmp4), ipv4, icmp4) + ); + embedded_agrees!( + read_inner_icmp_in_icmp6, + mutable_inner_icmp_in_icmp6, + (O4V6, eth, ipv6, icmp6), + ((&Ipv6, &TruncatedIcmp6), ipv6, icmp6) + ); + + // ---- The embedded extension region --------------------------------------------------- + // + // `ext_gap_ok_embedded` versus `ext_gap_ok_mut_embedded`: naming an extension switches the + // quoted packet from skip-extensions-silently to consume-them-all, so these match only a quote + // carrying that extension and no other. The read pair shares `ext_gap_ok_embedded` between the + // two walks and so tests the by-hand `ec` threading; the mutable pair runs two genuinely + // different implementations against each other, one over `EmbeddedHeaders` and one over a + // pre-split `EmbeddedFields`. + embedded_agrees!( + read_inner_ext_v6, + mutable_inner_ext_v6, + (O4V6, eth, ipv6, icmp6), + ((&Ipv6, &HopByHop, &TruncatedTcp), ipv6, hop_by_hop, tcp) + ); + embedded_agrees!( + read_inner_ext_v4_auth, + mutable_inner_ext_v4_auth, + (O4V4, eth, ipv4, icmp4), + ((&Ipv4, &Ipv4Auth, &TruncatedTcp), ipv4, ipv4_auth, tcp) + ); + // Entering the region and stopping there: the gap check runs at the transport step, so with no + // transport named the remaining extensions are simply left unvisited. Telling this apart from + // the two above is the whole reason the check sits where it does. + embedded_agrees!( + read_inner_ext_no_transport, + mutable_inner_ext_no_transport, + (O4V6, eth, ipv6, icmp6), + ((&Ipv6, &HopByHop), ipv6, hop_by_hop) + ); + + // ---- Outer arities ------------------------------------------------------------------- + // + // `as_embedded` and `as_embedded_mut` are written out once per outer arity, eight times. Arities + // 1 and 2 cannot be instantiated: both require `EmbeddedHeaders: Within`, which holds + // only for `Icmp4` and `Icmp6`, and neither can appear that early. `Eth` is the only layer with + // `Within<()>`, so an arity-1 shape must be `(&Eth,)`; an arity-2 shape must then end in + // something `Within`, and no ICMP layer is. Asking for either is a compile error -- + // `Icmp4: Within<()> is not satisfied` and `Icmp4: Within is not satisfied` -- so those + // two macro arms are 44 lines that can never run and could be deleted. + // + // 3 through 8 is therefore all of them. The inner shape is held cheap on purpose here, so that a + // thin outer shape is the only improbable thing each property asks for. + embedded_agrees!( + read_outer_4, + mutable_outer_4, + ((&Eth, &Vlan, &Ipv4, &Icmp4), eth, vlan, ipv4, icmp4), + ((&Ipv4, &TruncatedTcp), ipv4, tcp) + ); + embedded_agrees!( + read_outer_5, + mutable_outer_5, + ( + (&Eth, &Vlan, &Vlan, &Ipv6, &Icmp6), + eth, + vlan, + vlan, + ipv6, + icmp6 + ), + ((&Ipv6, &TruncatedUdp), ipv6, udp) + ); + embedded_agrees!( + read_outer_6, + mutable_outer_6, + ( + (&Eth, &Vlan, &Vlan, &Vlan, &Ipv4, &Icmp4), + eth, + vlan, + vlan, + vlan, + ipv4, + icmp4 + ), + // An inner shape that can miss on a quote that *is* present, so the shape check's own + // rejection is reached rather than only the absent-quote early return above it. + ((&Ipv4, &TruncatedTcp), ipv4, tcp) + ); + embedded_agrees!( + read_outer_7, + mutable_outer_7, + ( + (&Eth, &Vlan, &Vlan, &Vlan, &Vlan, &Ipv4, &Icmp4), + eth, + vlan, + vlan, + vlan, + vlan, + ipv4, + icmp4 + ), + ((&Ipv4, &TruncatedTcp), ipv4, tcp) + ); + // Arity 8, and the only outer shape that also enters the *outer* extension region: `Icmp6` + // directly after a `HopByHop` makes the outer gap check strict too. + embedded_agrees!( + read_outer_8, + mutable_outer_8, + ( + (&Eth, &Vlan, &Vlan, &Vlan, &Vlan, &Ipv6, &HopByHop, &Icmp6), + eth, + vlan, + vlan, + vlan, + vlan, + ipv6, + hop_by_hop, + icmp6 + ), + // `Ipv6` rather than `Net`: a quote of the other family misses, which is the only way to + // reach this arity's shape-check rejection. + ((&Ipv6,), ipv6) + ); + + // The `EmbeddedTransport` enum has no read-side property above, and that is an asymmetry in + // `pat.rs` rather than a choice here: `EmbeddedMatcherMut::transport` exists but + // `EmbeddedMatcher::transport` does not, so the immutable oracle cannot name the enum even + // though every concrete variant and every other layer is paired between the two. The mutable + // side is checked below; adding the missing method would let the read side follow in one line. + #[test] + fn mutable_inner_transport_enum() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static SEEN: AtomicUsize = AtomicUsize::new(0); + static HIT: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ShapedIcmpError) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + SEEN.fetch_add(1, Ordering::Relaxed); + let licensed = match owned.as_view_mut::() { + Some(w) => w.as_embedded_mut::<(&Net, &EmbeddedTransport)>().is_some(), + None => false, + }; + if licensed { + HIT.fetch_add(1, Ordering::Relaxed); + } + let deliverable = owned + .pat_mut() + .eth() + .ipv4() + .icmp4() + .embedded() + .net() + .transport() + .done() + .is_some(); + assert_eq!( + licensed, deliverable, + "`matches` and the mutable walk disagree for (&Net, &EmbeddedTransport): {h:?}" + ); + }); + agreement_is_not_vacuous( + "(&Net, &EmbeddedTransport)", + SEEN.load(Ordering::Relaxed), + HIT.load(Ordering::Relaxed), + ); + } + + // ---- Delivering, not merely deciding ------------------------------------------------- + + /// Write through every reference `look_mut` hands out, in the extension-region shape. + /// + /// The properties above compare *decisions*. This one takes the decision up on its offer: it + /// calls `look_mut`, which runs the `unreachable_unchecked` the decision licenses, and then + /// writes through all three references. The assertions are close to beside the point -- what + /// matters is that three `&mut` into one `EmbeddedHeaders` are created and written through, so + /// that miri with stacked borrows enabled can judge whether + /// [`EmbeddedFields`](super::super::pat::EmbeddedFields) handed out two paths to the same + /// layer. The arity-3 shape is the one that borrows from all three of its fields at once, + /// including a slice element, which is the hardest of them to split soundly. + fn exercise_the_embedded_split() { + bolero::check!() + .with_generator(ShapedIcmpError) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + let Some(outer) = owned.as_view_mut::() else { + return; + }; + let Some(ew) = outer.as_embedded_mut::<(&Ipv6, &HopByHop, &TruncatedTcp)>() else { + return; + }; + let (ip, ext, tcp) = ew.look_mut(); + + let want_hops = ip.hop_limit().wrapping_add(1); + ip.set_hop_limit(want_hops); + let seen_ext = ext.next_header(); + let seen_tcp = matches!(tcp, TruncatedTcp::FullHeader(_)); + + assert_eq!( + ip.hop_limit(), + want_hops, + "the write through ipv6 did not stick" + ); + assert_eq!( + ext.next_header(), + seen_ext, + "the extension header changed under a write to ipv6" + ); + assert_eq!( + matches!(tcp, TruncatedTcp::FullHeader(_)), + seen_tcp, + "the quoted transport changed under a write to ipv6" + ); + }); + } + + /// Shards of [`exercise_the_embedded_split`], so the machine can be used. + /// + /// Same reasoning as the shards in [`view`](super::super::view): under miri this is the + /// expensive property and the one that matters most, and its cost is wall-clock rather than + /// cores. Each shard seeds itself from the OS, so `N` shards explore `N` independent streams + /// and nextest runs them concurrently. Eight rather than sixteen -- `view`'s split is the wider + /// one and should keep the larger share of the cores. + macro_rules! split_shards { + ($($name:ident),* $(,)?) => { + $( + #[test] + fn $name() { + exercise_the_embedded_split(); + } + )* + }; + } + + split_shards!( + the_embedded_split_hands_out_distinct_layers, + embedded_split_shard_2, + embedded_split_shard_3, + embedded_split_shard_4, + embedded_split_shard_5, + embedded_split_shard_6, + embedded_split_shard_7, + embedded_split_shard_8, + ); + + /// `look` and `look_mut` pick the same slots, one inner shape per instantiation. + /// + /// Agreeing that a match exists is weaker than agreeing on *which* layers were selected, and + /// this is the only thing in the suite that asks the stronger question across the family. + /// + /// It is also the only thing that *calls* `look` and `look_mut` at more than a couple of shapes, + /// which matters more than it looks: the differential properties above go through + /// [`embedded_sealed::Sealed::matches`] and so exercise [`EmbeddedStep`], while `look_mut` is the + /// sole caller of [`EmbeddedStepMut`] -- a second, separate set of per-layer impls that delegate + /// into [`EmbeddedMatcherMut`]. Deciding a shape matches never runs a line of it. So there is one + /// instantiation per layer that has an `EmbeddedStepMut` impl, and the extension ones are the + /// point: one implementation indexes `net_ext` by a cursor, the other consumes a slice, and + /// nothing before this compared what they returned. + macro_rules! same_layers { + ($name:ident, $gen:expr, $outer:ty, $shape:ty, $($binding:ident),+) => { + #[test] + fn $name() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static SEEN: AtomicUsize = AtomicUsize::new(0); + static HIT: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator($gen) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + SEEN.fetch_add(1, Ordering::Relaxed); + // `h` and `&mut h` cannot be held at once, so the packet is pinned across + // both calls and the raw addresses compared afterwards. + let immutable = { + let Some(outer) = owned.as_view::<$outer>() else { + return; + }; + let Some(ew) = outer.as_embedded::<$shape>() else { + return; + }; + let ($($binding,)+) = ew.look(); + ($(std::ptr::from_ref($binding),)+) + }; + HIT.fetch_add(1, Ordering::Relaxed); + let mutable = { + let outer = owned.as_view_mut::<$outer>().unwrap_or_else(|| { + unreachable!("the same packet matched a moment ago") + }); + let ew = outer.as_embedded_mut::<$shape>().unwrap_or_else(|| { + unreachable!("the same shape matched a moment ago") + }); + let ($($binding,)+) = ew.look_mut(); + ($(std::ptr::from_ref(&*$binding),)+) + }; + assert_eq!( + immutable, mutable, + concat!( + "`look` and `look_mut` selected different layers of the quoted \ + packet for ", + stringify!($shape) + ) + ); + }); + agreement_is_not_vacuous( + concat!("look/look_mut ", stringify!($shape)), + SEEN.load(Ordering::Relaxed), + HIT.load(Ordering::Relaxed), + ); + } + }; + } + + /// The strict gap check applies to the transport *enum* too, not just concrete variants. + /// + /// Stated as a direct assertion rather than a differential, because the immutable oracle cannot + /// express it -- see the note above `mutable_inner_transport_enum`. That leaves the enum's own + /// gap check with nothing else reaching it, and it is worth reaching: naming an extension makes + /// the check strict, so a second, unnamed extension must turn a hit into a miss even though the + /// transport pattern is the permissive one. + /// + /// The second extension is a clone of the first, which keeps this free of any assumption about + /// what other extension types exist while still being two headers where the shape names one. + #[test] + fn the_transport_enum_still_refuses_an_unconsumed_extension() { + bolero::check!() + .with_generator(ShapedQuote { ext: 0, v4: false }) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + let quoted = owned + .embedded_ip_mut() + .unwrap_or_else(|| unreachable!("ShapedQuote always attaches a quote")); + let first = quoted + .net_ext + .first() + .unwrap_or_else(|| unreachable!("ShapedQuote always places one extension")) + .clone(); + quoted.net_ext.push(first); + + let outer = owned + .as_view::() + .unwrap_or_else(|| unreachable!("ShapedQuote draws an ICMPv6 error")); + assert!( + outer + .as_embedded::<(&Ipv6, &HopByHop, &EmbeddedTransport)>() + .is_none(), + "naming one extension of two matched anyway, so the gap check did not run for \ + the transport enum" + ); + // Not naming an extension skips the region silently, which is the other half of the + // same rule and shows the miss above is the gap check rather than a broken shape. + assert!( + outer.as_embedded::<(&Ipv6, &EmbeddedTransport)>().is_some(), + "a shape that never entered the extension region was refused" + ); + }); + } + + // Network layer, both concrete and as the enum, at inner arity 1. These shapes are common + // enough in the broad generator's output -- around one packet in twenty -- to use it. + same_layers!(same_layers_v4_only, ShapedIcmpError, O4V4, (&Ipv4,), ip); + same_layers!(same_layers_v6_only, ShapedIcmpError, O4V6, (&Ipv6,), ip); + same_layers!(same_layers_net_only, ShapedIcmpError, O4V4, (&Net,), net); + + // Every truncated transport, and the transport enum. + same_layers!( + same_layers_v4_tcp, + ShapedIcmpError, + O4V4, + (&Ipv4, &TruncatedTcp), + ip, + tcp + ); + same_layers!( + same_layers_v6_udp, + ShapedIcmpError, + O4V6, + (&Ipv6, &TruncatedUdp), + ip, + udp + ); + same_layers!( + same_layers_icmp_in_icmp4, + ShapedIcmpError, + O4V4, + (&Ipv4, &TruncatedIcmp4), + ip, + icmp + ); + same_layers!( + same_layers_icmp_in_icmp6, + ShapedIcmpError, + O4V6, + (&Ipv6, &TruncatedIcmp6), + ip, + icmp + ); + same_layers!( + same_layers_transport_enum, + ShapedIcmpError, + O4V4, + (&Net, &EmbeddedTransport), + net, + transport + ); + + // Every extension header, which is where the two walks diverge most: one indexes `net_ext` by a + // cursor, the other consumes a slice. These use the narrow generator, because on the broad one + // they matched six to twenty-three packets in twenty-four thousand -- see [`ShapedQuote`]. + // + // Each therefore sees a quoted packet with exactly one extension, and that is not a limitation + // of the generator but of what is expressible: embedded shapes stop at arity 3, so a shape can + // name at most one extension, and naming one makes the gap check strict -- a second, unconsumed + // extension is a miss. One is the only number of extensions a matching quote can carry. Cursor + // arithmetic past position zero is consequently unreachable from here, and is covered by the + // differential properties above, which do draw multi-extension quotes and compare the two walks + // on the misses. + same_layers!( + same_layers_ext_hop_by_hop, + ShapedQuote { ext: 0, v4: false }, + O4V6, + (&Ipv6, &HopByHop, &TruncatedTcp), + ip, + ext, + tcp + ); + same_layers!( + same_layers_ext_dest_opts, + ShapedQuote { ext: 1, v4: false }, + O4V6, + (&Ipv6, &DestOpts, &TruncatedTcp), + ip, + ext, + tcp + ); + same_layers!( + same_layers_ext_routing, + ShapedQuote { ext: 2, v4: false }, + O4V6, + (&Ipv6, &Routing, &TruncatedTcp), + ip, + ext, + tcp + ); + same_layers!( + same_layers_ext_fragment, + ShapedQuote { ext: 3, v4: false }, + O4V6, + (&Ipv6, &Fragment, &TruncatedTcp), + ip, + ext, + tcp + ); + same_layers!( + same_layers_ext_v6_auth, + ShapedQuote { ext: 4, v4: false }, + O4V6, + (&Ipv6, &Ipv6Auth, &TruncatedTcp), + ip, + ext, + tcp + ); + same_layers!( + same_layers_ext_v4_auth, + ShapedQuote { ext: 0, v4: true }, + O4V4, + (&Ipv4, &Ipv4Auth, &TruncatedTcp), + ip, + ext, + tcp + ); +} diff --git a/net/src/headers/mod.rs b/net/src/headers/mod.rs index cf409cee4d..68ed7e0176 100644 --- a/net/src/headers/mod.rs +++ b/net/src/headers/mod.rs @@ -1194,7 +1194,10 @@ where mod contract { use crate::eth::ethtype::CommonEthType; use crate::eth::{Eth, GenWithEthType}; - use crate::headers::{Headers, MAX_NET_EXTENSIONS, MAX_VLANS, Net, NetExt, Transport}; + use crate::headers::{ + EmbeddedHeaders, EmbeddedTransport, Headers, MAX_NET_EXTENSIONS, MAX_VLANS, Net, NetExt, + Transport, + }; use crate::icmp4::Icmp4; use crate::icmp6::Icmp6; use crate::ipv4; @@ -1290,31 +1293,267 @@ mod contract { } // Extension headers hang off the net layer, so there is nothing to attach them to without - // one. The IPv4 authentication header is the only one that belongs on a v4 packet. - let ipv4 = matches!(headers.net, Some(Net::Ipv4(_))); + // one. if headers.net.is_some() { - let exts = - driver.gen_usize(Bound::Included(&0), Bound::Included(&MAX_NET_EXTENSIONS))?; - for _ in 0..exts { - let ext = if ipv4 { - NetExt::Ipv4Auth(driver.produce()?) - } else { - match driver.gen_u8(Bound::Included(&0), Bound::Included(&4))? { - 0 => NetExt::HopByHop(driver.produce()?), - 1 => NetExt::DestOpts(driver.produce()?), - 2 => NetExt::Routing(driver.produce()?), - 3 => NetExt::Fragment(driver.produce()?), - _ => NetExt::Ipv6Auth(driver.produce()?), - } - }; - headers.net_ext.push(ext); - } + headers.net_ext = ext_run(driver, matches!(headers.net, Some(Net::Ipv4(_))))?; } Some(headers) } } + /// Draw a run of 0..=[`MAX_NET_EXTENSIONS`] extension headers for the given family. + /// + /// The IPv4 authentication header is the only extension that belongs on a v4 packet, so `v4` + /// selects between one choice and five rather than merely reweighting them. + /// + /// A quarter of the v6 runs follow RFC 8200's recommended order instead of drawing each slot + /// independently. Without that bias a shape naming three specific extensions in sequence is + /// drawn about once in fifteen thousand packets, which is enough to keep a property honest and + /// nowhere near enough for it to find anything: measured at two hits in 36,000 cases. The + /// ordered runs are also the ones a real peer sends, so this makes the generator both more + /// useful and more realistic. + fn ext_run( + driver: &mut D, + v4: bool, + ) -> Option> { + let mut out = ArrayVec::default(); + let count = driver.gen_usize(Bound::Included(&0), Bound::Included(&MAX_NET_EXTENSIONS))?; + let ordered = driver.gen_u8(Bound::Included(&0), Bound::Included(&3))? == 0; + for slot in 0..count { + let pick = if ordered { + u8::try_from(slot).unwrap_or(u8::MAX) + } else { + driver.gen_u8(Bound::Included(&0), Bound::Included(&4))? + }; + out.push(one_ext(driver, v4, pick)?); + } + Some(out) + } + + /// Draw one extension header, with fuzzed contents. + /// + /// `pick` indexes the IPv6 extension order RFC 8200 recommends -- 0 hop-by-hop, 1 destination + /// options, 2 routing, 3 fragment, anything else authentication -- and is ignored for `v4`, + /// where the IPv4 authentication header is the only extension there is. + fn one_ext(driver: &mut D, v4: bool, pick: u8) -> Option { + if v4 { + return Some(NetExt::Ipv4Auth(driver.produce()?)); + } + Some(match pick { + 0 => NetExt::HopByHop(driver.produce()?), + 1 => NetExt::DestOpts(driver.produce()?), + 2 => NetExt::Routing(driver.produce()?), + 3 => NetExt::Fragment(driver.produce()?), + _ => NetExt::Ipv6Auth(driver.produce()?), + }) + } + + /// Draws an ICMP error whose quoted packet carries exactly one chosen extension header, and + /// nothing else in the way. + /// + /// Companion to [`ShapedIcmpError`], and the division between them is deliberate. A property + /// comparing *hit against miss* needs both, so it wants the broad generator. A property that + /// only inspects matches -- does `look` pick the same layer `look_mut` does -- gets nothing from + /// a packet it skips, so for that one a narrow generator is not a weaker test but a stronger + /// one. + /// + /// The difference is not marginal. Naming a specific extension inside a quoted packet compounds + /// six independent conditions, one of which is `P(no VLAN tags) = 1/5`, since a tag the outer + /// shape does not name is a miss. Measured on [`ShapedIcmpError`], + /// `(&Ipv6, &DestOpts, &TruncatedTcp)` matched 6 packets in 23,910: a property doing real work + /// six times a second, and a hit rate low enough that asserting it is ever non-zero is itself a + /// coin flip. This generator produces that shape every time, and fuzzes the contents instead. + #[allow(dead_code)] // constructed through `.with_generator()` + pub struct ShapedQuote { + /// Which extension header to place, as the `pick` index [`one_ext`] uses. + pub ext: u8, + /// Family of both the quoting message and the packet it quotes. + pub v4: bool, + } + + impl ValueGenerator for ShapedQuote { + type Output = Headers; + + fn generate(&self, driver: &mut D) -> Option { + let eth_type = if self.v4 { + CommonEthType::Ipv4 + } else { + CommonEthType::Ipv6 + }; + let eth = GenWithEthType(eth_type.into()).generate(driver)?; + + let mut quoted_ext = ArrayVec::default(); + quoted_ext.push(one_ext(driver, self.v4, self.ext)?); + let quoted_transport = EmbeddedTransport::Tcp(driver.produce()?); + + let (net, transport, quoted_net) = if self.v4 { + ( + Net::Ipv4( + ipv4::GenWithNextHeader(ipv4::CommonNextHeader::Icmp4.into()) + .generate(driver)?, + ), + Transport::Icmp4(driver.produce()?), + Net::Ipv4( + ipv4::GenWithNextHeader(ipv4::CommonNextHeader::Tcp.into()) + .generate(driver)?, + ), + ) + } else { + ( + Net::Ipv6( + ipv6::GenWithNextHeader(ipv6::CommonNextHeader::Icmp6.into()) + .generate(driver)?, + ), + Transport::Icmp6(driver.produce()?), + Net::Ipv6( + ipv6::GenWithNextHeader(ipv6::CommonNextHeader::Tcp.into()) + .generate(driver)?, + ), + ) + }; + + Some(Headers { + eth: Some(eth), + // No VLAN tags and no outer extensions: a tag the outer shape does not name is a + // miss, and this generator exists to stop producing misses. + vlan: ArrayVec::default(), + net: Some(net), + net_ext: ArrayVec::default(), + transport: Some(transport), + udp_encap: None, + embedded_ip: Some(EmbeddedHeaders::new( + Some(quoted_net), + Some(quoted_transport), + quoted_ext, + None, + )), + }) + } + } + + /// Draws an ICMP error message quoting an inner packet, with the layer structure of *both* the + /// outer message and the quoted packet varying. + /// + /// [`CommonHeaders`] cannot produce one at all: all six of its construction sites set + /// `embedded_ip: None`, so [`ShapedHeaders`], which builds on it, cannot either. That is a + /// better explanation of [`embedded_view`](crate::headers::embedded_view) sitting at 34% line + /// coverage than any claim about missing tests -- its hand-written tests are thorough, but every + /// one of them builds its packet through `HeaderStack`/`header_chain`, which pin the shape at + /// `(Eth, Ipv4, Icmp4)` outside and `(Ipv4, Tcp)` inside and offer no way to attach an extension + /// header to either side. Six of the eight `as_embedded` arities and the whole embedded + /// extension region were unreachable by construction. + /// + /// What varies here is what `EmbeddedShape`, `EmbeddedStep` and + /// [`ExtGapCheck`](crate::headers::pat::ExtGapCheck) actually turn on: + /// + /// * outer VLAN tags and outer extension headers, so the outer arity spans the range of + /// `as_embedded` impls instead of only the one a builder can express; + /// * whether the embedded section is present at all -- an ICMP message that is not an error + /// quotes nothing, and every embedded shape must miss on it; + /// * the inner network layer, its extension headers, and its truncated transport, including a + /// quote that stops before the transport header and the ICMP-inside-ICMP case. + /// + /// The inner family follows the outer family most of the time, because that is what a real ICMP + /// error looks like: an `ICMPv4` message quotes an IPv4 packet. It deliberately does not always. + /// A family mismatch is where two independent structural walks are most likely to disagree, so + /// it wants drawing rather than assuming away. + /// + /// Structural on purpose, for the same reason as [`ShapedHeaders`]: `next_header` is set to + /// match the transport where one was drawn, but nothing keeps the extension chain's own + /// `next_header` fields consistent, because no code under test reads them. + #[allow(dead_code)] // constructed through `.with_generator()` + #[repr(transparent)] + pub struct ShapedIcmpError; + + impl ValueGenerator for ShapedIcmpError { + type Output = Headers; + + fn generate(&self, driver: &mut D) -> Option { + let outer_v4 = driver.produce::()?; + let eth_type = if outer_v4 { + CommonEthType::Ipv4 + } else { + CommonEthType::Ipv6 + }; + let eth = GenWithEthType(eth_type.into()).generate(driver)?; + + let mut vlan = ArrayVec::default(); + let vlans = driver.gen_usize(Bound::Included(&0), Bound::Included(&MAX_VLANS))?; + for _ in 0..vlans { + vlan.push(driver.produce()?); + } + + let (net, transport) = if outer_v4 { + let ip = ipv4::GenWithNextHeader(ipv4::CommonNextHeader::Icmp4.into()) + .generate(driver)?; + (Net::Ipv4(ip), Transport::Icmp4(driver.produce()?)) + } else { + let ip = ipv6::GenWithNextHeader(ipv6::CommonNextHeader::Icmp6.into()) + .generate(driver)?; + (Net::Ipv6(ip), Transport::Icmp6(driver.produce()?)) + }; + + // Echo requests and replies are not errors and quote nothing, so absence is a shape the + // API has to handle, not an edge case to skip. + let embedded_ip = if driver.produce::()? { + Some(quoted_packet(driver, outer_v4)?) + } else { + None + }; + + Some(Headers { + eth: Some(eth), + vlan, + net: Some(net), + net_ext: ext_run(driver, outer_v4)?, + transport: Some(transport), + udp_encap: None, + embedded_ip, + }) + } + } + + /// Draw the packet quoted inside an ICMP error. `outer_v4` is the family of the quoting + /// message; see [`ShapedIcmpError`] for why the quoted packet usually but not always shares it. + fn quoted_packet(driver: &mut D, outer_v4: bool) -> Option { + let mismatch = driver.gen_u8(Bound::Included(&0), Bound::Included(&7))? == 0; + let v4 = outer_v4 != mismatch; + + // Pick the transport first so the network layer's `next_header` can name it. A quote that + // stops before the transport header is the truncation the `Truncated*` types exist for. + let transport = match driver.gen_u8(Bound::Included(&0), Bound::Included(&3))? { + 0 => Some(EmbeddedTransport::Tcp(driver.produce()?)), + 1 => Some(EmbeddedTransport::Udp(driver.produce()?)), + 2 if v4 => Some(EmbeddedTransport::Icmp4(driver.produce()?)), + 2 => Some(EmbeddedTransport::Icmp6(driver.produce()?)), + _ => None, + }; + + let net = if v4 { + let next = match transport { + Some(EmbeddedTransport::Udp(_)) => ipv4::CommonNextHeader::Udp, + Some(EmbeddedTransport::Icmp4(_)) => ipv4::CommonNextHeader::Icmp4, + _ => ipv4::CommonNextHeader::Tcp, + }; + Net::Ipv4(ipv4::GenWithNextHeader(next.into()).generate(driver)?) + } else { + let next = match transport { + Some(EmbeddedTransport::Udp(_)) => ipv6::CommonNextHeader::Udp, + Some(EmbeddedTransport::Icmp6(_)) => ipv6::CommonNextHeader::Icmp6, + _ => ipv6::CommonNextHeader::Tcp, + }; + Net::Ipv6(ipv6::GenWithNextHeader(next.into()).generate(driver)?) + }; + + Some(EmbeddedHeaders::new( + Some(net), + transport, + ext_run(driver, v4)?, + None, + )) + } + #[allow(dead_code)] // rustc not able to infer we construct this through .with_generator() #[repr(transparent)] pub struct CommonHeaders; diff --git a/net/src/headers/view.rs b/net/src/headers/view.rs index ee480085ca..19fbe2bcf0 100644 --- a/net/src/headers/view.rs +++ b/net/src/headers/view.rs @@ -2750,6 +2750,9 @@ mod view_mut_properties { use crate::eth::Eth; use crate::headers::view::LookMut; use crate::headers::{Headers, Net, ShapedHeaders, Transport}; + use crate::ip_auth::Ipv4Auth; + use crate::ipv4::Ipv4; + use crate::ipv6::{DestOpts, HopByHop, Ipv6, Routing}; use crate::vlan::Vlan; /// Both walks, at every arity the oracle can express. @@ -2852,22 +2855,14 @@ mod view_mut_properties { } } - // Every arity from one to seven, which is as far as the oracle reaches. + // Every arity from one to eight, which is all of them. // - // `Matcher`'s vocabulary is `eth`, `vlan`, `net`, `transport`, `vxlan` and `embedded`. The last two - // are unreachable in a builder chain: `Vxlan: Within` and the embedded header sits under - // `Icmp4`/`Icmp6`, both *concrete* layers, and `Matcher` has no concrete-layer methods -- no - // `.udp()`, no `.tcp()`. So the longest chain it can express is `Eth`, four VLAN tags - // (`MAX_VLANS`), `Net`, `Transport`: seven. - // - // Two gaps follow, and both are about the oracle rather than the code: - // - // * the **arity-8** arm of the macro is generated and goes unchecked here, because nothing the - // oracle can say is eight elements long; - // * shapes entering the **IPv6 extension region** cannot be expressed at all, and that is the - // more interesting loss -- `ExtGapCheck` is the subtlest part of the contract, the part the - // module documentation spends most of its words on, and it has no oracle. Closing it means - // either extension-header methods on `Matcher` or a different oracle. + // `Matcher` names every layer the `Within` graph does -- `matcher_net!`, `matcher_ext!` and + // `matcher_transport!` give it `.ipv4()`, `.ipv6()`, `.hop_by_hop()`, `.dest_opts()`, + // `.routing()`, `.fragment()`, `.ipv4_auth()`, `.ipv6_auth()`, `.tcp()`, `.udp()`, `.icmp4()` and + // `.icmp6()` alongside the generic `.eth()` / `.vlan()` / `.net()` / `.transport()`. So the + // oracle reaches as far as the code does: `Eth` + four VLAN tags (`MAX_VLANS`) + `Ipv6` + + // `HopByHop` + `Transport` is eight, and the extension region is expressible. arity_agrees!(read_1, mutable_1, (&Eth,), eth); arity_agrees!(read_2, mutable_2, (&Eth, &Net), eth, net); arity_agrees!( @@ -2920,6 +2915,89 @@ mod view_mut_properties { net, transport ); + arity_agrees!( + read_8, + mutable_8, + ( + &Eth, &Vlan, &Vlan, &Vlan, &Vlan, &Ipv6, &HopByHop, &Transport + ), + eth, + vlan, + vlan, + vlan, + vlan, + ipv6, + hop_by_hop, + transport + ); + + // Shapes that enter the IPv6 extension region, where `ExtGapCheck` switches from + // skip-extensions-silently to consume-them-all. + // + // This is the subtlest part of the contract and the part the module documentation spends most of + // its words on. It is also where the two walks are least alike: on the read path both call + // `ExtGapCheck::ext_gap_ok`, so what is under test is the by-hand threading of the `ec` cursor + // through each separately generated arity -- which is the plausible bug. On the mutable path + // they call genuinely different implementations, `ext_gap_ok` against a `Headers` versus + // `ext_gap_ok_mut` against a pre-split `Fields`, so those two are checked against each other + // as well. + // + // Each shape below is deliberately exact: naming `HopByHop` and then `Transport` matches only a + // packet carrying that extension and no other, since an unconsumed extension is a miss once the + // chain has entered the region. + arity_agrees!( + read_ext_v6_one, + mutable_ext_v6_one, + (&Eth, &Ipv6, &HopByHop, &Transport), + eth, + ipv6, + hop_by_hop, + transport + ); + arity_agrees!( + read_ext_v6_two, + mutable_ext_v6_two, + (&Eth, &Ipv6, &HopByHop, &DestOpts, &Transport), + eth, + ipv6, + hop_by_hop, + dest_opts, + transport + ); + arity_agrees!( + read_ext_v6_three, + mutable_ext_v6_three, + (&Eth, &Ipv6, &HopByHop, &DestOpts, &Routing, &Transport), + eth, + ipv6, + hop_by_hop, + dest_opts, + routing, + transport + ); + // The IPv4 authentication header is the only extension that belongs on a v4 packet, and it is + // the only way to reach the strict branch without IPv6. + arity_agrees!( + read_ext_v4_auth, + mutable_ext_v4_auth, + (&Eth, &Ipv4, &Ipv4Auth, &Transport), + eth, + ipv4, + ipv4_auth, + transport + ); + // Entering the region and then *not* naming a transport: the gap check never runs, so every + // extension after the named one is simply left unvisited. Distinguishing this from the strict + // case above is the whole point of running the check at the transport step rather than the + // extension step. + arity_agrees!( + read_ext_v6_no_transport, + mutable_ext_v6_no_transport, + (&Eth, &Ipv6, &HopByHop), + eth, + ipv6, + hop_by_hop + ); /// Exercise the multi-`&mut` split itself: write through every reference and read the writes back. /// From 1f0592ee56309813b4c6f350740883540b7f790c Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 18:01:39 -0600 Subject: [PATCH 26/30] test(net): Cover the optional matchers, and find what their combinators watch `pat.rs` had 296 unreached lines and 250 of them were one thing: every `opt_*` method on all four matchers, plus `when`, `inspect` and `otherwise` on each. The optional half of the pattern-matching API had never been run. That half is not a thin wrapper over the strict half. Three families live under one naming convention. `opt_eth` cannot miss. `opt_vlan` and the optional extension methods cannot miss either, but advance their cursor only when they matched, so they skip rather than refuse -- and an optional extension still moves `Pos` into the extension region, which makes a later, unrelated transport step strict. `opt_net`, the optional transports and `opt_vxlan` are three-way: present and right is a hit, absent is a hit carrying `None`, present and wrong is a miss. Conflating the middle case with either neighbour is the mistake the design invites. Nothing could draw the middle case. `CommonHeaders` sets every layer on every path, so `net` and `transport` are always `Some` in anything it or `ShapedHeaders` produces, and the absent-layer arm of every optional method was unreachable by construction. `ThinHeaders` truncates the stack by suffix, which is the only shape a real short packet takes. `ShapedIcmpError` gained the quote too short to hold a network header, which RFC 792's header-plus- eight-bytes makes an ordinary thing rather than an exotic one. One invariant covers all three families and every layer: weakening a requirement cannot turn a match into a miss. It is worth stating because `map` and `and_then` differ by exactly that, and it is the direction a mis-wiring inverts. The guard on it counts both outcomes -- the strict form must sometimes match, and the optional form must sometimes accept what the strict form refused. An implication passes for free when its antecedent never holds, and just as quietly when the two sides never differ, which is the more likely failure and would leave the optional method's whole reason for existing untested. The property found something. `EmbeddedMatcher` and `EmbeddedMatcherMut` carry two accumulators and `done()` requires both, but `when`, `inspect` and `otherwise` all read the inner one alone. A packet whose outer chain fails -- an unconsumed VLAN tag will do it -- but whose quoted packet matches will run `inspect`, skip `otherwise`, and then return `None`. It happens to 4,974 packets in 25,000, so it is the common case rather than a corner. The doc comments say "inner accumulator" and "inner match", so this is documented rather than broken, but `otherwise` is the error-handling hook and there is a class of failure it stays silent for. `the_embedded_combinators_track_the_ inner_match_only` pins the behaviour as it stands and will fail if it is ever changed, so that becomes a decision rather than a discovery. A second finding, this one about what cannot be written down. The enum-level vocabulary is complete on the outer matchers and mostly missing on the embedded ones: `EmbeddedMatcher` has `net` but not `opt_net`, `transport` or `opt_transport`; `EmbeddedMatcherMut` has `net` and `transport` but neither optional form. All five missing methods are hand-written rather than macro-generated, which is likely how they came to be missing, since every per-variant method is present. The consequence is that a shape naming `Net` or `EmbeddedTransport` inside a quoted packet cannot be expressed as a matcher chain at all -- which is also why `embedded_view`'s differential properties have no read-side oracle for the enum forms. The table is in the source next to the tests that would use them. Verified by breaking the code twice. Making an optional extension advance its cursor unconditionally fails exactly the gap-check property, and nothing else. Making `opt_eth` refuse an absent Ethernet header fails exactly `read_opt_eth`, through the both-outcomes guard, reporting that the optional form never accepted anything the strict form refused. Coverage: `pat.rs` 63.2% -> 98.8%, 296 uncovered lines down to 10. `net/` 69.8% -> 73.1%. Of the 10 left, three are defensive `unreachable!()` and the rest are gap-fail arms on the mutable optional paths. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 31ee5a6d5a76d5cd2362785fefba774066891e29) --- net/src/headers/mod.rs | 57 ++++ net/src/headers/pat.rs | 735 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 792 insertions(+) diff --git a/net/src/headers/mod.rs b/net/src/headers/mod.rs index 68ed7e0176..58a05df01e 100644 --- a/net/src/headers/mod.rs +++ b/net/src/headers/mod.rs @@ -1331,6 +1331,56 @@ mod contract { Some(out) } + /// Draws a packet with layers deliberately missing, which is what the optional matchers are for. + /// + /// [`CommonHeaders`] sets every layer it knows about on every path, so `net` and `transport` are + /// always `Some` in anything it or [`ShapedHeaders`] produces. That makes the absent-layer arm of + /// every `opt_*` method in [`pat`](crate::headers::pat) -- `None => Some(a.append(None))`, the + /// arm that distinguishes "the layer is not there, which is fine" from "the layer is there and is + /// the wrong one, which is a miss" -- unreachable by any generator in the crate. The whole point + /// of an optional matcher is the packet that stops early, and nothing could draw one. + /// + /// Truncation here is by *suffix*, because that is the only shape a real short packet takes: a + /// layer sits inside the one below it, so a packet cannot carry a transport header without a + /// network header to carry it. Dropping a suffix also keeps `net_ext` consistent, since + /// extensions hang off the network layer and have to go when it does. + #[allow(dead_code)] // constructed through `.with_generator()` + #[repr(transparent)] + pub struct ThinHeaders; + + impl ValueGenerator for ThinHeaders { + type Output = Headers; + + fn generate(&self, driver: &mut D) -> Option { + let mut headers = ShapedHeaders.generate(driver)?; + // Keep the full stack a fifth of the time, so a property reading this generator still + // sees the ordinary case alongside the truncated ones. + match driver.gen_u8(Bound::Included(&0), Bound::Included(&4))? { + 0 => {} + 1 => headers.udp_encap = None, + 2 => { + headers.udp_encap = None; + headers.transport = None; + } + 3 => { + headers.udp_encap = None; + headers.transport = None; + headers.net_ext.clear(); + headers.net = None; + } + _ => { + headers.udp_encap = None; + headers.transport = None; + headers.net_ext.clear(); + headers.net = None; + headers.vlan.clear(); + headers.eth = None; + } + } + Some(headers) + } + } + /// Draw one extension header, with fuzzed contents. /// /// `pick` indexes the IPv6 extension order RFC 8200 recommends -- 0 hop-by-hop, 1 destination @@ -1517,6 +1567,13 @@ mod contract { /// Draw the packet quoted inside an ICMP error. `outer_v4` is the family of the quoting /// message; see [`ShapedIcmpError`] for why the quoted packet usually but not always shares it. fn quoted_packet(driver: &mut D, outer_v4: bool) -> Option { + // A quoting host copies as much of the offending packet as it can, and RFC 792 asked for + // only the header plus eight bytes. A quote can therefore be too short to hold even the + // network header -- which is the one case where the optional embedded matchers' absent-layer + // arm is reachable, since a quote that *has* a network layer always has a version. + if driver.gen_u8(Bound::Included(&0), Bound::Included(&7))? == 0 { + return Some(EmbeddedHeaders::new(None, None, ArrayVec::default(), None)); + } let mismatch = driver.gen_u8(Bound::Included(&0), Bound::Included(&7))? == 0; let v4 = outer_v4 != mismatch; diff --git a/net/src/headers/pat.rs b/net/src/headers/pat.rs index a5f640119d..623c5fc032 100644 --- a/net/src/headers/pat.rs +++ b/net/src/headers/pat.rs @@ -2955,3 +2955,738 @@ mod tests { }; } } + +// =========================================================================== +// Optional-layer and combinator properties +// =========================================================================== + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] // fine to unwrap in tests +mod opt_properties { + use super::*; + use crate::headers::{Headers, ShapedIcmpError, ThinHeaders}; + use std::cell::Cell; + + /// `opt_X` is never stricter than `X`, for every layer and on both the read and mutable paths. + /// + /// One invariant, universally true, across three families whose internals differ sharply. + /// `opt_eth` cannot miss at all. `opt_vlan` and the optional extension methods cannot miss + /// either, but advance their cursor only when they matched, so they skip rather than refuse. + /// `opt_net`, the optional transport methods and `opt_vxlan` are three-way: present and right is + /// a hit, absent is a hit carrying `None`, present and wrong is a miss. Whatever the family, + /// weakening a requirement cannot turn a match into a miss -- and the direction is the thing a + /// mis-wiring would invert, since `and_then` and `map` differ by exactly that. + /// + /// Every one of these methods was uncovered before this: 250 of `pat.rs`'s 296 unreached lines + /// were the `opt_*` family and the combinators below. + macro_rules! opt_is_weaker { + ($read:ident, $mutable:ident, [$($pre:ident),*], $strict:ident, $opt:ident) => { + #[test] + fn $read() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static STRICT: AtomicUsize = AtomicUsize::new(0); + static OPT_ONLY: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ThinHeaders) + .for_each(|h: &Headers| { + let strict = h.pat()$(.$pre())*.$strict().done().is_some(); + let opt = h.pat()$(.$pre())*.$opt().done().is_some(); + assert!( + !strict || opt, + concat!( + "`", stringify!($strict), "` matched where `", stringify!($opt), + "` did not, so the optional form is the stricter one: {:?}" + ), + h + ); + if strict { + STRICT.fetch_add(1, Ordering::Relaxed); + } else if opt { + OPT_ONLY.fetch_add(1, Ordering::Relaxed); + } + }); + both_outcomes_seen( + concat!(stringify!($opt), " (read)"), + STRICT.load(Ordering::Relaxed), + OPT_ONLY.load(Ordering::Relaxed), + ); + } + + #[test] + fn $mutable() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static STRICT: AtomicUsize = AtomicUsize::new(0); + static OPT_ONLY: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ThinHeaders) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + let strict = owned.pat_mut()$(.$pre())*.$strict().done().is_some(); + let opt = owned.pat_mut()$(.$pre())*.$opt().done().is_some(); + assert!( + !strict || opt, + concat!( + "`", stringify!($strict), "` matched where `", stringify!($opt), + "` did not on the mutable path: {:?}" + ), + h + ); + if strict { + STRICT.fetch_add(1, Ordering::Relaxed); + } else if opt { + OPT_ONLY.fetch_add(1, Ordering::Relaxed); + } + }); + both_outcomes_seen( + concat!(stringify!($opt), " (mut)"), + STRICT.load(Ordering::Relaxed), + OPT_ONLY.load(Ordering::Relaxed), + ); + } + }; + } + + /// An implication is satisfied for free when its antecedent never holds, and again when the two + /// sides never differ. + /// + /// `strict implies opt` would pass on a generator that produced nothing matching -- and it would + /// pass just as quietly on one where `opt` never accepted anything `strict` refused, which is the + /// more likely failure and the one that would make the optional method's whole reason for + /// existing untested. So both counts have to be non-zero: the requirement is sometimes met, and + /// relaxing it sometimes matters. + fn both_outcomes_seen(what: &str, strict: usize, opt_only: usize) { + println!("{what}: {strict} strict matches, {opt_only} matched only optionally"); + assert!( + strict > 0, + "{what}: the strict form never matched, so the implication held vacuously" + ); + assert!( + opt_only > 0, + "{what}: the optional form never accepted anything the strict form refused, so being \ + optional was never tested" + ); + } + + opt_is_weaker!(read_opt_eth, mut_opt_eth, [], eth, opt_eth); + opt_is_weaker!(read_opt_vlan, mut_opt_vlan, [eth], vlan, opt_vlan); + opt_is_weaker!(read_opt_net, mut_opt_net, [eth], net, opt_net); + opt_is_weaker!(read_opt_ipv4, mut_opt_ipv4, [eth], ipv4, opt_ipv4); + opt_is_weaker!(read_opt_ipv6, mut_opt_ipv6, [eth], ipv6, opt_ipv6); + opt_is_weaker!( + read_opt_hop_by_hop, + mut_opt_hop_by_hop, + [eth, ipv6], + hop_by_hop, + opt_hop_by_hop + ); + opt_is_weaker!( + read_opt_dest_opts, + mut_opt_dest_opts, + [eth, ipv6], + dest_opts, + opt_dest_opts + ); + opt_is_weaker!( + read_opt_routing, + mut_opt_routing, + [eth, ipv6], + routing, + opt_routing + ); + opt_is_weaker!( + read_opt_fragment, + mut_opt_fragment, + [eth, ipv6], + fragment, + opt_fragment + ); + opt_is_weaker!( + read_opt_ipv6_auth, + mut_opt_ipv6_auth, + [eth, ipv6], + ipv6_auth, + opt_ipv6_auth + ); + opt_is_weaker!( + read_opt_ipv4_auth, + mut_opt_ipv4_auth, + [eth, ipv4], + ipv4_auth, + opt_ipv4_auth + ); + opt_is_weaker!(read_opt_tcp, mut_opt_tcp, [eth, net], tcp, opt_tcp); + opt_is_weaker!(read_opt_udp, mut_opt_udp, [eth, net], udp, opt_udp); + opt_is_weaker!(read_opt_icmp4, mut_opt_icmp4, [eth, ipv4], icmp4, opt_icmp4); + opt_is_weaker!(read_opt_icmp6, mut_opt_icmp6, [eth, ipv6], icmp6, opt_icmp6); + opt_is_weaker!( + read_opt_transport, + mut_opt_transport, + [eth, net], + transport, + opt_transport + ); + opt_is_weaker!( + read_opt_vxlan, + mut_opt_vxlan, + [eth, net, udp], + vxlan, + opt_vxlan + ); + + /// The same invariant for the matchers over a quoted ICMP-error payload. + /// + /// Separate macro only because the chain has to pass through `.embedded()` partway, which is not + /// a layer name. The embedded families mirror the outer ones and are generated by their own set + /// of macros, so every arm needs reaching on its own -- covering `opt_hop_by_hop` says nothing + /// about the five sibling copies `embedded_ext!` emits. + macro_rules! embedded_opt_is_weaker { + ( + $read:ident, $mutable:ident, + [$($o:ident),*], [$($i:ident),*], + $strict:ident, $opt:ident + ) => { + #[test] + fn $read() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static STRICT: AtomicUsize = AtomicUsize::new(0); + static OPT_ONLY: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ShapedIcmpError) + .for_each(|h: &Headers| { + let strict = h.pat()$(.$o())*.embedded()$(.$i())*.$strict() + .done().is_some(); + let opt = h.pat()$(.$o())*.embedded()$(.$i())*.$opt() + .done().is_some(); + assert!( + !strict || opt, + concat!( + "quoted `", stringify!($strict), "` matched where `", + stringify!($opt), "` did not: {:?}" + ), + h + ); + if strict { + STRICT.fetch_add(1, Ordering::Relaxed); + } else if opt { + OPT_ONLY.fetch_add(1, Ordering::Relaxed); + } + }); + both_outcomes_seen( + concat!("quoted ", stringify!($opt), " (read)"), + STRICT.load(Ordering::Relaxed), + OPT_ONLY.load(Ordering::Relaxed), + ); + } + + #[test] + fn $mutable() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static STRICT: AtomicUsize = AtomicUsize::new(0); + static OPT_ONLY: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ShapedIcmpError) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + let strict = owned.pat_mut()$(.$o())*.embedded()$(.$i())*.$strict() + .done().is_some(); + let opt = owned.pat_mut()$(.$o())*.embedded()$(.$i())*.$opt() + .done().is_some(); + assert!( + !strict || opt, + concat!( + "quoted `", stringify!($strict), "` matched where `", + stringify!($opt), "` did not on the mutable path: {:?}" + ), + h + ); + if strict { + STRICT.fetch_add(1, Ordering::Relaxed); + } else if opt { + OPT_ONLY.fetch_add(1, Ordering::Relaxed); + } + }); + both_outcomes_seen( + concat!("quoted ", stringify!($opt), " (mut)"), + STRICT.load(Ordering::Relaxed), + OPT_ONLY.load(Ordering::Relaxed), + ); + } + }; + } + + embedded_opt_is_weaker!( + read_quoted_opt_ipv4, + mut_quoted_opt_ipv4, + [eth, ipv4, icmp4], + [], + ipv4, + opt_ipv4 + ); + embedded_opt_is_weaker!( + read_quoted_opt_ipv6, + mut_quoted_opt_ipv6, + [eth, ipv6, icmp6], + [], + ipv6, + opt_ipv6 + ); + // No `opt_net` or `opt_transport` pair here, and no `transport` on the read side, because the + // embedded matchers do not have them. The enum-level vocabulary is complete on the outer + // matchers and mostly absent on the embedded ones: + // + // | | `net` | `opt_net` | `transport` | `opt_transport` | + // |----------------------|-------|-----------|-------------|-----------------| + // | `Matcher` | yes | yes | yes | yes | + // | `MatcherMut` | yes | yes | yes | yes | + // | `EmbeddedMatcher` | yes | no | no | no | + // | `EmbeddedMatcherMut` | yes | no | yes | no | + // + // Five methods missing, all hand-written rather than macro-generated, which is the likely reason + // -- the per-variant methods come from `embedded_net!` and `embedded_transport!` and are all + // present. The consequence is not cosmetic: a shape naming `Net` or `EmbeddedTransport` inside a + // quoted packet cannot be written as a matcher chain, so `embedded_view`'s differential + // properties have no read-side oracle for the enum forms either. + embedded_opt_is_weaker!( + read_quoted_opt_hop_by_hop, + mut_quoted_opt_hop_by_hop, + [eth, ipv6, icmp6], + [ipv6], + hop_by_hop, + opt_hop_by_hop + ); + embedded_opt_is_weaker!( + read_quoted_opt_dest_opts, + mut_quoted_opt_dest_opts, + [eth, ipv6, icmp6], + [ipv6], + dest_opts, + opt_dest_opts + ); + embedded_opt_is_weaker!( + read_quoted_opt_routing, + mut_quoted_opt_routing, + [eth, ipv6, icmp6], + [ipv6], + routing, + opt_routing + ); + embedded_opt_is_weaker!( + read_quoted_opt_fragment, + mut_quoted_opt_fragment, + [eth, ipv6, icmp6], + [ipv6], + fragment, + opt_fragment + ); + embedded_opt_is_weaker!( + read_quoted_opt_ipv6_auth, + mut_quoted_opt_ipv6_auth, + [eth, ipv6, icmp6], + [ipv6], + ipv6_auth, + opt_ipv6_auth + ); + embedded_opt_is_weaker!( + read_quoted_opt_ipv4_auth, + mut_quoted_opt_ipv4_auth, + [eth, ipv4, icmp4], + [ipv4], + ipv4_auth, + opt_ipv4_auth + ); + embedded_opt_is_weaker!( + read_quoted_opt_tcp, + mut_quoted_opt_tcp, + [eth, ipv4, icmp4], + [ipv4], + tcp, + opt_tcp + ); + embedded_opt_is_weaker!( + read_quoted_opt_udp, + mut_quoted_opt_udp, + [eth, ipv6, icmp6], + [ipv6], + udp, + opt_udp + ); + embedded_opt_is_weaker!( + read_quoted_opt_icmp4, + mut_quoted_opt_icmp4, + [eth, ipv4, icmp4], + [ipv4], + icmp4, + opt_icmp4 + ); + embedded_opt_is_weaker!( + read_quoted_opt_icmp6, + mut_quoted_opt_icmp6, + [eth, ipv6, icmp6], + [ipv6], + icmp6, + opt_icmp6 + ); + + // ---- Exact semantics, per family ------------------------------------------------------ + + /// The three-way families accept a matching layer or an absent one, and refuse a wrong one. + /// + /// `opt_is_weaker` above only pins the direction. This states the whole rule, and the arm it + /// exists for is the middle one: a packet that stops before the layer is a *hit* carrying `None`, + /// while a packet carrying the wrong layer is a miss. Conflating those two is the mistake this + /// family invites, and no generator could produce the first case until `ThinHeaders`. + #[test] + fn the_three_way_families_separate_an_absent_layer_from_a_wrong_one() { + bolero::check!() + .with_generator(ThinHeaders) + .for_each(|h: &Headers| { + // Reachability, spelled out once: a strict `.eth()` needs an Ethernet header, and + // the network step's gap check needs every VLAN tag consumed -- none were named. + let reached_net = h.eth().is_some() && h.vlan().is_empty(); + + assert_eq!( + h.pat().eth().opt_net().done().is_some(), + reached_net, + "the network enum is never the wrong variant, so absent or present must both \ + match: {h:?}" + ); + assert_eq!( + h.pat().eth().opt_ipv4().done().is_some(), + reached_net && !matches!(h.net(), Some(Net::Ipv6(_))), + "opt_ipv4 must accept IPv4 and absence, and refuse IPv6: {h:?}" + ); + assert_eq!( + h.pat().eth().opt_ipv6().done().is_some(), + reached_net && !matches!(h.net(), Some(Net::Ipv4(_))), + "opt_ipv6 must accept IPv6 and absence, and refuse IPv4: {h:?}" + ); + + let reached_transport = reached_net && h.net().is_some(); + assert_eq!( + h.pat().eth().net().opt_transport().done().is_some(), + reached_transport, + "the transport enum is never the wrong variant either: {h:?}" + ); + assert_eq!( + h.pat().eth().net().opt_tcp().done().is_some(), + reached_transport && matches!(h.transport(), None | Some(Transport::Tcp(_))), + "opt_tcp must accept TCP and absence, and refuse every other transport: {h:?}" + ); + + // `opt_vxlan` sits behind a concrete `.udp()`, so its own absent arm is the packet + // that carries UDP and no encapsulation -- much the commoner case in real traffic. + let reached_vxlan = + reached_transport && matches!(h.transport(), Some(Transport::Udp(_))); + assert_eq!( + h.pat().eth().net().udp().opt_vxlan().done().is_some(), + reached_vxlan, + "a UDP packet with no encapsulation must match opt_vxlan: {h:?}" + ); + }); + } + + /// The cursor families skip rather than refuse, and advance only on a hit. + /// + /// This is the sharpest observable difference between the two designs, and it is entirely about + /// the cursor. `opt_vlan` cannot miss, so a chain of them followed by a *strict* network step + /// succeeds exactly when the packet has no more tags than the chain has optional slots -- the + /// strict step's gap check is what makes the cursor's behaviour visible from outside. + #[test] + fn optional_vlans_absorb_one_tag_each_and_only_when_they_match() { + bolero::check!() + .with_generator(ThinHeaders) + .for_each(|h: &Headers| { + let reachable = h.eth().is_some() && h.net().is_some(); + let tags = h.vlan().len(); + assert_eq!( + h.pat().eth().opt_vlan().net().done().is_some(), + reachable && tags <= 1, + "one optional tag absorbed the wrong number of tags: {h:?}" + ); + assert_eq!( + h.pat().eth().opt_vlan().opt_vlan().net().done().is_some(), + reachable && tags <= 2, + "two optional tags absorbed the wrong number of tags: {h:?}" + ); + // `MAX_VLANS` is four, so four optional slots absorb any packet there can be. If + // `opt_vlan` advanced its cursor on a miss this would fail on the untagged packets. + assert_eq!( + h.pat() + .eth() + .opt_vlan() + .opt_vlan() + .opt_vlan() + .opt_vlan() + .net() + .done() + .is_some(), + reachable, + "four optional tags failed to absorb a packet with at most four: {h:?}" + ); + }); + } + + /// Naming an extension optionally still enters the region, which makes the gap check strict. + /// + /// The subtlety worth a test: `opt_hop_by_hop` cannot itself fail, so it looks harmless, but it + /// moves `Pos` to `HopByHop` and that is what `ExtGapCheck` dispatches on. The transport step + /// afterwards therefore demands every extension be consumed -- so an *optional* extension can + /// turn a later, unrelated step into a miss. + #[test] + fn an_optional_extension_still_makes_the_transport_gap_check_strict() { + bolero::check!() + .with_generator(ThinHeaders) + .for_each(|h: &Headers| { + let consumed = + usize::from(matches!(h.net_ext().first(), Some(NetExt::HopByHop(_)))); + let want = h.eth().is_some() + && h.vlan().is_empty() + && matches!(h.net(), Some(Net::Ipv6(_))) + && h.transport().is_some() + && h.net_ext().len() == consumed; + assert_eq!( + h.pat() + .eth() + .ipv6() + .opt_hop_by_hop() + .transport() + .done() + .is_some(), + want, + "an optional extension left the transport gap check lenient: {h:?}" + ); + }); + } + + // ---- Combinators ---------------------------------------------------------------------- + + /// `when`, `inspect` and `otherwise` on all four matchers. + /// + /// Three small methods repeated four times, and all twelve copies were unreached. Two of them + /// are side-effecting, which makes "did it run" the entire contract rather than a detail: + /// `inspect` must run exactly on a match and `otherwise` exactly on a miss, and neither may + /// change the outcome. `when` must be able to destroy a match and must never manufacture one -- + /// on a chain that already failed, a `true` predicate has nothing to revive. + macro_rules! combinators_fire_exactly_once_and_only_when_due { + ($name:ident, $gen:expr, $subject:expr, $fires:expr, $($chain:tt)*) => { + #[test] + fn $name() { + bolero::check!() + .with_generator($gen) + .for_each(|h: &Headers| { + // The chain is a token sequence rather than a closure because a closure + // returning a matcher borrowed from its own argument needs a higher-ranked + // lifetime, which closure inference will not produce. + let mut owned = h.clone(); + let base = owned $($chain)* .done().is_some(); + // What the combinators actually track, which is not always `base`: see + // `the_embedded_combinators_track_the_inner_match_only` below. + let fires: bool = $fires(h); + + let mut owned = h.clone(); + assert!( + owned $($chain)* .when(|_| false).done().is_none(), + concat!($subject, ": a false predicate left the match standing: {:?}"), + h + ); + let mut owned = h.clone(); + assert_eq!( + owned $($chain)* .when(|_| true).done().is_some(), + base, + concat!($subject, ": a true predicate was not a no-op: {:?}"), + h + ); + + let ran = Cell::new(false); + let mut owned = h.clone(); + let after = owned $($chain)* + .inspect(|_| ran.set(true)) + .done() + .is_some(); + assert_eq!( + ran.get(), fires, + concat!($subject, ": inspect ran on a miss or skipped a match: {:?}"), + h + ); + assert_eq!( + after, base, + concat!($subject, ": inspect changed the result: {:?}"), + h + ); + + let ran = Cell::new(false); + let mut owned = h.clone(); + let after = owned $($chain)* + .otherwise(|| ran.set(true)) + .done() + .is_some(); + assert_eq!( + ran.get(), !fires, + concat!($subject, ": otherwise ran on a match or skipped a miss: {:?}"), + h + ); + assert_eq!( + after, base, + concat!($subject, ": otherwise changed the result: {:?}"), + h + ); + }); + } + }; + } + + /// The outer matchers carry one accumulator, so their combinators fire exactly on the result. + fn whole_chain(h: &Headers) -> bool { + h.pat().eth().net().done().is_some() + } + + /// The embedded matchers carry two, and their combinators watch only the inner one. + /// + /// `.embedded().ipv4()` leaves the inner accumulator populated exactly when a quote is present + /// and its network layer is IPv4 -- a condition that says nothing about whether the outer chain + /// that led there succeeded. + fn quoted_ipv4_matched(h: &Headers) -> bool { + h.embedded_ip() + .is_some_and(|e| matches!(e.net(), Some(Net::Ipv4(_)))) + } + + combinators_fire_exactly_once_and_only_when_due!( + matcher_combinators, ThinHeaders, "Matcher", whole_chain, + .pat().eth().net() + ); + combinators_fire_exactly_once_and_only_when_due!( + matcher_mut_combinators, ThinHeaders, "MatcherMut", whole_chain, + .pat_mut().eth().net() + ); + combinators_fire_exactly_once_and_only_when_due!( + embedded_matcher_combinators, ShapedIcmpError, "EmbeddedMatcher", quoted_ipv4_matched, + .pat().eth().ipv4().icmp4().embedded().ipv4() + ); + combinators_fire_exactly_once_and_only_when_due!( + embedded_matcher_mut_combinators, ShapedIcmpError, "EmbeddedMatcherMut", + quoted_ipv4_matched, + .pat_mut().eth().ipv4().icmp4().embedded().ipv4() + ); + + /// `otherwise` does not run for every chain that returns `None`, and `inspect` runs for some. + /// + /// Found by the property above, which originally assumed the combinators tracked `.done()`. + /// They do not, and the difference is only observable on the embedded matchers, which carry two + /// accumulators: `done()` requires *both* to be populated, while `when`, `inspect` and + /// `otherwise` all read the inner one alone. + /// + /// So a packet whose outer chain fails -- an unconsumed VLAN tag will do it -- but whose quoted + /// packet matches will run `inspect`, skip `otherwise`, and then return `None`. The doc comments + /// are accurate ("apply a predicate to the inner accumulator", "run a closure if the inner match + /// has already failed"), so this is documented rather than broken. It still seems worth a + /// decision: `otherwise` is the error-handling hook, and there is a whole class of failure it + /// stays silent for. + /// + /// This test pins the behaviour as it stands. If the combinators are ever changed to track + /// `done()` it will fail, which is the point -- that should be a decision rather than a + /// discovery. + #[test] + fn the_embedded_combinators_track_the_inner_match_only() { + use concurrency::sync::atomic::{AtomicUsize, Ordering}; + static DIVERGED: AtomicUsize = AtomicUsize::new(0); + bolero::check!() + .with_generator(ShapedIcmpError) + .for_each(|h: &Headers| { + let whole = h + .pat() + .eth() + .ipv4() + .icmp4() + .embedded() + .ipv4() + .done() + .is_some(); + let inner = quoted_ipv4_matched(h); + if whole == inner { + return; + } + DIVERGED.fetch_add(1, Ordering::Relaxed); + // Divergence is one-directional: the inner match can succeed where the whole chain + // fails, never the reverse, since `done()` needs the inner accumulator too. + assert!( + inner && !whole, + "the whole chain matched while the inner one did not, which `done` forbids: \ + {h:?}" + ); + let ran = Cell::new(false); + h.pat() + .eth() + .ipv4() + .icmp4() + .embedded() + .ipv4() + .otherwise(|| ran.set(true)) + .done(); + assert!( + !ran.get(), + "`otherwise` ran on a chain whose inner match succeeded; the divergence \ + documented here has been fixed, so this test should be deleted: {h:?}" + ); + }); + let diverged = DIVERGED.load(Ordering::Relaxed); + println!("outer failed while the quote matched: {diverged} packets"); + assert!( + diverged > 0, + "the two never diverged, so this test proved nothing about which one the combinators \ + follow" + ); + } + + /// The quoted transport enum refuses an unconsumed extension on the mutable path too. + /// + /// `EmbeddedMatcherMut::transport` is the one enum-level embedded method that exists, and its + /// gap-check rejection had nothing reaching it. Stated directly rather than differentially, + /// because the read-side counterpart it would be compared against is one of the five missing + /// methods listed above. + #[test] + fn the_quoted_transport_enum_refuses_an_unconsumed_extension() { + bolero::check!() + .with_generator(crate::headers::ShapedQuote { ext: 0, v4: false }) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + let quoted = owned + .embedded_ip_mut() + .unwrap_or_else(|| unreachable!("ShapedQuote always attaches a quote")); + let first = quoted + .net_ext + .first() + .unwrap_or_else(|| unreachable!("ShapedQuote always places one extension")) + .clone(); + quoted.net_ext.push(first); + + let mut two = owned.clone(); + assert!( + two.pat_mut() + .eth() + .ipv6() + .icmp6() + .embedded() + .ipv6() + .hop_by_hop() + .transport() + .done() + .is_none(), + "one extension named of two, yet the transport enum matched: {h:?}" + ); + let mut one = h.clone(); + assert!( + one.pat_mut() + .eth() + .ipv6() + .icmp6() + .embedded() + .ipv6() + .hop_by_hop() + .transport() + .done() + .is_some(), + "the sole extension was named and consumed, yet the chain missed: {h:?}" + ); + }); + } +} From c7cde9b2c3caa237a7a1cb3da6b64fc86f92c955 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 19:32:46 -0600 Subject: [PATCH 27/30] test(net): Make the shape follow the chain, and reach every arity's look `view.rs` sat at 53.8% with 224 production lines unreached, and almost all of them were `look` and `look_mut`: the bodies at arities five through eight had never executed, nor had `look_mut` at one and two. The properties compared `matches` against the matcher chain -- the decision -- and stopped there. The delivery half was checked at arities three and four only, in the split test, because comparing two tuples means destructuring them into a fixed number of bindings and that has to be written out per arity. `Addrs` removes that constraint. Reducing a tuple of references to an array of addresses is arity-generic at the call site even though the impls are not, so the delivery check now runs wherever the decision check does. Addresses rather than values, because two VLAN tags can hold equal bytes without being the same tag, and picking the wrong one out of four is exactly the cursor bug this is looking for: `matches`, `look` and `look_mut` are generated separately at each arity, threading `vc` and `ec` through by hand every time, so a shape naming four tags has four chances to be off by one and the decision check cannot see any of them. Breaking arity six's `look` to read one tag early fails `read_6` and nothing else -- not even `read_ext_v6_three`, the other arity-six shape, whose third layer is an extension header and reads the other cursor. Two more gaps, both of the kind that hides behind a passing suite: `Vxlan` had no `ViewStep` coverage at all, not because the generators could not draw a VXLAN packet -- `CommonHeaders` has been drawing them all along -- but because no shape ever named the layer. It is the one step that runs no gap check and the one layer outside the linear stack, so nothing about it follows from the other arities. Two chains now name it; making the step refuse fails exactly those four tests and nothing else. The branch where the *first* step refuses is generated once per arity and was unreachable at every one of them, since `CommonHeaders` sets `eth` on all six of its paths. `ThinHeaders` can drop it but truncates four packets in five, which would cost the deep shapes most of their hit rate; `SometimesHeadless` drops it one packet in eight, enough for the branch and cheap enough that arity seven still matches six thousand times in thirty-five. The shapes are now derived rather than written. A shape and the chain matching it are one statement said twice, and every instantiation said it twice by hand: `(&Eth, &Ipv6, &HopByHop, &Transport)` next to `.eth().ipv6().hop_by_hop() .transport()`. A pair that disagrees compiles and passes and silently tests something else. `layer_ty!` holds the correspondence once, `shape_of!` builds the tuple from the chain, and the instantiations shrank to the chain alone. That is as far as generating tests from the macro tables usefully goes here. Enumerating the `Within` graph gives 4,755 legal chains up to arity eight -- 2,614 at arity eight alone -- so one property per chain is not a suite anyone would run, and the exhaustive version would have to be a shallow sweep over a fixed corpus rather than a fuzz run. It would also be worth less than it looks: an oracle enumerated from the same table the implementation is generated from cannot notice a wrong table entry. What survives the objection is the differential, since `matches` and the matcher chain are independent implementations and the table only chooses which chains to test, not the verdict. Coverage: `view.rs` 53.8% -> 98.4%, 224 uncovered lines to eight. The eight are the `unreachable_unchecked` arm of each arity, which must stay uncovered -- reaching one is the undefined behaviour the whole `HeadersView` invariant exists to prevent. `net` overall 74.2% -> 76.8%. Also fixes two lints in the previous commit that only appear under `--features bolero,test_buffer,builder` rather than `--all-features`. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit a17ff2e67ef9bf42c578bfaa71b8ef3a8757bb1d) --- net/src/headers/mod.rs | 30 +++++ net/src/headers/pat.rs | 6 +- net/src/headers/view.rs | 263 +++++++++++++++++++++++++++++++++------- 3 files changed, 253 insertions(+), 46 deletions(-) diff --git a/net/src/headers/mod.rs b/net/src/headers/mod.rs index 58a05df01e..76a8712285 100644 --- a/net/src/headers/mod.rs +++ b/net/src/headers/mod.rs @@ -1381,6 +1381,36 @@ mod contract { } } + /// [`ShapedHeaders`] with the Ethernet header taken away one packet in eight. + /// + /// Every `Shape` starts at `Eth`, and the branch where that first step refuses is generated + /// separately at each of the eight arities -- eight copies of `return false`, none of which any + /// generator could reach, because `CommonHeaders` sets `eth` on all six of its paths. + /// + /// [`ThinHeaders`] reaches them, but it truncates four packets in five, and the deep shapes pay + /// for that: arity seven already needs exactly four VLAN tags, so another factor of five would + /// leave it matching too rarely to find anything. One in eight covers the branch at every arity + /// while leaving seven eighths of the draw doing the work it was doing before. + #[allow(dead_code)] // constructed through `.with_generator()` + #[repr(transparent)] + pub struct SometimesHeadless; + + impl ValueGenerator for SometimesHeadless { + type Output = Headers; + + fn generate(&self, driver: &mut D) -> Option { + let mut headers = ShapedHeaders.generate(driver)?; + if driver.gen_u8(Bound::Included(&0), Bound::Included(&7))? == 0 { + // The tags go with it. A VLAN tag is carried by the Ethernet header, so a packet + // holding one without the other is not a short packet, it is an impossible one, and + // the properties reading this generator are about short packets. + headers.vlan.clear(); + headers.eth = None; + } + Some(headers) + } + } + /// Draw one extension header, with fuzzed contents. /// /// `pick` indexes the IPv6 extension order RFC 8200 recommends -- 0 hop-by-hop, 1 destination diff --git a/net/src/headers/pat.rs b/net/src/headers/pat.rs index 623c5fc032..d38a1f0436 100644 --- a/net/src/headers/pat.rs +++ b/net/src/headers/pat.rs @@ -3469,6 +3469,9 @@ mod opt_properties { /// on a chain that already failed, a `true` predicate has nothing to revive. macro_rules! combinators_fire_exactly_once_and_only_when_due { ($name:ident, $gen:expr, $subject:expr, $fires:expr, $($chain:tt)*) => { + // The `mut` binding is what `pat_mut()` needs and what `pat()` does not, and the same + // macro serves both, so half the instantiations declare a `mut` they never use. + #[allow(unused_mut)] #[test] fn $name() { bolero::check!() @@ -3614,7 +3617,8 @@ mod opt_properties { {h:?}" ); let ran = Cell::new(false); - h.pat() + let _ = h + .pat() .eth() .ipv4() .icmp4() diff --git a/net/src/headers/view.rs b/net/src/headers/view.rs index 19fbe2bcf0..9f14ee8135 100644 --- a/net/src/headers/view.rs +++ b/net/src/headers/view.rs @@ -2748,12 +2748,123 @@ mod view_properties { #[cfg(test)] mod view_mut_properties { use crate::eth::Eth; - use crate::headers::view::LookMut; - use crate::headers::{Headers, Net, ShapedHeaders, Transport}; - use crate::ip_auth::Ipv4Auth; - use crate::ipv4::Ipv4; - use crate::ipv6::{DestOpts, HopByHop, Ipv6, Routing}; - use crate::vlan::Vlan; + use crate::headers::view::{Look, LookMut}; + use crate::headers::{Headers, Net, ShapedHeaders, SometimesHeadless, Transport}; + + /// The type a `Matcher` method name selects. + /// + /// A shape and the chain that matches it are the same statement written twice -- + /// `(&Eth, &Ipv6, &HopByHop, &Transport)` and `.eth().ipv6().hop_by_hop().transport()` -- and + /// writing both by hand at every instantiation is a standing invitation to write two different + /// statements. The pair that disagrees still compiles and still passes; it just quietly tests + /// something other than what it says. So the chain is the input and the shape is derived. + /// + /// The table is the only place the correspondence lives, and it is the whole of it: every entry + /// below names a method the `matcher_net!`, `matcher_ext!` and `matcher_transport!` invocations + /// in [`pat`](super::pat) generate, plus the four written out by hand. + macro_rules! layer_ty { + (eth) => { + crate::eth::Eth + }; + (vlan) => { + crate::vlan::Vlan + }; + (net) => { + crate::headers::Net + }; + (ipv4) => { + crate::ipv4::Ipv4 + }; + (ipv6) => { + crate::ipv6::Ipv6 + }; + (hop_by_hop) => { + crate::ipv6::HopByHop + }; + (dest_opts) => { + crate::ipv6::DestOpts + }; + (routing) => { + crate::ipv6::Routing + }; + (fragment) => { + crate::ipv6::Fragment + }; + (ipv4_auth) => { + crate::ip_auth::Ipv4Auth + }; + (ipv6_auth) => { + crate::ip_auth::Ipv6Auth + }; + (transport) => { + crate::headers::Transport + }; + (tcp) => { + crate::tcp::Tcp + }; + (udp) => { + crate::udp::Udp + }; + (icmp4) => { + crate::icmp4::Icmp4 + }; + (icmp6) => { + crate::icmp6::Icmp6 + }; + (vxlan) => { + crate::vxlan::Vxlan + }; + } + + /// The `Shape` a chain of matcher method names denotes. + macro_rules! shape_of { + ($($layer:ident),+ $(,)?) => { ($(&'static layer_ty!($layer),)+) }; + } + + /// The address of each layer in a tuple of references, without naming the arity. + /// + /// Agreeing that a shape matches is the soundness question; handing back the *same* layers is the + /// correctness one, and it was previously checked only at arities three and four, because that is + /// where a tuple can be destructured by hand into a fixed number of bindings. Reducing the tuple + /// to its addresses removes the need to name the arity at all, so the check applies wherever the + /// agreement check does. + /// + /// Addresses rather than values: two layers can hold equal bytes without being the same layer, and + /// picking the wrong VLAN tag out of four identical ones is exactly the cursor bug this is looking + /// for. + trait Addrs { + /// One address per element. + type Out: PartialEq + core::fmt::Debug; + /// Where each element of this tuple lives. + fn addrs(&self) -> Self::Out; + } + + macro_rules! impl_addrs { + ($n:literal; $($T:ident $idx:tt),+) => { + impl<'a, $($T),+> Addrs for ($(&'a $T,)+) { + type Out = [usize; $n]; + fn addrs(&self) -> [usize; $n] { + [$(core::ptr::from_ref::<$T>(self.$idx) as usize),+] + } + } + + impl<'a, $($T),+> Addrs for ($(&'a mut $T,)+) { + type Out = [usize; $n]; + fn addrs(&self) -> [usize; $n] { + [$(core::ptr::from_ref::<$T>(&*self.$idx) as usize),+] + } + } + }; + } + + impl_addrs!(1; A 0); + impl_addrs!(2; A 0, B 1); + impl_addrs!(3; A 0, B 1, C 2); + impl_addrs!(4; A 0, B 1, C 2, D 3); + impl_addrs!(5; A 0, B 1, C 2, D 3, E 4); + impl_addrs!(6; A 0, B 1, C 2, D 3, E 4, F 5); + impl_addrs!(7; A 0, B 1, C 2, D 3, E 4, F 5, G 6); + impl_addrs!(8; A 0, B 1, C 2, D 3, E 4, F 5, G 6, H 7); /// Both walks, at every arity the oracle can express. /// @@ -2765,38 +2876,60 @@ mod view_mut_properties { /// a macro, with the VLAN and extension cursors threaded through by hand every time. Testing two /// arities tested two of eight copies. /// - /// Pointer identity -- did the two implementations pick the *same* layer, not merely agree that one - /// exists -- is checked separately, at arities 3 and 4, where the tuple can be destructured - /// concretely. That is a correctness question rather than a soundness one, so it is checked deeply - /// at representative arities rather than shallowly at all of them. + /// Each property asks two things of a shape, and the second is the reason `look` and `look_mut` + /// appear here rather than only in the split test: + /// + /// * **the decision**, `matches` against the matcher chain. This is what soundness turns on: if + /// `matches` says yes where the walk that must deliver says no, `look`/`look_mut` reach + /// `unreachable_unchecked`. Disagreement here is undefined behaviour, not a wrong answer. + /// * **the delivery**, `look`/`look_mut` against the same chain's tuple, compared by address. + /// Agreeing that a shape is present is not the same as picking the same layers out of it, and + /// the cursor arithmetic that decides *which* VLAN tag or *which* extension header comes back + /// is written out by hand once per arity. A shape naming four tags has four chances to be off + /// by one and no way for the decision check to notice. + /// + /// Delivery was previously checked at arities three and four only, because a tuple has to be + /// destructured into a fixed number of bindings to be compared -- which is why [`Addrs`] exists. macro_rules! arity_agrees { - ($read:ident, $mutable:ident, $shape:ty, $($layer:ident),+) => { + ($read:ident, $mutable:ident, $gen:expr, $($layer:ident),+) => { #[test] fn $read() { + type Shape = shape_of!($($layer),+); use concurrency::sync::atomic::{AtomicUsize, Ordering}; static SEEN: AtomicUsize = AtomicUsize::new(0); static HIT: AtomicUsize = AtomicUsize::new(0); bolero::check!() - .with_generator(ShapedHeaders) + .with_generator($gen) .for_each(|h: &Headers| { SEEN.fetch_add(1, Ordering::Relaxed); - let licensed = h.as_view::<$shape>().is_some(); + let licensed = h.as_view::().is_some(); if licensed { HIT.fetch_add(1, Ordering::Relaxed); } - let deliverable = h.pat()$(.$layer())+.done().is_some(); + let chain = h.pat()$(.$layer())+.done(); assert_eq!( - licensed, deliverable, + licensed, chain.is_some(), concat!( "`matches` and the read walk disagree for ", - stringify!($shape), + stringify!(($($layer),+)), ", so `look` would reach `unreachable_unchecked`: {:?}" ), h ); + if let (Some(view), Some(chain)) = (h.as_view::(), chain) { + assert_eq!( + view.look().addrs(), chain.addrs(), + concat!( + "`look` and the read walk agreed that ", + stringify!(($($layer),+)), + " is present and then handed back different layers: {:?}" + ), + h + ); + } }); agreement_is_not_vacuous( - stringify!($shape), + stringify!(($($layer),+)), SEEN.load(Ordering::Relaxed), HIT.load(Ordering::Relaxed), ); @@ -2804,31 +2937,46 @@ mod view_mut_properties { #[test] fn $mutable() { + type Shape = shape_of!($($layer),+); use concurrency::sync::atomic::{AtomicUsize, Ordering}; static SEEN: AtomicUsize = AtomicUsize::new(0); static HIT: AtomicUsize = AtomicUsize::new(0); bolero::check!() - .with_generator(ShapedHeaders) + .with_generator($gen) .for_each(|h: &Headers| { let mut owned = h.clone(); SEEN.fetch_add(1, Ordering::Relaxed); - let licensed = owned.as_view_mut::<$shape>().is_some(); + let licensed = owned.as_view_mut::().is_some(); if licensed { HIT.fetch_add(1, Ordering::Relaxed); } - let deliverable = owned.pat_mut()$(.$layer())+.done().is_some(); + // The two walks each want the whole of `owned` mutably, so they take turns + // and hand back addresses rather than references. Nothing is written + // between the two, so the addresses stay comparable. + let chain = owned.pat_mut()$(.$layer())+.done().map(|t| t.addrs()); assert_eq!( - licensed, deliverable, + licensed, chain.is_some(), concat!( "`matches` and the mutable walk disagree for ", - stringify!($shape), + stringify!(($($layer),+)), ", so `look_mut` would reach `unreachable_unchecked`: {:?}" ), h ); + if let Some(view) = owned.as_view_mut::() { + assert_eq!( + Some(view.look_mut().addrs()), chain, + concat!( + "`look_mut` and the mutable walk agreed that ", + stringify!(($($layer),+)), + " is present and then handed back different layers: {:?}" + ), + h + ); + } }); agreement_is_not_vacuous( - stringify!($shape), + stringify!(($($layer),+)), SEEN.load(Ordering::Relaxed), HIT.load(Ordering::Relaxed), ); @@ -2863,20 +3011,17 @@ mod view_mut_properties { // `.icmp6()` alongside the generic `.eth()` / `.vlan()` / `.net()` / `.transport()`. So the // oracle reaches as far as the code does: `Eth` + four VLAN tags (`MAX_VLANS`) + `Ipv6` + // `HopByHop` + `Transport` is eight, and the extension region is expressible. - arity_agrees!(read_1, mutable_1, (&Eth,), eth); - arity_agrees!(read_2, mutable_2, (&Eth, &Net), eth, net); - arity_agrees!( - read_3, - mutable_3, - (&Eth, &Net, &Transport), - eth, - net, - transport - ); + // + // `SometimesHeadless` rather than `ShapedHeaders`: the first step of every one of these is `Eth`, + // and the branch where that step refuses is generated once per arity. Nothing that always sets + // `eth` can reach any of them. + arity_agrees!(read_1, mutable_1, SometimesHeadless, eth); + arity_agrees!(read_2, mutable_2, SometimesHeadless, eth, net); + arity_agrees!(read_3, mutable_3, SometimesHeadless, eth, net, transport); arity_agrees!( read_4, mutable_4, - (&Eth, &Vlan, &Net, &Transport), + SometimesHeadless, eth, vlan, net, @@ -2885,7 +3030,7 @@ mod view_mut_properties { arity_agrees!( read_5, mutable_5, - (&Eth, &Vlan, &Vlan, &Net, &Transport), + SometimesHeadless, eth, vlan, vlan, @@ -2895,7 +3040,7 @@ mod view_mut_properties { arity_agrees!( read_6, mutable_6, - (&Eth, &Vlan, &Vlan, &Vlan, &Net, &Transport), + SometimesHeadless, eth, vlan, vlan, @@ -2906,7 +3051,7 @@ mod view_mut_properties { arity_agrees!( read_7, mutable_7, - (&Eth, &Vlan, &Vlan, &Vlan, &Vlan, &Net, &Transport), + SometimesHeadless, eth, vlan, vlan, @@ -2918,9 +3063,7 @@ mod view_mut_properties { arity_agrees!( read_8, mutable_8, - ( - &Eth, &Vlan, &Vlan, &Vlan, &Vlan, &Ipv6, &HopByHop, &Transport - ), + SometimesHeadless, eth, vlan, vlan, @@ -2948,7 +3091,7 @@ mod view_mut_properties { arity_agrees!( read_ext_v6_one, mutable_ext_v6_one, - (&Eth, &Ipv6, &HopByHop, &Transport), + SometimesHeadless, eth, ipv6, hop_by_hop, @@ -2957,7 +3100,7 @@ mod view_mut_properties { arity_agrees!( read_ext_v6_two, mutable_ext_v6_two, - (&Eth, &Ipv6, &HopByHop, &DestOpts, &Transport), + SometimesHeadless, eth, ipv6, hop_by_hop, @@ -2967,7 +3110,7 @@ mod view_mut_properties { arity_agrees!( read_ext_v6_three, mutable_ext_v6_three, - (&Eth, &Ipv6, &HopByHop, &DestOpts, &Routing, &Transport), + SometimesHeadless, eth, ipv6, hop_by_hop, @@ -2980,7 +3123,7 @@ mod view_mut_properties { arity_agrees!( read_ext_v4_auth, mutable_ext_v4_auth, - (&Eth, &Ipv4, &Ipv4Auth, &Transport), + SometimesHeadless, eth, ipv4, ipv4_auth, @@ -2993,12 +3136,42 @@ mod view_mut_properties { arity_agrees!( read_ext_v6_no_transport, mutable_ext_v6_no_transport, - (&Eth, &Ipv6, &HopByHop), + SometimesHeadless, eth, ipv6, hop_by_hop ); + // The UDP encapsulation layer, which no shape named until now. + // + // `Vxlan` is the one `ViewStep` that runs no gap check at all -- it reads `udp_encap` and ignores + // both cursors -- and the one layer that is not part of the linear header stack, so nothing about + // it follows from the arities above. `CommonHeaders` has been drawing VXLAN packets the whole + // time; the gap was that no property ever asked for one. + // + // `ShapedHeaders` rather than `SometimesHeadless` here, because this shape is already narrow: + // `Udp` is one of three next headers, the encapsulation is a coin flip on top of that, and the + // net step demands no VLAN tags. Removing the Ethernet header as well would spend the hit rate + // on a branch the eight arities above already cover. + arity_agrees!( + read_vxlan_v4, + mutable_vxlan_v4, + ShapedHeaders, + eth, + ipv4, + udp, + vxlan + ); + arity_agrees!( + read_vxlan_net, + mutable_vxlan_net, + ShapedHeaders, + eth, + net, + udp, + vxlan + ); + /// Exercise the multi-`&mut` split itself: write through every reference and read the writes back. /// /// The assertions are almost beside the point. What matters is that the references are *created and From 9833e03b1243a972f24bd488d153c43ff4d8f42f Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 20:37:05 -0600 Subject: [PATCH 28/30] test(net): Stack the extension headers the builder always could `within.rs` sat at 32.7%, and the shape of the gap was unusually clean: thirty-two of the thirty-three `conform` bodies that do any work had never run, and the one that had was `DestOpts` inside `HopByHop`. Every unreached one was an IPv6 extension header transition, plus `Vlan` inside `Vlan`. The cause is a seam rather than an oversight. `conform` runs only from `HeaderStack::stack`, and the builder has had `.hop_by_hop()`, `.dest_opts()`, `.routing()`, `.fragment()`, `.ipv4_auth()` and `.ipv6_auth()` all along -- no test ever called one. The generators reach the extension region constantly, but they assemble `Headers` field by field and never go near the builder, so they never conform anything. Two ways to build a packet, and the fuzzing all went down the one that skips this trait. Seventeen chains cover the thirty-two transitions between them. Three extension headers is the ceiling, `MAX_NET_EXTENSIONS`, so the deeper corners of the graph need several short chains rather than one long one. The oracle is deparse-then-parse. Reading back the field `conform` just wrote would check the implementation against itself; the parser decides what follows an IPv6 header by reading that same field, so a `conform` naming the wrong protocol produces bytes that parse as a different packet, or as no packet at all. Naming TCP where the fragment header goes fails exactly the two chains carrying `routing -> fragment`. Each layer's protocol field is scrambled to a fuzzed byte before the next layer is stacked, so `conform` always overwrites a wrong value instead of filling in a blank one. That turns out to be load-bearing rather than cautious, and `Ipv4` inside `Eth` is the proof: `Blank for Eth` already produces `EthType::IPV4`, so on a blank header the conform setting `EthType::IPV4` has nothing to do. Deleting its body passes all seventeen chains unscrambled and fails two of them scrambled. `Vlan` has the same blank and the same exposure. Coverage: `within.rs` 32.7% -> 88.9%. `net` overall 76.8% -> 79.9%. The nineteen lines left are the no-op bodies -- the enum-level impls, the `EmbeddedStart` impls, and everything `impl_truncated_within!` generates. Those are unreachable through the builder, and the compiler says so twice over: `stack::` fails on both `Net: Blank` and `Headers: Install`, either of which would be enough on its own. They exist to give the pattern matcher its `Within` edges, which need the trait but not the method. Documented rather than deleted; whether to keep nineteen uncallable bodies belongs to whoever owns the trait. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit f9607e9ef38fc7545e6a9836246f4e163c6ae3ea) --- net/src/headers/mod.rs | 2 +- net/src/headers/within.rs | 218 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 219 insertions(+), 1 deletion(-) diff --git a/net/src/headers/mod.rs b/net/src/headers/mod.rs index 76a8712285..6514cfec17 100644 --- a/net/src/headers/mod.rs +++ b/net/src/headers/mod.rs @@ -1781,7 +1781,7 @@ mod test { use crate::tcp::{TcpChecksum, TcpChecksumPayload, TcpPort}; use crate::udp::{UdpChecksum, UdpChecksumPayload, UdpPort}; - fn parse_back_test(headers: &Headers) { + pub(crate) fn parse_back_test(headers: &Headers) { let mut buffer = [0_u8; 1024]; let bytes_written = match headers.deparse(&mut buffer[..headers.size().into_non_zero_usize().get()]) { diff --git a/net/src/headers/within.rs b/net/src/headers/within.rs index 2fc8646360..7e72cf2939 100644 --- a/net/src/headers/within.rs +++ b/net/src/headers/within.rs @@ -554,3 +554,221 @@ impl Within for Tcp { impl Within for Udp { fn conform(_parent: &mut Net) {} } + +/// What [`Within::conform`] is for, checked against the parser. +/// +/// `conform` writes the parent's protocol field so it names the child: `EthType::IPV6` on an +/// Ethernet header carrying IPv6, `NextHeader::ROUTING` on an IPv6 header carrying a routing +/// extension. It is the only reason this trait has a method at all -- the ordering half of the +/// contract is enforced by the compiler, by the presence or absence of an impl, and needs no test. +/// +/// Thirty-two of these bodies had never run. Every one of them was an IPv6 extension header +/// transition or `Vlan` inside `Vlan`, which is to say: the whole extension region, plus the second +/// tag of a double-tagged frame. The reason is that `conform` is reached only through +/// [`HeaderStack::stack`](crate::headers::builder::HeaderStack::stack), and while the builder has had +/// `.hop_by_hop()`, `.dest_opts()`, `.routing()`, `.fragment()`, `.ipv4_auth()` and `.ipv6_auth()` +/// all along, no test ever called one. The generators reach the extension region constantly, but +/// they assemble [`Headers`](crate::headers::Headers) field by field and never go through the +/// builder, so they never conform anything. +/// +/// # The oracle +/// +/// A test that builds a packet and then reads back the field `conform` just wrote would be checking +/// the implementation against itself. So the check is deparse-then-parse: the parser decides what +/// follows an IPv6 header by reading its next-header field, which is the field `conform` sets, so a +/// `conform` that names the wrong protocol produces bytes the parser reads as a different packet -- +/// or as no valid packet at all. +/// +/// # Scrambling +/// +/// Each layer's next-header field is set to a fuzzed byte *before* the next layer is stacked, so +/// `conform` is always overwriting a wrong value rather than filling in a blank one. +/// +/// That is not a precaution, it is load-bearing, and the case that proves it is `Ipv4` inside `Eth`. +/// [`Blank`] for `Eth` produces `EthType::IPV4`, so on a blank Ethernet header the conform that sets +/// `EthType::IPV4` has nothing to do: delete its body and every packet still round-trips. Scrambled, +/// the same deletion fails two chains. `Vlan` has the same blank and the same exposure. +/// +/// # What is left, and why it stays uncovered +/// +/// Nineteen no-op `conform` bodies remain unrun: the enum-level impls, the `EmbeddedStart` impls and +/// everything `impl_truncated_within!` generates. They are not merely untested, they are unreachable +/// through the builder, and the compiler says so twice -- writing +/// `HeaderStack::new().eth(..).stack::(..)` fails with both `Net: Blank is not satisfied` and +/// `Headers: Install is not satisfied`. Either bound alone would be enough. +/// +/// They exist to give the pattern matcher its `Within` edges, which need the trait but not the +/// method. `conform` is a public trait method, so they are callable in principle by anyone holding +/// the parent; nothing in the tree does. Whether nineteen uncallable bodies are worth keeping is a +/// question for whoever owns the trait, not something a test can settle. +/// +/// [`Blank`]: crate::headers::builder::Blank +#[cfg(test)] +mod conform_properties { + use crate::eth::Eth; + use crate::eth::ethtype::EthType; + use crate::headers::builder::HeaderStack; + use crate::headers::test::parse_back_test; + use crate::icmp4::Icmp4; + use crate::icmp6::Icmp6; + use crate::ip::NextHeader; + use crate::ip_auth::{Ipv4Auth, Ipv6Auth}; + use crate::ipv4::Ipv4; + use crate::ipv6::{DestOpts, Fragment, HopByHop, Ipv6, Routing}; + use crate::tcp::Tcp; + use crate::udp::Udp; + use crate::vlan::Vlan; + + /// Put a wrong protocol number in the field `conform` is responsible for. + /// + /// Implemented for every layer the builder can stack. The transport types are the leaves of + /// every chain -- nothing is ever stacked on top of one, so nothing ever conforms one -- and + /// their impls are deliberately empty rather than absent, so that the chain macro does not have + /// to know which layers are interior. + trait Scramble { + /// Overwrite the protocol field with something derived from `seed`. + fn scramble(&mut self, seed: u8); + } + + macro_rules! scramble_next_header { + ($($T:ty),+ $(,)?) => {$( + impl Scramble for $T { + fn scramble(&mut self, seed: u8) { + self.set_next_header(NextHeader::new(seed)); + } + } + )+}; + } + + macro_rules! scramble_leaf { + ($($T:ty),+ $(,)?) => {$( + impl Scramble for $T { + fn scramble(&mut self, _seed: u8) {} + } + )+}; + } + + scramble_next_header!( + Ipv4, Ipv6, HopByHop, DestOpts, Routing, Fragment, Ipv4Auth, Ipv6Auth + ); + scramble_leaf!(Tcp, Udp, Icmp4, Icmp6); + + impl Scramble for Eth { + fn scramble(&mut self, seed: u8) { + self.set_ether_type(EthType::new(u16::from(seed))); + } + } + + impl Scramble for Vlan { + fn scramble(&mut self, seed: u8) { + self.set_inner_ethtype(EthType::new(u16::from(seed))); + } + } + + /// Build the named chain through the builder, scrambling as it goes, and round-trip it. + /// + /// The chain is a list of `HeaderStack` method names, and the closures are written here rather + /// than at the call site: a closure written at the call site could not name the `seed` this + /// macro binds, macro hygiene being what it is, and every call site would then have to repeat + /// the same closure once per layer. + macro_rules! conform_chain { + ($name:ident, $($layer:ident),+ $(,)?) => { + #[test] + fn $name() { + bolero::check!().with_type().for_each(|seed: &u8| { + let seed = *seed; + let built = HeaderStack::new() + $(.$layer(|l| l.scramble(seed)))+ + .build_headers(); + let headers = built.unwrap_or_else(|e| { + unreachable!("a blank {} chain does not overflow: {e:?}", stringify!($name)) + }); + parse_back_test(&headers); + }); + } + }; + } + + // Seventeen chains, chosen to cover all thirty-two unreached transitions between them. Three + // extension headers is the ceiling -- `MAX_NET_EXTENSIONS` -- so the deeper regions of the graph + // have to be reached by several chains rather than one long one. + conform_chain!( + double_tag_then_three_extensions, + eth, + vlan, + vlan, + ipv6, + dest_opts, + routing, + fragment, + tcp + ); + conform_chain!( + hop_by_hop_routing_dest_opts, + eth, + ipv6, + hop_by_hop, + routing, + dest_opts, + udp + ); + conform_chain!( + routing_fragment_auth, + eth, + ipv6, + routing, + fragment, + ipv6_auth, + tcp + ); + conform_chain!( + fragment_then_dest_opts, + eth, + ipv6, + fragment, + dest_opts, + icmp6 + ); + conform_chain!( + hop_by_hop_then_fragment, + eth, + ipv6, + hop_by_hop, + fragment, + udp + ); + conform_chain!( + dest_opts_then_fragment, + eth, + ipv6, + dest_opts, + fragment, + icmp6 + ); + conform_chain!(auth_then_dest_opts, eth, ipv6, ipv6_auth, dest_opts, udp); + conform_chain!( + hop_by_hop_then_auth, + eth, + ipv6, + hop_by_hop, + ipv6_auth, + icmp6 + ); + conform_chain!(dest_opts_then_auth, eth, ipv6, dest_opts, ipv6_auth, tcp); + conform_chain!(routing_then_auth, eth, ipv6, routing, ipv6_auth, udp); + conform_chain!(hop_by_hop_then_udp, eth, ipv6, hop_by_hop, udp); + conform_chain!(routing_then_udp, eth, ipv6, routing, udp); + conform_chain!( + hop_by_hop_then_routing_then_tcp, + eth, + ipv6, + hop_by_hop, + routing, + tcp + ); + conform_chain!(hop_by_hop_then_icmp6, eth, ipv6, hop_by_hop, icmp6); + conform_chain!(routing_then_icmp6, eth, ipv6, routing, icmp6); + // The IPv4 authentication header is the only extension that belongs on a v4 packet. + conform_chain!(v4_auth_then_udp, eth, ipv4, ipv4_auth, udp); + conform_chain!(v4_auth_then_icmp4, eth, ipv4, ipv4_auth, icmp4); +} From 214a62b09ebd08a70500dc8fefb062ff49633e77 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 8 Aug 2026 21:09:45 -0600 Subject: [PATCH 29/30] test(net): Specialize the ICMP subtypes, and find that conform is dead The last of `net/headers`. `builder.rs` had two untouched regions, both the same shape as everything else this campaign has turned up: methods the builder has always offered that no test ever called. The twelve ICMP message subtypes are one. `.dest_unreachable()`, `.redirect()`, `.time_exceeded()`, `.param_problem()`, `.echo_request()`, `.echo_reply()` and their v6 twins were unused, and ten of the twelve `Blank` impls behind them had never been called. The two that had are why the shared macro bodies looked covered -- a `macro_rules!` line counts as run once any one of its expansions runs, so a table of twelve generated impls reports green when one of the twelve is exercised, and only the hand-written part of each shows the difference. Scrambling had to change for these. `conform` writes a message type here rather than a protocol number, and the first attempt scrambled it to `Unknown` with a fuzzed type byte -- which fails six chains for a reason that has nothing to do with `conform`: `Unknown { type_u8: 3 }` deparses to the bytes of a destination-unreachable message and parses back as one. The scramble now uses 253 and 200, reserved for experimentation, which belong to no variant and survive the round trip as themselves. The round trip alone cannot check these. The scrambled type is a well-formed ICMP message, so a packet that never got specialized still deparses and parses back perfectly. The subtype chains assert separately that the scramble did not survive the build. That check is what exposes the finding: all twelve `Within for ` conform bodies are dead. Empty them and every test still passes. `Install` runs unconditionally from `build_headers`, after `conform`, and overwrites whatever `conform` wrote; nothing can be stacked on a subtype, so there is no arrangement in which `conform` gets the last word. The two are indistinguishable until the caller customizes the subtype, because until then both write the same value -- `a_customized_subtype_survives_the_build` is the one test that separates them, and it fails on `Install` and not on `conform`. Pinned, not acted on; emptying them is a call for whoever owns the builder. The other region is an ICMP error quoting an ICMP packet. `EmbeddedAssembler::icmp4` and `::icmp6` were the two inner-transport methods nothing called, and with them the arm of `fixup_embedded` that writes `NextHeader::ICMP` onto the quoted IP header. A ping drawing a destination-unreachable is the ordinary way to produce one. The protocol number is asserted directly rather than left to the shape match, which would pass without it; naming TCP there fails exactly the one test. Coverage: `builder.rs` 79.1% -> 97.7%, `within.rs` unchanged at 88.9%, `net` 79.9% -> 81.1%. Eight lines left in `builder.rs`: two defensive `unreachable!` arms, two absent- layer arms, `Blank for ()` which `stack` never instantiates, and `Default for HeaderStack`. A test written to touch the last of those would be a test that exists to move a number, which is the failure mode this campaign has been finding, not one to add. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit 52fd730a791436cea4aa4780333cee7140e0da13) --- net/src/headers/embedded_view.rs | 46 ++++++++++ net/src/headers/within.rs | 142 ++++++++++++++++++++++++++++++- 2 files changed, 185 insertions(+), 3 deletions(-) diff --git a/net/src/headers/embedded_view.rs b/net/src/headers/embedded_view.rs index ad52e28bf7..b07cd5e1f6 100644 --- a/net/src/headers/embedded_view.rs +++ b/net/src/headers/embedded_view.rs @@ -1285,6 +1285,52 @@ mod tests { assert!(matches::<(&Ipv4, &TruncatedTcp)>(e)); } + /// An ICMP error quoting an ICMP packet, which the builder had never been asked to assemble. + /// + /// `EmbeddedAssembler::icmp4` and `::icmp6` were the two inner-transport methods no test called, + /// and with them the `EmbeddedTransport::Icmp4 => NextHeader::ICMP` arm of the fixup that sets + /// the quoted packet's protocol field. A ping that draws a destination-unreachable is the + /// ordinary way to produce one, so the gap was in the tests rather than in the scenario. + /// + /// The inner protocol number is checked directly: `fixup_embedded` is what writes it, the shape + /// match below would pass without it, and a quoted packet whose IP header disagrees with the + /// transport it carries is exactly what a peer cannot parse. + #[test] + fn icmp4_quoted_inside_an_icmp4_error() { + use crate::headers::builder::Blank; + use crate::icmp4::Icmp4; + use crate::ip::NextHeader; + let h = icmp4_with_embedded(|a| a.ipv4(|_| {}).icmp4(Icmp4::blank())); + let e = h.embedded_ip().expect("embedded must be present"); + assert!(matches::<(&Ipv4, &TruncatedIcmp4)>(e)); + let Some(Net::Ipv4(ip)) = e.net() else { + unreachable!("the quoted packet carries an IPv4 header") + }; + assert_eq!( + ip.next_header(), + NextHeader::ICMP, + "the quoted IP header does not name the transport it carries" + ); + } + + #[test] + fn icmp6_quoted_inside_an_icmp6_error() { + use crate::headers::builder::Blank; + use crate::icmp6::Icmp6; + use crate::ip::NextHeader; + let h = icmp6_with_embedded(|a| a.ipv6(|_| {}).icmp6(Icmp6::blank())); + let e = h.embedded_ip().expect("embedded must be present"); + assert!(matches::<(&Ipv6, &TruncatedIcmp6)>(e)); + let Some(Net::Ipv6(ip)) = e.net() else { + unreachable!("the quoted packet carries an IPv6 header") + }; + assert_eq!( + ip.next_header(), + NextHeader::ICMP6, + "the quoted IP header does not name the transport it carries" + ); + } + #[test] fn ipv6_truncated_udp_matches_full_inner_packet() { use crate::udp::UdpPort; diff --git a/net/src/headers/within.rs b/net/src/headers/within.rs index 7e72cf2939..c2e2240537 100644 --- a/net/src/headers/within.rs +++ b/net/src/headers/within.rs @@ -609,8 +609,15 @@ mod conform_properties { use crate::eth::ethtype::EthType; use crate::headers::builder::HeaderStack; use crate::headers::test::parse_back_test; - use crate::icmp4::Icmp4; - use crate::icmp6::Icmp6; + use crate::headers::{Headers, Transport}; + use crate::icmp4::{ + Icmp4, Icmp4DestUnreachable, Icmp4EchoReply, Icmp4EchoRequest, Icmp4ParamProblem, + Icmp4Redirect, Icmp4TimeExceeded, Icmp4Type, + }; + use crate::icmp6::{ + Icmp6, Icmp6DestUnreachable, Icmp6EchoReply, Icmp6EchoRequest, Icmp6PacketTooBig, + Icmp6ParamProblem, Icmp6TimeExceeded, Icmp6Type, + }; use crate::ip::NextHeader; use crate::ip_auth::{Ipv4Auth, Ipv6Auth}; use crate::ipv4::Ipv4; @@ -651,7 +658,52 @@ mod conform_properties { scramble_next_header!( Ipv4, Ipv6, HopByHop, DestOpts, Routing, Fragment, Ipv4Auth, Ipv6Auth ); - scramble_leaf!(Tcp, Udp, Icmp4, Icmp6); + scramble_leaf!(Tcp, Udp); + // The ICMP subtypes are the only layers that can sit on top of an ICMP header, and nothing sits + // on top of them. + scramble_leaf!( + Icmp4DestUnreachable, + Icmp4Redirect, + Icmp4TimeExceeded, + Icmp4ParamProblem, + Icmp4EchoRequest, + Icmp4EchoReply, + Icmp6DestUnreachable, + Icmp6PacketTooBig, + Icmp6TimeExceeded, + Icmp6ParamProblem, + Icmp6EchoRequest, + Icmp6EchoReply, + ); + + // ICMP is the one place where `conform` writes a message type rather than a protocol number, so + // `Unknown` is the scramble: it is the one variant matching no subtype, which makes it wrong for + // every chain below rather than accidentally right for one of them. + // + // The type byte is fixed rather than fuzzed, and has to be. `Unknown` stores the raw byte, so + // `Unknown { type_u8: 3 }` deparses to the same three bytes a destination-unreachable message + // does and parses back as one -- a header the round-trip is right to reject, and nothing to do + // with `conform`. 253 for v4 and 200 for v6 are reserved for experimentation and belong to no + // variant, so they survive the round trip as themselves. The rest of the message stays fuzzed. + impl Scramble for Icmp4 { + fn scramble(&mut self, seed: u8) { + self.set_type(crate::icmp4::Icmp4Type::Unknown { + type_u8: 253, + code_u8: seed, + bytes5to8: [seed; 4], + }); + } + } + + impl Scramble for Icmp6 { + fn scramble(&mut self, seed: u8) { + self.set_type(crate::icmp6::Icmp6Type::Unknown { + type_u8: 200, + code_u8: seed, + bytes5to8: [seed; 4], + }); + } + } impl Scramble for Eth { fn scramble(&mut self, seed: u8) { @@ -673,6 +725,12 @@ mod conform_properties { /// the same closure once per layer. macro_rules! conform_chain { ($name:ident, $($layer:ident),+ $(,)?) => { + conform_chain!(@build $name, |_| {}, $($layer),+); + }; + (specialized $name:ident, $($layer:ident),+ $(,)?) => { + conform_chain!(@build $name, icmp_type_was_specialized, $($layer),+); + }; + (@build $name:ident, $check:expr, $($layer:ident),+) => { #[test] fn $name() { bolero::check!().with_type().for_each(|seed: &u8| { @@ -684,11 +742,35 @@ mod conform_properties { unreachable!("a blank {} chain does not overflow: {e:?}", stringify!($name)) }); parse_back_test(&headers); + $check(&headers); }); } }; } + /// The scrambled ICMP type did not survive into the built packet. + /// + /// Only for chains that end in a subtype layer. A chain ending at a bare `.icmp4()` has nothing + /// above it to conform it, so the scramble is *supposed* to survive there, and round-tripping is + /// the only thing to check. + /// + /// Worth stating why this is separate from the round trip rather than folded into it: the + /// scrambled type is a well-formed ICMP message, so a packet still carrying it deparses and + /// parses back perfectly. The round trip cannot tell that the specialization never happened. + fn icmp_type_was_specialized(headers: &Headers) { + match headers.transport() { + Some(Transport::Icmp4(icmp)) => assert!( + !matches!(icmp.icmp_type(), Icmp4Type::Unknown { type_u8: 253, .. }), + "the scrambled ICMPv4 type survived the build, so nothing specialized it" + ), + Some(Transport::Icmp6(icmp)) => assert!( + !matches!(icmp.icmp_type(), Icmp6Type::Unknown { type_u8: 200, .. }), + "the scrambled ICMPv6 type survived the build, so nothing specialized it" + ), + other => unreachable!("a subtype chain builds an ICMP transport, got {other:?}"), + } + } + // Seventeen chains, chosen to cover all thirty-two unreached transitions between them. Three // extension headers is the ceiling -- `MAX_NET_EXTENSIONS` -- so the deeper regions of the graph // have to be reached by several chains rather than one long one. @@ -771,4 +853,58 @@ mod conform_properties { // The IPv4 authentication header is the only extension that belongs on a v4 packet. conform_chain!(v4_auth_then_udp, eth, ipv4, ipv4_auth, udp); conform_chain!(v4_auth_then_icmp4, eth, ipv4, ipv4_auth, icmp4); + + // The ICMP message subtypes, which the builder can specialize into and which no test had ever + // asked for. Ten of the twelve `Blank` impls behind them had never been called either -- the two + // that had were what made the shared macro bodies look covered, since a `macro_rules!` line + // counts as run once any one of its expansions runs. Worth knowing generally: a table of twelve + // generated impls reports as covered when one of the twelve is exercised, and only the + // hand-written part of each -- here `blank()` -- shows the difference. + conform_chain!(specialized icmp4_dest_unreachable, eth, ipv4, icmp4, dest_unreachable); + conform_chain!(specialized icmp4_redirect, eth, ipv4, icmp4, redirect); + conform_chain!(specialized icmp4_time_exceeded, eth, ipv4, icmp4, time_exceeded); + conform_chain!(specialized icmp4_param_problem, eth, ipv4, icmp4, param_problem); + conform_chain!(specialized icmp4_echo_request, eth, ipv4, icmp4, echo_request); + conform_chain!(specialized icmp4_echo_reply, eth, ipv4, icmp4, echo_reply); + conform_chain!(specialized icmp6_dest_unreachable, eth, ipv6, icmp6, dest_unreachable6); + conform_chain!(specialized icmp6_packet_too_big, eth, ipv6, icmp6, packet_too_big6); + conform_chain!(specialized icmp6_time_exceeded, eth, ipv6, icmp6, time_exceeded6); + conform_chain!(specialized icmp6_param_problem, eth, ipv6, icmp6, param_problem6); + conform_chain!(specialized icmp6_echo_request, eth, ipv6, icmp6, echo_request6); + conform_chain!(specialized icmp6_echo_reply, eth, ipv6, icmp6, echo_reply6); + + /// A subtype the caller customized, which is what separates the two writers of the ICMP type. + /// + /// The chains above cannot tell `Within::conform` from `Install` for these layers, and no test + /// could, because both write the same value: `conform` sets `DestUnreachable(blank())` and + /// `Install` sets `DestUnreachable(value)`, and `value` is `blank()` whenever the caller does not + /// change it. Empty either one alone and the other still produces the right packet. + /// + /// Choosing a code other than the blank one separates them, and the answer is that `conform` is + /// the redundant half. `Install` runs unconditionally from `build_headers`, after `conform`, and + /// overwrites whatever `conform` wrote -- so all twelve of these `conform` bodies could be empty + /// with no observable change. Nothing can be stacked on a subtype, so there is no arrangement in + /// which `conform` gets the last word. + /// + /// Pinned here rather than acted on: emptying them is a call for whoever owns the builder. + #[test] + fn a_customized_subtype_survives_the_build() { + let headers = HeaderStack::new() + .eth(|l| l.scramble(0)) + .ipv4(|l| l.scramble(0)) + .icmp4(|l| l.scramble(0)) + .dest_unreachable(|d| *d = Icmp4DestUnreachable::Port) + .build_headers() + .unwrap_or_else(|e| unreachable!("a blank chain does not overflow: {e:?}")); + + let Some(Transport::Icmp4(icmp)) = headers.transport() else { + unreachable!("the chain builds an ICMPv4 transport") + }; + assert_eq!( + icmp.icmp_type(), + Icmp4Type::DestUnreachable(Icmp4DestUnreachable::Port), + "the code the caller chose did not reach the built packet" + ); + parse_back_test(&headers); + } } From f5188c389b48c727249092a715b3b3dcbe1fdfa1 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Mon, 17 Aug 2026 14:31:57 -0600 Subject: [PATCH 30/30] test(net): Keep the embedded vacuity guard armed under coverage The guard only asserts once a run is long enough for a miss to mean something, and the threshold was set at the twenty-five thousand cases a default one-second run managed when it was written. That leaves no room: coverage instrumentation costs about a fifth of the throughput here -- 17 hits in 20,206 cases against 21 in 25,857 without it -- so the threshold now sits a couple of hundred cases below what CI actually draws. A busier runner drops under it and the check disappears without saying so, which is the failure mode the guard exists to prevent. Ten thousand instead. The thinnest shape in this module draws about eight hits per ten thousand, so at that many draws a shape that really is reachable comes up empty about three times in ten thousand runs. The instrumentation cost is small because these properties are bound by the generator rather than by a counter loop, which is the opposite of the fib test that `--cfg=instrumented` was added for. No iteration counts need cutting here. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland (cherry picked from commit b455ebdcd18e3e66bb313a7db45ee7292666447e) --- net/src/headers/embedded_view.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/net/src/headers/embedded_view.rs b/net/src/headers/embedded_view.rs index b07cd5e1f6..f5aeee0ad6 100644 --- a/net/src/headers/embedded_view.rs +++ b/net/src/headers/embedded_view.rs @@ -1966,9 +1966,15 @@ mod embedded_view_properties { // compounding costs an order of magnitude: the widest pair here matches about one packet in // twenty, and `(&Ipv6, &HopByHop, &TruncatedTcp)` inside an ICMPv6 error matches about one // in a thousand. Five hundred cases at that rate would fail this check outright half the - // time it ran. A default one-second run manages twenty-five thousand, so the guard still - // bites in CI; a deliberately short run should say nothing rather than say something false. - if seen > 20_000 { + // time it ran. A deliberately short run should say nothing rather than say something false. + // + // Ten thousand, not the twenty-five thousand a default run manages here. The thinnest shape + // in this module draws about eight hits per ten thousand, and coverage instrumentation costs + // about a fifth of the throughput -- measured at 17 hits in 20,206 cases against 21 in + // 25,857 without. A threshold set at the observed count would therefore switch itself off on + // any busier runner, silently. At ten thousand draws a shape that really is drawable misses + // entirely about three times in ten thousand runs, which is the trade this wants. + if seen > 10_000 { assert!( hit > 0, "{shape} never matched in {seen} packets: the two walks agree only because the \