diff --git a/.github/workflows/README.md b/.github/workflows/README.md index d28097881f..3c0dc0ba7b 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 @@ -104,6 +109,8 @@ If those queue failures stop being rare, the phasing is worth revisiting. - 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 +- 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 @@ -111,6 +118,83 @@ 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 + ``` + +- `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 = {}, + }, + } + ``` + +- `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 @@ -121,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/.github/workflows/dev.yml b/.github/workflows/dev.yml index 5cb2a33d5c..acd5c20e4f 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,10 +436,7 @@ jobs: fail-fast: false max-parallel: ${{ fromJSON(needs.plan.outputs.parallel) }} matrix: - nix-target: - - frr.dataplane - - dataplane - - 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: @@ -443,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/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. 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/default.nix b/default.nix index 301dba5408..6eaefdc632 100644 --- a/default.nix +++ b/default.nix @@ -159,10 +159,15 @@ let cargo-deny cargo-depgraph cargo-edit + cargo-expand cargo-llvm-cov + cargo-mutants cargo-nextest + cargo-show-asm commitlint-rs + deno direnv + duvet gateway-crd gettext jq @@ -182,6 +187,7 @@ let qemu-user rust-toolchain shellcheck + skim # the `just debug` picker skopeo wasmtime wget @@ -431,6 +437,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 +478,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 @@ -962,32 +977,169 @@ 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" + 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 = '' + # 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" + "-iex" + "add-auto-load-safe-path /lib/rustlib/etc" + "-iex" + "source /lib/rustlib/etc/gdb_load_rust_pretty_printers.py" + "/bin/dataplane" ]; - paths = [ - pkgs.pkgsBuildHost.gdb - pkgs.pkgsBuildHost.rr - pkgs.pkgsBuildHost.coreutils - pkgs.pkgsBuildHost.bashInteractive - pkgs.pkgsBuildHost.iproute2 - pkgs.pkgsBuildHost.ethtool - pkgs.pkgsHostHost.dockerTools.usrBinEnv + Env = [ "HOME=/tmp" ]; + }; + }).overrideAttrs + source-volatile; - pkgs.pkgsHostHost.libc.debug - workspace.cli.debug - workspace.dataplane.debug - workspace.init.debug + # 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 = '' + # 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. + "--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; + + # 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 = '' + # 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" + # Include the worker threads where the dataplane does its work. + "--follow-forks" + "/bin/dataplane" ]; + Env = [ "HOME=/tmp" ]; }; }).overrideAttrs source-volatile; @@ -1006,6 +1158,7 @@ let # pkgs.wireshark-cli pkgs.bashInteractive + pkgs.bugstalker pkgs.coreutils pkgs.curl pkgs.debianutils diff --git a/justfile b/justfile index 6eed433ff8..45cb7d752b 100644 --- a/justfile +++ b/justfile @@ -114,7 +114,9 @@ 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_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 @@ -256,6 +258,339 @@ 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. +# 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) @@ -279,10 +614,20 @@ 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 }}" + ;; + "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 }}" + ;; + "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.) @@ -386,8 +731,14 @@ 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 }}" + ;; + "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!" @@ -421,7 +772,14 @@ push-container target="dataplane" *args: (build-container target args) && versio [script] push: {{ _just_debuggable_ }} - for container in dataplane 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 @@ -747,9 +1105,49 @@ 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 +# 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: 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/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"))] diff --git a/net/src/headers/embedded_view.rs b/net/src/headers/embedded_view.rs index fe6d5eba0e..f5aeee0ad6 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; @@ -1790,3 +1836,655 @@ 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 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 \ + 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 22fb5237ca..6514cfec17 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, Net, 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; @@ -1205,6 +1208,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 +1254,393 @@ 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. + if headers.net.is_some() { + 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) + } + + /// 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) + } + } + + /// [`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 + /// 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 { + // 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; + + // 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; @@ -1390,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/pat.rs b/net/src/headers/pat.rs index a5f640119d..d38a1f0436 100644 --- a/net/src/headers/pat.rs +++ b/net/src/headers/pat.rs @@ -2955,3 +2955,742 @@ 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)*) => { + // 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!() + .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); + let _ = 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:?}" + ); + }); + } +} diff --git a/net/src/headers/view.rs b/net/src/headers/view.rs index 516f60ff28..9f14ee8135 100644 --- a/net/src/headers/view.rs +++ b/net/src/headers/view.rs @@ -2575,3 +2575,682 @@ 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. +/// +/// # 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. +/// +/// Against a divergence in that chaining there are two independent guards, and both were demonstrated +/// by breaking the arity-3 arm on purpose: +/// +/// 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; + 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. +} + +/// 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::{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. + /// + /// 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. + /// + /// 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, $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($gen) + .for_each(|h: &Headers| { + SEEN.fetch_add(1, Ordering::Relaxed); + let licensed = h.as_view::().is_some(); + if licensed { + HIT.fetch_add(1, Ordering::Relaxed); + } + let chain = h.pat()$(.$layer())+.done(); + assert_eq!( + licensed, chain.is_some(), + concat!( + "`matches` and the read walk disagree for ", + 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!(($($layer),+)), + SEEN.load(Ordering::Relaxed), + HIT.load(Ordering::Relaxed), + ); + } + + #[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($gen) + .for_each(|h: &Headers| { + let mut owned = h.clone(); + SEEN.fetch_add(1, Ordering::Relaxed); + let licensed = owned.as_view_mut::().is_some(); + if licensed { + HIT.fetch_add(1, Ordering::Relaxed); + } + // 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, chain.is_some(), + concat!( + "`matches` and the mutable walk disagree for ", + 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!(($($layer),+)), + 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 eight, which is all of them. + // + // `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. + // + // `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, + SometimesHeadless, + eth, + vlan, + net, + transport + ); + arity_agrees!( + read_5, + mutable_5, + SometimesHeadless, + eth, + vlan, + vlan, + net, + transport + ); + arity_agrees!( + read_6, + mutable_6, + SometimesHeadless, + eth, + vlan, + vlan, + vlan, + net, + transport + ); + arity_agrees!( + read_7, + mutable_7, + SometimesHeadless, + eth, + vlan, + vlan, + vlan, + vlan, + net, + transport + ); + arity_agrees!( + read_8, + mutable_8, + SometimesHeadless, + 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, + SometimesHeadless, + eth, + ipv6, + hop_by_hop, + transport + ); + arity_agrees!( + read_ext_v6_two, + mutable_ext_v6_two, + SometimesHeadless, + eth, + ipv6, + hop_by_hop, + dest_opts, + transport + ); + arity_agrees!( + read_ext_v6_three, + mutable_ext_v6_three, + SometimesHeadless, + 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, + SometimesHeadless, + 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, + 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 + /// 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, + ); +} diff --git a/net/src/headers/within.rs b/net/src/headers/within.rs index 2fc8646360..c2e2240537 100644 --- a/net/src/headers/within.rs +++ b/net/src/headers/within.rs @@ -554,3 +554,357 @@ 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::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; + 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); + // 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) { + 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),+ $(,)?) => { + 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| { + 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); + $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. + 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); + + // 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); + } +} diff --git a/nix/overlays/dataplane-dev.nix b/nix/overlays/dataplane-dev.nix index c8829018f2..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 // { @@ -21,9 +27,48 @@ 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="; + }; + }); + # 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; }; + 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. @@ -43,15 +88,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 = ""; + }); } 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" 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 369e701a56..722332c937 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": { @@ -71,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": { 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})" diff --git a/scripts/spec-interlock.ts b/scripts/spec-interlock.ts new file mode 100755 index 0000000000..cf4dc07323 --- /dev/null +++ b/scripts/spec-interlock.ts @@ -0,0 +1,730 @@ +#!/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 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.", + }, + // 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. */ +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; + 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[], + 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(); + 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; +} + +/** + * 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" + | "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[]; +} + +/** Mutate the cited implementation, run only the cited tests, and report survivors. */ +async function runTriple( + triple: Triple, + output: string, + jobs: number, + used: Set, +): 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(); + + // 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", + 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, 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. + 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, + }; + } + // 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, + }; +} + +/** `--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 }); + 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), used); + 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(", ")}`, + ); + 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}`); + } + // 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)`); + } + // 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(); + console.log( + `${triples.length - failures}/${triples.length} requirements hold their citations`, + ); + return failures ? 1 : 0; +} + +if (import.meta.main) Deno.exit(await main());