fix: avoid tty stuck forever#348
Conversation
ssh-agent requests carry no environment, so the agent reuses the last environment the main agent saw. by the time a background process triggers an ssh request, that tty is often stale; pinentry then attaches to a dead tty, can't be seen or answered, and busy-loops at 100% cpu. with the previous --timeout 0 these accumulated indefinitely. - only pass --ttyname when the recorded tty still opens as a terminal - fail fast on ssh-agent requests when locked with no way to prompt, instead of spawning a hidden pinentry - bound pinentry lifetime with --timeout 120 instead of 0
add a `pinentry_timeout` config option controlling how long an unanswered pinentry prompt lingers before pinentry dismisses it. defaults to 120 seconds (previously hardcoded); set to 0 to disable the timeout and restore the old hang-forever behaviour.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds safeguards to prevent invisible pinentry prompts (notably for stale TTYs in ssh-agent flows) by introducing a configurable pinentry timeout and checking whether prompting is actually possible.
Changes:
- Add
pinentry_timeoutconfig option and thread it through all pinentry invocations. - Add
Environmenthelpers to detect a usable TTY / ability to prompt; prevent ssh-agent unlock attempts when no prompt can be displayed. - Minor refactors/modernizations (e.g.,
map_or, formatting updates,setrlimitcall).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/protocol.rs | Adds TTY usability checks and a “can we prompt” helper used by agent flows. |
| src/pinentry.rs | Makes pinentry timeout configurable and avoids passing stale --ttyname. |
| src/config.rs | Introduces pinentry_timeout with a default value. |
| src/bin/rbw/commands.rs | Supports setting/unsetting pinentry_timeout; small map_or refactor. |
| src/bin/rbw-agent/debugger.rs | Updates error formatting and setrlimit pointer usage. |
| src/bin/rbw-agent/actions.rs | Plumbs timeout into pinentry calls and bails early for ssh-agent when prompting isn’t possible. |
| src/api.rs | Adjusts login error classification match arm (currently introduces a syntax issue). |
| README.md | Documents the new pinentry_timeout option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "" | ||
| // bitwarden_rs returns an empty error and error_description for | ||
| // this case, for some reason | ||
| if error_desc.is_none() || error_desc == Some("") { | ||
| if (error_desc.is_none() || error_desc == Some("")) => { | ||
| if let Some(error_model) = error_res.error_model.as_ref() { | ||
| let message = error_model.message.as_str().to_string(); | ||
| match message.as_str() { |
| async fn config_pinentry_timeout() -> anyhow::Result<u64> { | ||
| let config = rbw::config::Config::load_async().await?; | ||
| Ok(config.pinentry_timeout) | ||
| } |
|
Hello, this project has not received maintenance in the last 6 months. I forked it and you can find it under https://github.com/deade1e/rbw/tree/refactor. The branch is receiving frequent commits right now. I changed the pinentry.rs file entirely (and a lot of other code). Could you please check if this bug keeps happening with my refactored version? Thank you |
I don't think i feel comfortable to directly use a fork for my password manager from a random fork, and it's quite impossible to review the 262 "refactor commits" on top of the original repo |
Summary
Fixes hidden
pinentryprocesses spawned byrbw-agentthat pile up at ~100% CPU when the vault is locked and an SSH-agent request comes in.This is what i see on htop:
The processes seen in
ps(pinentry --timeout 0 --ttyname /dev/ttysNNN) are launched by rbw-agent, not gpg-agent — that argv (--timeout 0 --ttynameon the command line) is built insrc/pinentry.rs; gpg passes the tty over the Assuan pipe instead.Root cause
SSH-agent requests carry no environment of their own, so the agent falls back to "the last environment the main agent saw" (
State::last_environment). By the time a background process triggers an SSH request, that TTY is usually stale. With--timeout 0, the terminal pinentry attaches to a dead TTY, can't be seen or answered, and busy-loops at 100% CPU forever. Every subsequent locked request spawns another one — hence the accumulation across different stale TTYs.Changes
Don't launch invisible pinentry on stale TTYs
--ttynamewhen the recorded TTY still opens as a terminal (Environment::has_usable_tty()— non-blockingO_NOCTTYopen +isatty). Live interactive terminals are unaffected; only dead TTYs are dropped, letting pinentry fall back to its own detection or a GUI prompt.get_ssh_public_keysandfind_ssh_private_key) when the vault is locked and there's no usable terminal or display, instead of spawning a hidden prompt. Returns an error telling the user to runrbw unlockfirst.Configurable prompt timeout
pinentry_timeoutconfig option (default120, replaces the old hardcoded--timeout 0). Set to0to disable and restore the old hang-forever behavior. Wired throughrbw config set/unsetand documented in the README.Caveats
/dev/ttysNNNcould now belong to a different live session — in that narrow casehas_usable_tty()returns true and we'd still pass it. The fail-fast guard is what fully covers the SSH path regardless.pinentry_timeout = 0is intentionally still allowed (documented as "disable"), unlikelock_timeoutwhich rejects0.Testing
cargo build/cargo clippyclean (no new warnings)cargo test --lib pinentrypassessetsid ssh -T ...) — previously spawned a runaway hidden pinentry; now it either drops the stale--ttynameor fails fast.