From d8feeeee4c5432bdd02b6df70c5adf54b8a31bea Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 24 Jan 2023 14:03:57 -0800 Subject: [PATCH] wgengine/magicsock: fix buggy fast path in Conn.SetNetworkMap Spotted by Maisem. Fixes #6680 Change-Id: I5fdc01de8b006a1c43a2a4848f69397f54b4453a Co-authored-By: Maisem Ali Co-authored-By: Andrew Dunham Signed-off-by: Brad Fitzpatrick --- wgengine/magicsock/magicsock.go | 18 ++++++++++++++---- wgengine/magicsock/magicsock_test.go | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 1ce098809..9723eedf0 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -2499,15 +2499,25 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) { return } - if c.netMap != nil && nodesEqual(c.netMap.Peers, nm.Peers) { - return + priorNetmap := c.netMap + var priorDebug *tailcfg.Debug + if priorNetmap != nil { + priorDebug = priorNetmap.Debug } - + debugChanged := !reflect.DeepEqual(priorDebug, nm.Debug) metricNumPeers.Set(int64(len(nm.Peers))) - c.logf("[v1] magicsock: got updated network map; %d peers", len(nm.Peers)) + // Update c.netMap regardless, before the following early return. c.netMap = nm + if priorNetmap != nil && nodesEqual(priorNetmap.Peers, nm.Peers) && !debugChanged { + // The rest of this function is all adjusting state for peers that have + // changed. But if the set of peers is equal and the debug flags (for + // silent disco) haven't changed, no need to do anything else. + return + } + + c.logf("[v1] magicsock: got updated network map; %d peers", len(nm.Peers)) heartbeatDisabled := debugEnableSilentDisco() || (c.netMap != nil && c.netMap.Debug != nil && c.netMap.Debug.EnableSilentDisco) // Try a pass of just upserting nodes and creating missing diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index be0b73afd..c9b4a6c06 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -1836,3 +1836,21 @@ func TestRebindingUDPConn(t *testing.T) { c.setConnLocked(realConn.(nettype.PacketConn), "udp4") c.setConnLocked(newBlockForeverConn(), "") } + +// https://github.com/tailscale/tailscale/issues/6680: don't ignore +// SetNetworkMap calls when there are no peers. (A too aggressive fast path was +// previously bailing out early, thinking there were no changes since all zero +// peers didn't change, but the netmap has non-peer info in it too we shouldn't discard) +func TestSetNetworkMapWithNoPeers(t *testing.T) { + var c Conn + c.logf = logger.Discard + + for i := 1; i <= 3; i++ { + nm := &netmap.NetworkMap{} + c.SetNetworkMap(nm) + t.Logf("ptr %d: %p", i, nm) + if c.netMap != nm { + t.Fatalf("call %d: didn't store netmap", i) + } + } +}