block / block/buzz

buzz-terminal: shell lookup uses MT-unsafe getpwuid static storage (pre-existing; env_fence CI failure in run 34631406730)

Open
#7,589 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.