buzz-terminal: shell lookup uses MT-unsafe getpwuid static storage (pre-existing; env_fence CI failure in run 34631406730)
- Dominant language
- Rust
- Stars
- 32.7k
- Forks
- 4.3k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 253
Description
**Record of a pre-existing production defect** in `buzz-terminal`'s Unix shell lookup, surfaced by a hosted CI failure. It is **not a regression from PR #7192** (that PR changes no `buzz-terminal`/terminal-path files); recording it separately so the residual race stays visible regardless of CI rerun outcomes.
## Unsafe code (exact-head permalink)
[`desktop/src-tauri/crates/buzz-terminal/src/shell.rs` L103–L116 @ `63a1c47a`](https://github.com/block/buzz/blob/63a1c47aa42cc50509c3164c1740c018ee3f22f1/desktop/src-tauri/crates/buzz-terminal/src/shell.rs#L103-L116)
`passwd_shell()` calls non-reentrant `getpwuid()` and copies `pw_shell` out of the libc-owned **static** `passwd` struct. The SAFETY comment promises no intervening call *in the current thread*, but Linux documents `getpwuid` as **MT-Unsafe `race:pwuid`**: the returned storage may be overwritten by a passwd-database call from **any** thread. glibc's `getXXbyYY.c` unlocks its internal NSS lock *before* returning the pointer, so a concurrent `getpwuid*`/`getpwnam*` call can rewrite the struct between libc's return and our copy — the copy itself is the race window. `resolve_shell` also evaluates `passwd_shell()` eagerly even when an explicit valid shell was passed ([L82](https://github.com/block/buzz/blob/63a1c47aa42cc50509c3164c1740c018ee3f22f1/desktop/src-tauri/crates/buzz-terminal/src/shell.rs#L82)), so every concurrent resolver use exercises the unsafe lifetime. Production caller: [`desktop/src-tauri/src/terminal_runtime.rs:421`](https://github.com/block/buzz/blob/63a1c47aa42cc50509c3164c1740c018ee3f22f1/desktop/src-tauri/src/terminal_runtime.rs#L421).
This code is **unchanged** between integration base `7f8c2e5c` and head `63a1c47a` (`git diff --exit-code base head -- desktop/src-tauri` is clean; the PR diff is 11 Welcome/E2E files) — the defect predates that PR's diff.
## Observed CI failure (attempt 1, preserved)
- Run: https://github.com/block/buzz/actions/runs/34631406730 (attempt 1) — job [Desktop Core `103369033534`](https://github.com/block/buzz/actions/runs/34631406730/job/103369033534), Ubuntu 24.04, step **"Desktop Tauri compiled-flag verification"** (`just desktop-tauri-test-compiled-flags` → final `BUZZ_BUILD_DEMO_SLUG=… cargo test --workspace` sweep, exit 101).
- Earlier **in the same job**, the same displayed test binary `deps/buzz_terminal-bd20f73363094eae` passed all **27/27** library tests; the final workspace sweep then passed **26/27**.
- Failing test: `env_fence_tests::resolve_shell_falls_through_to_passwd_not_the_default`, panic at [`crates/buzz-terminal/src/env_fence_tests.rs:237`](https://github.com/block/buzz/blob/63a1c47aa42cc50509c3164c1740c018ee3f22f1/desktop/src-tauri/crates/buzz-terminal/src/env_fence_tests.rs#L237) — `assertion left == right failed: an unset $SHELL did not resolve to the passwd entry — left: "/bin/bash", right: "/back"`.
### Failure model (concrete)
The failing test first reads `passwd_shell()` into an owned `String`, then calls `resolve_shell(None)` — **two separate `getpwuid` calls in a concurrently-running test binary** (default parallel test threads; sibling tests in the same binary spawn children and call the same resolver). Any concurrent passwd-database call can rewrite the static result during either window, so the two reads can disagree — the observed mismatched `/back` fragment is that signature. The right-hand value is a raw libc lookup, not a mocked fixture, so `/back` is not evidence of the runner's configured login shell.
## Causation and repro limits (explicit)
- The static-storage MT-unsafe use is **concrete and independently established** from the Linux man-pages/glibc API contract (`getpwuid`: MT-Unsafe `race:pwuid`; `getpwuid_r`: MT-Safe).
- **The exact interleaving that produced `/back` in CI is not proved.** The CI log cannot show which thread's call rewrote the storage, nor independently exclude an NSS/backend anomaly. This issue does not claim the CI interleaving was reproduced.
- Local Linux repro is currently blocked: `cargo test -p buzz-terminal --offline --locked` fails at dependency resolution (uncached `mesh-llm` git dependency pulled in by workspace resolution). macOS `getpwuid` returns a thread-specific structure, so a Darwin stress pass would not clear the Linux defect, and no local Docker/Linux environment was available. No reproduction was manufactured.
## Proposed correction (owner: native terminal/desktop Rust)
1. Replace `getpwuid` with `getpwuid_r` and **caller-owned storage** (`passwd` struct + buffer, `ERANGE` handled with bounded growth), validating pointers/encoding before copying; preserve lookup-error/absent-entry fallback and the existing executable-file validation and resolution-order assertions. A private mutex is insufficient — it cannot govern unrelated libc consumers in the same process.
2. Add a regression at the production lookup seam that discriminates old/new code under concurrent calls (not just a flaky stress pass), then run the full `cargo test -p buzz-terminal` package suite on **Linux/glibc** — that full-package Linux evidence is still needed and does not exist yet.
3. Test-hygiene debt (process-env mutation without restoration/serialization in sibling tests) may deserve separate hardening; it is not a fix for the unsafe lookup.
A green CI rerun at any head does **not** fix or explain this race; the residual risk stays open until the reentrant correction lands with Linux validation.
Contributor guide
Assessment
This issue has not been assessed yet.