GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix: prevent tailnet coordination panics by dylanhuff-at-coder · Pull Request #29300 · coder/coder

Conversation

dylanhuff-at-coder commented Sep 14, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix: validate tailnet coordination requests

cb52fd4

linear-code Bot commented Sep 14, 2026

Copy link

Copy Markdown

PLAT-595

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot assigned dylanhuff-at-coder

Sep 14, 2026


          fix: prevent tailnet peer cleanup panics

07f7d7e

dylanhuff-at-coder changed the title ~~fix: validate tailnet coordination requests~~ fix: prevent tailnet coordination panics

Sep 14, 2026

dylanhuff-at-coder marked this pull request as ready for review

September 15, 2026 15:18

dylanhuff-at-coder requested review from a team and geokat

September 15, 2026 15:18

geokat approved these changes

Sep 15, 2026

View reviewed changes

geokat left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, except for one potential P2 and some nits.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tailnet/peer.go


		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() {

geokat Sep 15, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dylanhuff-at-coder Sep 18, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, added recovery around the outer goroutine too, with a regression test for a panic during cleanup.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tailnet/coordinator.go

		return xerrors.Errorf("node update failed: %w", err)
		}
		}
		if req.AddTunnel != nil {

geokat Sep 15, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An agent found this:

nodeUpdateLocked can now return successfully after recursive fan-out removes p. A compound request then continues into AddTunnel using the stale peer, potentially creating an orphan tunnel or sending on its closed response channel. Recheck c.peers[p.id] == p and return ErrAlreadyRemoved before continuing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dylanhuff-at-coder Sep 18, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the peer recheck after fan-out, plus regression cases for compound requests with AddTunnel and ReadyForHandshake.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

enterprise/tailnet/pgcoord.go Outdated

		func (m *mapper) run() {
		defer func() {
		if recovered := recover(); recovered != nil {
		m.logger.Error(m.ctx, "panic mapping peer responses (recovered)",

geokat Sep 15, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would "panic processing peer mappings (recovered)" be more clear?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dylanhuff-at-coder Sep 18, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that reads better. Updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tailnet/test/requests.go Outdated

		{
		name: "NilReadyForHandshake",
		req: &proto.CoordinateRequest{ReadyForHandshake: []*proto.CoordinateRequest_ReadyForHandshake{nil}},
		err: "ready_for_handshake entry is required",

geokat Sep 15, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dylanhuff-at-coder Sep 18, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, an empty list is valid. Updated the message and test expectations.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix: address tailnet coordinator review feedback

24c87fa

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.

dylanhuff-at-coder merged commit ae76be4 into main

Sep 22, 2026

27 of 28 checks passed

Uh oh!

There was an error while loading. Please reload this page.

dylanhuff-at-coder deleted the dylan/plat-595-security-nil-node-in-tailnet-updateself-crashes-entire branch

September 22, 2026 16:31

github-actions Bot locked and limited conversation to collaborators

Sep 22, 2026

Reviewers

geokat

geokat approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

dylanhuff-at-coder

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

dylanhuff-at-coder commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear-code Bot commented Sep 14, 2026

Uh oh!

geokat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dylanhuff-at-coder commented Sep 14, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.