Skip to content

Fix 10 correctness defects found by SME review of the v0.3.1 binary - #1

Merged
Zenofex merged 2 commits into
mainfrom
fix/cli-correctness-defects
Sep 24, 2026
Merged

Zenofex merged 2 commits into
mainfrom
fix/cli-correctness-defects

Conversation

@Zenofex

@Zenofex Zenofex commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Ten correctness defects found by an SME who installed the real v0.3.1 release binary and exercised it against socat PTY pairs. Every one was reproduced by running before being fixed, and the before/after output is pasted below.

Do not merge without review — flagging two behaviour changes up front:

  • Exit-code policy changes. scan now exits 2 for an empty capture and 3 when nothing was recognized, where both previously exited 0. Per CHANGELOG.md's own versioning policy an exit-code policy change is MAJOR. This is the point of item 2, but it will fail CI jobs that were previously (wrongly) green.
  • -q and the text summary block. The upsell moved from stdout to stderr. Anything scraping stdout for the "N findings identified locally" line will stop seeing it.

1 — CRITICAL: --log-file wrote 0 bytes until the clean quit path

The 8 KiB BufWriter was flushed only on Drop. A capture smaller than the buffer — which is most of them, since a boot log is a short burst followed by an idle console — existed only in process memory. Ctrl-C, a closed terminal window, an unplugged adapter or a sleeping laptop lost the entire capture, while the on-screen analysis showed the bytes had been received and understood. A 0-byte file is the worst possible outcome for a tool whose job is not losing your capture.

Reproduced with a socat PTY pair, a second PTY for the controlling terminal, and a realistic trickle feed (~46 B every 400 ms):

BEFORE                                  AFTER
  t= 2s  logsize=0                        t= 2s  logsize=235
  t= 4s  logsize=0                        t= 4s  logsize=470
  t= 6s  logsize=0                        t= 6s  logsize=705
  t= 8s  logsize=0                        t= 8s  logsize=940
  t=10s  logsize=0                        t=10s  logsize=1175
  after SIGINT   logsize=0                after SIGINT   logsize=1175
  tail -c 60 mid-session: ''              tail -c 60 mid-session:
                                            '8:21 +0000)U-Boot 2020.10 (Sep 17 ...'

SIGTERM and SIGHUP now also exit cleanly instead of killing the process where it stands:

AFTER, kill -TERM:                      AFTER, kill -HUP:
  pre-TERM  logsize=376                   pre-HUP  logsize=376
  exited cleanly on TERM                  exited cleanly on HUP
  post-TERM logsize=376                   post-HUP logsize=376
  [bootintel] session ended —             [bootintel] session ended —
    received SIGTERM                        received SIGHUP (terminal closed)
  [bootintel] capture saved —             [bootintel] capture saved —
    376 bytes in /tmp/.../cap2.log          376 bytes in /tmp/.../cap2.log

Fix. Flush after every write, so the bytes are in the kernel before the next read. That also makes the file tail -f-able from a second terminal, which is how people actually work. Cost is ~10 write(2) calls a second at 115200 baud; the buffer is kept so one serial read is still one syscall. A new term/signals.rs installs SIGINT/SIGTERM/SIGHUP handlers that do the only async-signal-safe thing available — set an atomic — and the terminal loop (which already polls on a 100 ms timeout) breaks out through its normal teardown, so raw mode is restored and the log is flushed.

Why the existing tests missed it. writes_bytes_and_flushes_on_drop asserts the final contents, which passes happily against the broken code. The four new tests assert the file is non-zero mid-session, with the LogFile still alive. Verified they catch the regression by reverting just the flush line:

test writes_bytes_and_flushes_on_drop ... ok          <- pre-existing, passes broken
test bytes_are_on_disk_mid_session_not_just_at_drop ... FAILED
test a_second_reader_can_tail_the_file_while_the_session_runs ... FAILED
test nothing_is_lost_when_the_process_never_drops_the_logfile ... FAILED
test timestamped_writes_are_also_flushed_mid_session ... FAILED

2 — An empty capture passed --gate-critical with exit 0

BEFORE                                  AFTER
$ bootintel scan empty.log \            $ bootintel scan empty.log \
    --gate-critical --format json           --gate-critical --format json
{                                       bootintel: empty capture; no inspection
  "bootintel_version": "0.3.1",           performed (empty.log)
  "analysis_source": "client",            Nothing was analyzed, so this is not a
  "detector_count": 9,                    passing scan.
  "findings": []                          Check that the serial capture actually
}                                         ran, that the adapter is still attached,
exit=0                                    and that the path is the one your
                                          capture step wrote.
                                        exit=2

Non-empty but unrecognized is a distinct answer:

AFTER
$ bootintel scan noise.log --format json
{ ... "findings": [], "analysis_status": "unrecognized" }
bootintel: no recognized evidence in noise.log; this is not a successful inspection
  The capture has content but matched none of the 9 detectors. Common causes: the
  capture started after the boot banner scrolled past, or the baud rate was wrong.
exit=3

Fix. Adopt the legacy Node analyzer's ladder (2 = empty/unusable, 3 = nothing recognized), ordered as the Node analyzer orders it. --gate-critical still exits 1 on a real critical finding — covered by a test, since the new ladder must not swallow the code the gate already had. Documented in the README exit table beside the existing sysexits codes, with a worked case $? dispatch. The bundled GitHub Action now emits a ::error annotation explaining 2 and 3, and exposes an analysis-status output.

3 — The Bootloader detector was line-anchored, so any prefix killed it

Self-inflicted: bootintel analyze --log-timestamps prefixes every line with an ISO-8601 timestamp, so the tool's own capture mode broke its own scan.

findings for `U-Boot 2020.10 (Sep 17 2023 - 11:38:21 +0000)`:

                                        BEFORE   AFTER
  bare line                             1        1
  [12:34:56.789] prefix                 0        1
  [2026-09-23T10:00:00.000Z] prefix     0        1
  ANSI \033[32m ... \033[0m             0        1

Fix. Port the Node analyzer's normalization (ANSI CSI stripper plus bracketed-timestamp stripper) into crates/detectors/src/lib.rs, applied before matching. The timestamp strip loops, because --log-timestamps over a Linux console stacks two prefixes ([2026-…Z] [ 0.000000] …) and the Node analyzer's single-shot strip leaves the second in place. Evidence still reports the original line verbatim, so the user can find it again in their log:

AFTER
$ bootintel scan p2.log --format json
"source": "[2026-09-23T10:00:00.000Z] U-Boot 2020.10 (Sep 17 2023 - 11:38:21 +0000)",
"line_number": 1

13 fixtures added, covering all four prefix shapes plus stacked prefixes, ANSI-and-timestamp together, the other anchored detectors (coreboot, GRUB, procd), bare-CR line endings, and a negative case asserting a non-timestamp bracket is not stripped.

4 — Any non-UTF-8 byte was a hard error

This is the normal shape of a real UART capture: bytes received before the baud locks, framing errors, a binary splash, a reset mid-line. And since --log-file writes raw bytes, analyze --log-file followed by scan could fail on the tool's own output.

BEFORE                                  AFTER
$ bootintel scan bin.log --format json  $ bootintel scan bin.log --format json -v
Error: reading /tmp/bi-evid/bin.log     [v] bin.log: 6 byte(s) were not valid UTF-8
                                            and were replaced with U+FFFD (normal for
Caused by:                                  a UART capture: pre-baud-lock noise,
    stream did not contain valid UTF-8      framing errors, a binary splash)
exit=1                                  { "findings": [ Bootloader U-Boot 2020.10 (line 1),
                                                        Autoboot interruptable (line 3) ],
                                          "analysis_status": "matched" }
                                        exit=0

That is byte-for-byte the same two findings, with the same line numbers, that the legacy Node analyzer returns for the same file.

Fix. Read bytes and decode lossily in a new crate::input module, counting replaced bytes so -v can report them. Line numbers are preserved because \n and \r are single-byte ASCII and can never be part of an invalid UTF-8 sequence — asserted by a test. The conversion is verified identical to String::from_utf8_lossy across five byte patterns.

5 — SIGPIPE gave three different exit codes for one command

BEFORE                                          AFTER
$ bootintel batch samples --format json | head -2
run1=0 run2=0 run3=0 run4=1 run5=1 run6=0       run1=0 ... run6=0

$ bootintel manpage | head -1
run1=0 run2=141 run3=141                        run1=0 ... run6=0

stderr on the exit-1 runs:                      stderr: (empty)
  Error: Broken pipe (os error 32)

Fix. The old check was err.downcast_ref::<std::io::Error>() on the outermost error only, so a single .context(...) hid it — and every JSON/SARIF path goes through serde_json::to_writer_pretty, whose serde_json::Error does not downcast to io::Error at all. That is what produced the observed message. The check now walks the whole anyhow cause chain and uses serde_json::Error::io_error_kind(), then exits 0 silently.

Chose the "map BrokenPipe to a silent exit 0" option over restoring SIG_DFL because it behaves identically on Windows, where there is no SIGPIPE to restore, and because it is the "conventional silent success" the report asked for. Worth a reviewer's opinion: SIG_DFL would instead give a deterministic 141-by-signal, which is what cat/yes do. One consequence of exit 0 to be aware of — scan --gate-critical | head -1 where head closes the pipe first now reports 0 rather than the gate's verdict. That hazard is inherent to any Unix filter, but it is adjacent to item 2's theme, so say the word if you want 141 instead.

6 — SARIF hardcoded the artifact path

BEFORE                                  AFTER
"uri": "boot.log"    (always)           "uri": "samples/bootintel.txt"   (in-workspace, relative)
                                        "uri": "stdin"                   (piped input)
                                        "uri": "/tmp/bi-evid/abs.txt"    (outside the workspace)

Fix. Emit the real input path, made relative to $GITHUB_WORKSPACE (falling back to the working directory) when it lives underneath, with forward slashes on every platform, and stdin for piped input. startLine now comes from the detector library's recorded line_number rather than a line.contains(source) substring search, with the old search kept as a fallback for findings rehydrated from an older archived envelope.

7 — -q did not quiet, and the upsell printed to stdout

BEFORE                                          AFTER
$ scan --format text -q 2>/dev/null | tail -4   $ scan --format text -q 2>/dev/null | tail -4
  3 findings identified locally ...               ...  Kernel  Linux 2.6.35.3-flex-dvt
  For CVE matches + exploit paths + AI report:    ...  Device family  Buildroot
    bootintel scan --api <log>            ...
    bootintel scan --api --preview <log>  ...   (findings only — no marketing)

scan --format text > report.txt was shipping a three-line advertisement inside a customer's report, and -q did nothing despite its own help text promising it suppresses banners and status hints. The block now goes to stderr unconditionally and is suppressed entirely by -q; stdout carries findings only.

8 — bootintel cve only worked from inside the source repo

BEFORE                                  AFTER
$ cd /tmp && bootintel cve              $ cd /usr && bootintel cve
Error: no CVE feed file found. Tried    feed: 1 entries, generated 2026-09-23T08:02:28
  $BOOTINTEL_CVE_FEED,                    (window 168h, severity floor MEDIUM)
  ./data/embedded-cves-feed.json,         MEDIUM  CVE-2026-54551  WireGuard  ...
  /data/embedded-cves-feed.json.        exit=0
exit=1

Fix. Resolve from the platform state dir (~/.local/state/bootintel/ and equivalents — the same dir history already uses), populated by a new bootintel cve --refresh that fetches the public feed. --refresh parses before it writes, so a captive-portal page or a truncated transfer cannot replace a good cached copy with junk. Repo-relative paths are still tried last, so working in a checkout behaves as before. Module docs and --feed/--refresh help text now state plainly where the feed lives rather than implying it is embedded.

9 — Smaller correctness items

BEFORE                                          AFTER
$ bootintel analyze /etc/hostname               $ bootintel analyze /etc/hostname
Error: permission denied opening /etc/hostname  Error: /etc/hostname is a regular file,
                                                  not a serial device
  On Linux you typically need to be in the
  `dialout` group (or `uucp` on Arch/RHEL):       This subcommand opens a UART and reads it
    sudo usermod -aG dialout $USER                live; it needs a character device such as
  Then log out and back in ...                    /dev/ttyUSB0 or /dev/ttyACM0.
                                                  Run `bootintel ports` to list what's here.
(wrong diagnosis: the open fails because a
 regular file has no termios state, not          To analyze a log you already have on disk:
 because of permissions)                           bootintel scan /etc/hostname
                                                   bootintel watch /etc/hostname
  • ports sorts USB adapters first, separates them from the built-in ports with a blank line, pads the name column, always shows a product string (falling back to (USB serial adapter)), and labels the /dev/ttyS* stubs (built-in / no USB descriptor) rather than leaving 32 bare paths in enumeration order. Sorting is natural, so ttyS9 precedes ttyS10.
  • The non-TTY error — previously the only error in the CLI with no recovery hint — now explains that the subcommand is interactive and offers scan/watch for CI/cron/nohup, or script -qec / ssh -tt to allocate a TTY.
  • history stays on by default but is now disclosed: one stderr notice, printed only when the file is first created, naming the path, what it records, that it never leaves the machine, and both ways to turn it off. Chose disclosure over making it opt-in to avoid breaking bootintel history for existing users, but happy to flip it if you would rather.

10 — Free supply-chain hardening

actions/attest-build-provenance@v2 on every release archive (per matrix leg, on the runner that built it) and on the container image, with id-token: write + attestations: write. Keyless, free for public repos, and a stronger claim than a paid signing certificate: it binds the artifact to a build, not merely to an organisation that paid a CA. Skipped on dry runs so nothing misleading lands in the public transparency log. SHA256SUMS is unchanged. Release notes and the README now tell users how to verify:

gh attestation verify bootintel-v0.3.1-x86_64-linux.tar.gz --repo BootIntel/cli
gh attestation verify oci://ghcr.io/bootintel/cli:latest --repo BootIntel/cli

Compatibility

No existing JSON envelope key was renamed, removed or repurposed. Two keys were added:

Field Where Meaning
analysis_status envelope matched / unrecognized
line_number each finding 1-based line of source, omitted when unknown

bootintel schema is updated to match — it declares additionalProperties: false, so without that update a strict validator would have started rejecting our own output. Cross-checked programmatically that the emitted envelope and the emitted schema have identical key sets. A test asserts the pre-existing keys are all still present.

One value did change meaning: source now carries the original log line rather than only the substring the regex matched. That is required by item 3 (evidence must show the prefix the user actually has) and matches the Node analyzer.

Known divergence from the Node analyzer (not addressed)

The two implementations still disagree on envelope keys, and this PR deliberately does not reconcile them:

Node Rust
bootintel_cli bootintel_version
source (the input path) — (not emitted; SARIF carries it)
critical_hits — (derivable from findings + CRITICAL_LABELS)
— analysis_source, detector_count

Renaming either side breaks existing consumers, so it needs a deliberate decision and probably a major version. Flagging it rather than fixing it. analysis_status is now common to both, which was the compatibility addition worth making.

Verification

  • cargo test --workspace — 274 passed, 0 failed. Same on --features tui.
  • cargo clippy --all-targets -- -D warnings — clean, on both default and tui features.
  • cargo fmt --all --check — clean.
  • Regression tests for each of items 1–6 (plus 7 and 9): 26 in a new crates/cli/tests/cli_regressions.rs driving the compiled binary, 13 detector fixtures, 4 mid-session log-file tests, 4 lossy-UTF-8 unit tests, 2 signal tests.
  • Smoke-tested every subcommand (version, detectors, schema, demo, batch, diff, export, share, encode-share, bench, doctor, completions, manpage, config, view) and every output format (json, text, sarif, junit, csv, html, md) — all behave as before.

Not verified

  • bootintel cve --refresh against the live endpoint. This sandbox intercepts TLS, so the fetch fails with invalid peer certificate: UnknownIssuer. Confirmed environmental: curl https://bootintel.com/feed/embedded-cves.json fails identically here. The resolution, cache-path and parse-before-write logic were verified by seeding the cache and running bootintel cve from an unrelated working directory. Please run bootintel cve --refresh once on a normal network before merging.
  • The release workflow's attestation steps. workflow_dispatch-only and not run. YAML validated by parsing; step wiring reviewed against the action's documented inputs (subject-path for files, subject-name + subject-digest + push-to-registry for the image, with id: docker_build added so the digest output exists).
  • Windows behaviour. No Windows runner available. The signal module compiles to a no-op there with a comment explaining why; the broken-pipe fix was written to be platform-independent for this reason.

Incidental

cargo fmt --all also reformatted 16 lines across five files I did not otherwise touch (cmd/diff.rs, cmd/init.rs, cmd/watch.rs, cmd/whoami.rs, config.rs) — main was not fmt-clean, and CI runs cargo fmt --all --check with RUSTFLAGS: -D warnings. Kept so this branch passes that gate; happy to split it out.

🤖 Generated with Claude Code

Zenofex and others added 2 commits September 23, 2026 10:54
Ten defects, all reproduced by running the released binary against
socat PTY pairs rather than by reading the source.

Severity order:

1. --log-file wrote 0 bytes unless the session exited via the clean
   quit path. The 8 KiB buffer was flushed only on Drop, so a capture
   smaller than that — most of them — existed only in process memory.
   Ctrl-C, a closed terminal, an unplugged adapter or a sleeping laptop
   lost the whole capture while the on-screen analysis showed the bytes
   had been received. Flush per write; add SIGINT/SIGTERM/SIGHUP
   handlers that exit through the normal path so raw mode is restored
   and the file is flushed. The file is now tailable live.

2. An empty capture passed --gate-critical with exit 0, so a CI job
   whose UART never came up reported green. Adopt the Node analyzer's
   ladder: 2 for empty/unusable input, 3 for non-empty-but-unrecognized.
   Add analysis_status to the JSON envelope; document both in the README
   exit table and in the bundled GitHub Action.

3. The Bootloader detector was line-anchored, so any prefix killed it —
   including the ISO-8601 timestamp that `analyze --log-timestamps`
   writes, which meant the tool's own capture mode broke its own scan.
   Port the Node analyzer's ANSI + bracketed-timestamp normalization
   into the detector library, applied before matching, with evidence
   still reporting the original line.

4. Any non-UTF-8 byte was a hard error, on captures the Node analyzer
   reads fine — and since --log-file writes raw bytes, scan could fail
   on the tool's own output. Read bytes and decode lossily; report the
   replaced-byte count under -v. Line numbers are preserved.

5. A closed pipe gave three different exit codes for one command. The
   broken-pipe check only inspected the outermost error, so anything
   behind a .context() or inside a serde_json::Error escaped it. Walk
   the cause chain, understand serde_json, and exit 0 silently.

6. SARIF hardcoded artifactLocation.uri = "boot.log", so GitHub
   attached findings to a file not in the repository and every
   annotation landed nowhere. Emit the real path, workspace-relative
   where possible, and stdin for piped input.

7. -q did not quiet and the upsell printed to stdout, so
   `scan --format text > report.txt` shipped marketing inside a
   customer's report. Route the summary block to stderr and honour -q.

8. `bootintel cve` resolved the feed relative to the working directory,
   so it only worked from inside a source checkout. Resolve from the
   platform state dir with a documented --refresh.

9. Smaller items: a readable regular file was diagnosed as a permissions
   problem with dialout-group advice; `ports` returned 32 unsorted bare
   paths with no way to spot the USB adapter; the non-TTY error had no
   recovery hint; the local scan history was undisclosed.

10. No artifact was signed. Enable keyless build-provenance attestations
    for release archives and the container image. SHA256SUMS unchanged.

No existing JSON envelope key was renamed or removed; analysis_status
and line_number are additions, and `bootintel schema` is updated to
match so strict validators keep passing.

Tests: 274 pass. Each of items 1-6 has a regression test. The four
mid-session log-file tests fail against the unfixed code while the
pre-existing flush-on-drop test passes, which is why the bug survived.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SHUTDOWN_SIGNAL was declared unconditionally, but its only reader is the
cfg(unix) arm of shutdown_reason and its only writers are the cfg(unix)
handler and reset_for_test. A Windows release build has neither, so the
static is dead code, and this crate denies warnings.

The result was that release build (windows-latest) went from green on main
to a hard failure on this branch, while ubuntu, macos, MSRV and all three
test matrices stayed green. Windows is one of the five published platform
binaries, so this would have shipped a gap.

Gated on cfg(any(unix, test)): present wherever something reads or writes
it, absent in the one configuration where nothing does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Zenofex
Zenofex merged commit 22195c4 into main Sep 24, 2026
10 of 11 checks passed
@Zenofex
Zenofex deleted the fix/cli-correctness-defects branch September 24, 2026 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant