audit: blocking operations with no timeout in fbuild-core + foundational crates (sub-issue of #802)

[zackees]

Intro

Sub-issue of #802. This audit covers the foundational crates that everything else depends on: fbuild-core, fbuild-paths, fbuild-config, fbuild-header-scan, and fbuild-library-select. The single highest-leverage finding is in fbuild-core/src/subprocess.rs::run_command, which is the central choke point every fbuild subprocess flows through. That helper accepts timeout: Option<Duration> and does the right thing when given a value (process.wait(timeout) then kill on ProcessError::Timeout then return FbuildError::Timeout). The problem is that the default is no timeout, and a non-trivial number of call sites across the workspace pass None (a quick grep finds 34+ such call sites across 20 files in the higher-level crates). That means a single misbehaving compiler, linker, esptool, avrdude, addr2line, etc. invocation can wedge the daemon thread that spawned it for as long as the child decides to live.

Everything else found in this audit is in line with the workspace's design: ini extends resolution has cycle detection (visited set) and max_depth = 10 for variable substitution; the header-scan walker is a BFS over a visited set; symbol-analysis loops are bounded by BTreeMap::range walks. Synchronization usage in these crates is conservative (OnceLock, short RwLock/Mutex critical sections). The notable secondary finding is install_status::publish_install_status, which invokes subscriber callbacks synchronously on the publisher thread — a slow / panicking subscriber back-presses the publisher (low severity in practice today because the only registered subscriber is the daemon's own short broadcast hop, but it's still a design contract worth documenting and ideally guarding).

Findings

Severity	Crate	File:line	Operation	Why unbounded	Suggested fix
HIGH	fbuild-core	src/subprocess.rs:81-89 (run_command), :151-174 (run_command_passthrough), :100-144 (run_command_with_stdin)	process.wait(timeout) with timeout: Option<Duration> defaulting to None	Helper itself is correct when given a Some(Duration), but the API makes timeout opt-in. 34+ call sites across fbuild-build, fbuild-deploy, fbuild-serial, fbuild-packages, fbuild-daemon, etc. pass None. A hung compiler / linker / esptool / avrdude blocks the daemon-side worker thread indefinitely.	Pick a sensible default cap (e.g. Duration::from_secs(15 * 60) per the existing #802 discussion) inside run_command* when the caller passes None. Keep Some(Duration) as explicit-override. Tag the few legitimately-unbounded cases (interactive pio passthrough, long QEMU runs) with a named constant or a run_command_no_timeout opt-in so they are auditable.
MEDIUM	fbuild-core	src/install_status.rs:82-93 (publish_install_status)	subscriber(status) called synchronously while still on the publisher's thread	The subscriber is a Arc<dyn Fn(...) + Send + Sync> invoked directly. A subscriber that takes a lock, does I/O, or blocks on a channel send will hold up every code path that calls publish_install_status — including the toolchain installer's hot path. There is no isolation or per-call deadline.	Either (a) document the "subscriber MUST be non-blocking" contract explicitly in the doc comment and add a debug_assert! / panic catcher around the call, or (b) wrap the call in a tokio::task::spawn_blocking / dedicated dispatcher thread so a slow subscriber cannot back-pressure publishers.
LOW	fbuild-core	src/usb/data.rs:225-228 (install_online_cache_map), :231-235 (lookup)	ONLINE_MAP.write().unwrap() / .read().unwrap() on a RwLock<Option<HashMap<...>>>	Lock is held for the duration of a *guard = Some(map) write or a .get(&pack(vid, pid)).cloned() read. Both are short and non-recursive, so contention is fine. Only listed for completeness — flag as LOW because no realistic path holds the read guard across an install.	None — acceptable as written. Consider switching to arc_swap::ArcSwap if write-while-reading ever becomes a measured concern.
LOW	fbuild-core	src/symbol_analysis/mod.rs:880 (nm_range_covers), src/symbol_analysis/graph/mod.rs:441 (BFS)	while let Some(...) walking BTreeMap::range / VecDeque	Bounded by the size of the input map / graph. ELF symbol tables are O(thousands) — no risk in practice.	None.
LOW	fbuild-config	src/ini_parser/parser.rs:111-166 (resolve_env), :168-210 (resolve_section), :218-237 (substitute_vars)	Recursive extends resolution + ${section.key} variable substitution	Cycle detection via visited: HashSet<String>; variable substitution capped at max_depth = 10. Already correctly hardened.	None.
LOW	fbuild-config	src/board/db.rs:139, src/board/loaders.rs:122, src/ini_parser/mod.rs:60, src/sdkconfig.rs:103,109	std::fs::read_to_string(path)	Reads whole file into memory. Bounded by filesystem (.ini, board JSON, sdkconfig — KB-range in practice) but unbounded in principle.	None for the small-config files. If a malicious / corrupt input file is a worry, consider a read_to_string_with_cap(path, MAX_BYTES) wrapper used uniformly.
LOW	fbuild-header-scan	src/walker.rs:139 (fs::read_to_string inside the rayon par_iter)	Reads each header without size cap	Walker BFS itself is correctly bounded by the visited set (cycles, diamonds, depth-N chains tested in tests::w10_cycle_terminates / w11_diamond_dedupes). Per-file read_to_string is unbounded in principle but headers are KB-range in practice.	None. Same caveat as the read_to_string row above.
LOW	fbuild-library-select	src/cache.rs:235-239 (cache_key seed read), :289 (canonical_header_hash)	std::fs::read(&canon) of seed sources / canonical headers for blake3 hashing	Reads whole file into memory to hash. Bounded by source file size in practice. No locks.	None.
LOW	fbuild-paths	src/running_process.rs:339,358,382 (ENV_LOCK.lock().unwrap())	Test-only mutex	Only in #[cfg(test)] paths.	None — test-only.

What was searched

Patterns grepped across crates/{fbuild-core,fbuild-paths,fbuild-config,fbuild-header-scan,fbuild-library-select}/src/**/*.rs:

• Command::new / .output() / .wait() / .wait_with_output() (subprocess spawn + blocking wait)
• .lock() / .read() / .write() (Mutex/RwLock guards)
• .recv() / recv_timeout (mpsc receivers)
• loop { / while (unbounded iteration / recursion)
• thread::sleep
• fs::read_to_string / fs::read (file reads on potentially-large artifacts)
• RwLock / Mutex / OnceLock / sync:: (synchronization primitives)
• subscriber / callback / on_status (event-bus patterns)
• extends / recursive / depth (config parser cycle / recursion controls)

Files read in full for context:

• crates/fbuild-core/src/subprocess.rs (the central concern)
• crates/fbuild-core/src/containment.rs
• crates/fbuild-core/src/install_status.rs
• crates/fbuild-core/src/response_file.rs
• crates/fbuild-core/src/build_log.rs
• crates/fbuild-core/src/usb/data.rs (excerpt)
• crates/fbuild-config/src/ini_parser/parser.rs
• crates/fbuild-config/src/ini_parser/variables.rs
• crates/fbuild-header-scan/src/walker.rs
• crates/fbuild-library-select/src/cache.rs

Out-of-scope notes

• Test code excluded. containment.rs tests use child.wait() directly on a hand-built Command, and running_process.rs tests use ENV_LOCK.lock().unwrap() to serialize env-var fiddling — both are #[cfg(test)] only and do not ship.
• Drop impls excluded. None found in the audited surface that block on subprocess termination.
• Higher-level orchestrator timeouts excluded. Whether fbuild-build, fbuild-deploy, fbuild-daemon etc. layer their own timeouts on top is a separate parallel audit (per audit: blocking operations with no timeout (meta) #802); this issue only covers what the foundational crates expose and enforce themselves.
• std::fs::read{,_to_string} everywhere. Treated as LOW because in practice these files are KB-range config / source headers. A workspace-wide "cap reads at N MB" wrapper would close the principled hole but is not load-bearing for any realistic build.
• The 34+ run_command(..., None) call sites themselves. Listed quantitatively but enumerating each is the job of the higher-level crate audits, not this one — the fix lands in subprocess.rs as a default cap so every existing call site inherits the safety net for free.

Generated with Claude Code

[zackees]

Closed by PR #843 (workspace-wide timeout sweep).