fix: prevent tailnet coordination panics - #29300
dylanhuff-at-coder merged 3 commits into
Conversation
dylanhuff-at-coder
changed the title
geokat
left a comment
geokat
left a comment
There was a problem hiding this comment.
lgtm, except for one potential P2 and some nits.
|
|
||
| func (p *peer) reqLoop(ctx context.Context, logger slog.Logger, handler func(context.Context, *peer, *proto.CoordinateRequest) error) error { | ||
| func (p *peer) reqLoop(ctx context.Context, logger slog.Logger, handler func(context.Context, *peer, *proto.CoordinateRequest) error) (err error) { | ||
| defer func() { |
There was a problem hiding this comment.
Just a heads-up, this recovery is not at the top of the goroutine, so if the rest of the goroutine's clean up code panics, coderd will still crash. While the original issue asks for a recovery around "coordinator per-connection goroutine".
On the other hand, the bug seems to be covered, so this is a non-blocking nit.
There was a problem hiding this comment.
Good catch, added recovery around the outer goroutine too, with a regression test for a panic during cleanup.
| return xerrors.Errorf("node update failed: %w", err) | ||
| } | ||
| } | ||
| if req.AddTunnel != nil { |
There was a problem hiding this comment.
An agent found this:
nodeUpdateLockedcan now return successfully after recursive fan-out removesp. A compound request then continues into AddTunnel using the stale peer, potentially creating an orphan tunnel or sending on its closed response channel. Recheckc.peers[p.id] == pand returnErrAlreadyRemovedbefore continuing.
There was a problem hiding this comment.
Added the peer recheck after fan-out, plus regression cases for compound requests with AddTunnel and ReadyForHandshake.
| func (m *mapper) run() { | ||
| defer func() { | ||
| if recovered := recover(); recovered != nil { | ||
| m.logger.Error(m.ctx, "panic mapping peer responses (recovered)", |
There was a problem hiding this comment.
Would "panic processing peer mappings (recovered)" be more clear?
There was a problem hiding this comment.
Yeah, that reads better. Updated.
| { | ||
| name: "NilReadyForHandshake", | ||
| req: &proto.CoordinateRequest{ReadyForHandshake: []*proto.CoordinateRequest_ReadyForHandshake{nil}}, | ||
| err: "ready_for_handshake entry is required", |
There was a problem hiding this comment.
Nit: Should this say ready_for_handshake entries must not be nil? “Entry is required” could imply the list must be non-empty, although an empty list is valid here I think?
There was a problem hiding this comment.
Good point, an empty list is valid. Updated the message and test expectations.
Contain cleanup panics at the connection goroutine boundary and stop compound requests when node update fan-out removes their peer. Clarify recovered mapping and nil handshake entry messages. Generated by Coder Agents.
2 participants
Malformed coordinate requests can panic per-connection coordinator goroutines and terminate coderd. Disconnect requests and nested cleanup of peers with full response buffers can also attempt to send on or close an already-closed channel.
Validate request structure before authorization or state mutation in both tailnet coordinators, stop processing after disconnect, and avoid removing a peer twice during nested cleanup. Unexpected per-connection panics close the affected connection through the existing lost-peer cleanup.
https://linear.app/codercom/issue/PLAT-595/security-nil-node-in-tailnet-updateself-crashes-entire-coderd-process