Skip to content

don't panic when sending error response to disconnected client#332

Open
yoyo930021 wants to merge 1 commit intodoy:mainfrom
yoyo930021:fix-agent-panic-on-broken-pipe
Open

don't panic when sending error response to disconnected client#332
yoyo930021 wants to merge 1 commit intodoy:mainfrom
yoyo930021:fix-agent-panic-on-broken-pipe

Conversation

@yoyo930021
Copy link
Copy Markdown

Summary

If handle_request fails and the client has already disconnected (for
example, the client was killed, timed out, or aborted while the agent
was still blocked on pinentry), sock.send(...).await.unwrap() on
src/bin/rbw-agent/agent.rs:82 panics the spawned tokio task on
Broken pipe (os error 32). Because it's a detached tokio::spawn
task, the panic is effectively silent from the client's perspective —
the socket is just dropped, and the client then reads EOF and reports:

rbw: failed to parse message from agent: EOF while parsing a value at line 1 column 0

This hides the original error (pinentry failure, network error, TLS
error, vault error, etc.) and makes agent-side issues very hard to
diagnose, because the real error message is thrown away and the panic
output in agent.err can be easy to miss (or missed entirely if the
user is looking in the wrong location for the log file).

Replace the .unwrap() with a log::warn! on send failure so the
spawned task exits cleanly, the agent stays alive, and at least the
send failure shows up in the agent log.

This does not fix the root cause of whatever made handle_request
return Err — the caller will still see EOF in the disconnect race
— but it removes the silent panic, which means any error path that
would have been reported to a still-connected client now is, and
agent logs become meaningful again.

Reproduction

On macOS with a daemonized agent, configuring pinentry to point at
a curses pinentry that can't acquire a usable TTY makes
handle_request return Err from actions::login. If the client
also experiences a delay (e.g. ttyname_r can take several seconds
on macOS — see #315) and the two races line up so the client has
closed the socket by the time the agent tries to write the error back,
the agent hits this unwrap() and the user sees the unhelpful
"EOF while parsing" message with no obvious cause.

The same silent-panic pattern triggers for any handler error when
the client has disconnected — network, TLS, 2FA, etc. — so the bug
isn't macOS- or pinentry-specific, just particularly easy to hit
there.

Test plan

  • Start agent foreground with --no-daemonize
  • From a client, send a Login request then immediately close the
    socket (simulating client timeout/Ctrl+C mid-pinentry)
  • Before fix: thread 'tokio-runtime-worker' panicked at src/bin/rbw-agent/agent.rs:82:30: called Result::unwrap() on an Err value: failed to write message to socket ... Broken pipe
  • After fix: WARN rbw_agent::agent: failed to send error response to client: failed to write message to socket: Broken pipe (os error 32), agent stays alive, subsequent requests still served

If handle_request fails after the client has disconnected (for example
a client timeout while the agent was still waiting on pinentry), the
unwrap in the error-reporting path panics the tokio task and drops the
socket. The client then reads EOF from the socket and reports an
unhelpful "failed to parse message from agent: EOF while parsing a
value at line 1 column 0", hiding the original error.

Log the send failure as a warning instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant