Repository navigation
Fix 10 correctness defects found by SME review of the v0.3.1 binary - #1
Merged
Merged
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
scannow exits 2 for an empty capture and 3 when nothing was recognized, where both previously exited 0. PerCHANGELOG.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.-qand 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-filewrote 0 bytes until the clean quit pathThe 8 KiB
BufWriterwas flushed only onDrop. 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):
SIGTERM and SIGHUP now also exit cleanly instead of killing the process where it stands:
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 ~10write(2)calls a second at 115200 baud; the buffer is kept so one serial read is still one syscall. A newterm/signals.rsinstalls 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_dropasserts the final contents, which passes happily against the broken code. The four new tests assert the file is non-zero mid-session, with theLogFilestill alive. Verified they catch the regression by reverting just the flush line:2 — An empty capture passed
--gate-criticalwith exit 0Non-empty but unrecognized is a distinct answer:
Fix. Adopt the legacy Node analyzer's ladder (2 = empty/unusable, 3 = nothing recognized), ordered as the Node analyzer orders it.
--gate-criticalstill 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 workedcase $?dispatch. The bundled GitHub Action now emits a::errorannotation explaining 2 and 3, and exposes ananalysis-statusoutput.3 — The Bootloader detector was line-anchored, so any prefix killed it
Self-inflicted:
bootintel analyze --log-timestampsprefixes every line with an ISO-8601 timestamp, so the tool's own capture mode broke its ownscan.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-timestampsover 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: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-filewrites raw bytes,analyze --log-filefollowed byscancould fail on the tool's own output.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::inputmodule, counting replaced bytes so-vcan report them. Line numbers are preserved because\nand\rare single-byte ASCII and can never be part of an invalid UTF-8 sequence — asserted by a test. The conversion is verified identical toString::from_utf8_lossyacross five byte patterns.5 — SIGPIPE gave three different exit codes for one command
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 throughserde_json::to_writer_pretty, whoseserde_json::Errordoes not downcast toio::Errorat all. That is what produced the observed message. The check now walks the wholeanyhowcause chain and usesserde_json::Error::io_error_kind(), then exits 0 silently.Chose the "map BrokenPipe to a silent exit 0" option over restoring
SIG_DFLbecause 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_DFLwould instead give a deterministic 141-by-signal, which is whatcat/yesdo. One consequence of exit 0 to be aware of —scan --gate-critical | head -1whereheadcloses 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
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, andstdinfor piped input.startLinenow comes from the detector library's recordedline_numberrather than aline.contains(source)substring search, with the old search kept as a fallback for findings rehydrated from an older archived envelope.7 —
-qdid not quiet, and the upsell printed to stdoutscan --format text > report.txtwas shipping a three-line advertisement inside a customer's report, and-qdid 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 cveonly worked from inside the source repoFix. Resolve from the platform state dir (
~/.local/state/bootintel/and equivalents — the same dirhistoryalready uses), populated by a newbootintel cve --refreshthat fetches the public feed.--refreshparses 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/--refreshhelp text now state plainly where the feed lives rather than implying it is embedded.9 — Smaller correctness items
portssorts 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, sottyS9precedesttyS10.scan/watchfor CI/cron/nohup, orscript -qec/ssh -ttto allocate a TTY.historystays 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 breakingbootintel historyfor existing users, but happy to flip it if you would rather.10 — Free supply-chain hardening
actions/attest-build-provenance@v2on every release archive (per matrix leg, on the runner that built it) and on the container image, withid-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.SHA256SUMSis unchanged. Release notes and the README now tell users how to verify:Compatibility
No existing JSON envelope key was renamed, removed or repurposed. Two keys were added:
analysis_statusmatched/unrecognizedline_numbersource, omitted when unknownbootintel schemais updated to match — it declaresadditionalProperties: 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:
sourcenow 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:
bootintel_clibootintel_versionsource(the input path)critical_hitsfindings+CRITICAL_LABELS)analysis_source,detector_countRenaming either side breaks existing consumers, so it needs a deliberate decision and probably a major version. Flagging it rather than fixing it.
analysis_statusis 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 andtuifeatures.cargo fmt --all --check— clean.crates/cli/tests/cli_regressions.rsdriving the compiled binary, 13 detector fixtures, 4 mid-session log-file tests, 4 lossy-UTF-8 unit tests, 2 signal tests.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 --refreshagainst the live endpoint. This sandbox intercepts TLS, so the fetch fails withinvalid peer certificate: UnknownIssuer. Confirmed environmental:curl https://bootintel.com/feed/embedded-cves.jsonfails identically here. The resolution, cache-path and parse-before-write logic were verified by seeding the cache and runningbootintel cvefrom an unrelated working directory. Please runbootintel cve --refreshonce on a normal network before merging.workflow_dispatch-only and not run. YAML validated by parsing; step wiring reviewed against the action's documented inputs (subject-pathfor files,subject-name+subject-digest+push-to-registryfor the image, withid: docker_buildadded so the digest output exists).Incidental
cargo fmt --allalso 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) —mainwas not fmt-clean, and CI runscargo fmt --all --checkwithRUSTFLAGS: -D warnings. Kept so this branch passes that gate; happy to split it out.🤖 Generated with Claude Code