From 171ec9f8f4e4b4751b5f07dbed6f01514597c1a0 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 15 Jul 2021 22:34:50 -0700 Subject: [PATCH] control/{controlknobs,controlclient}: simplify knobs API, fix controlclient crash From integration tests elsewhere: panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x70 pc=0x845c9b] goroutine 226 [running]: tailscale.com/control/controlclient.(*Direct).sendMapRequest(0xc00053e1e0, 0x16670f0, 0xc000353780, 0xffffffffffffffff, 0xc0003e5f10, 0x0, 0x0) /home/runner/go/pkg/mod/tailscale.com@v1.1.1-0.20210715222212-1bb6abc604c1/control/controlclient/direct.go:803 +0x19bb tailscale.com/control/controlclient.(*Direct).PollNetMap(...) /home/runner/go/pkg/mod/tailscale.com@v1.1.1-0.20210715222212-1bb6abc604c1/control/controlclient/direct.go:574 tailscale.com/control/controlclient.(*Auto).mapRoutine(0xc00052a1e0) /home/runner/go/pkg/mod/tailscale.com@v1.1.1-0.20210715222212-1bb6abc604c1/control/controlclient/auto.go:464 +0x571 created by tailscale.com/control/controlclient.(*Auto).Start /home/runner/go/pkg/mod/tailscale.com@v1.1.1-0.20210715222212-1bb6abc604c1/control/controlclient/auto.go:151 +0x65 exit status 2 Also remove types/opt.Bool API addition which is now unnecessary. Signed-off-by: Brad Fitzpatrick --- control/controlclient/direct.go | 2 +- control/controlknobs/controlknobs.go | 25 +++++++++---------------- net/portmapper/upnp.go | 4 ++-- types/opt/bool.go | 7 ------- types/opt/bool_test.go | 23 ----------------------- 5 files changed, 12 insertions(+), 49 deletions(-) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 05c905481..5037c25c9 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -800,7 +800,7 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*netm hasDebug := resp.Debug != nil // being conservative here, if Debug not present set to False - controlknobs.SetDisableUPnP(resp.Debug.DisableUPnP.And(hasDebug)) + controlknobs.SetDisableUPnP(hasDebug && resp.Debug.DisableUPnP.EqualBool(true)) if hasDebug { if resp.Debug.LogHeapPprof { go logheap.LogHeap(resp.Debug.LogHeapURL) diff --git a/control/controlknobs/controlknobs.go b/control/controlknobs/controlknobs.go index 884c0f8dd..84d71fdf2 100644 --- a/control/controlknobs/controlknobs.go +++ b/control/controlknobs/controlknobs.go @@ -9,33 +9,26 @@ package controlknobs import ( "os" "strconv" - "sync/atomic" - "tailscale.com/types/opt" + "tailscale.com/syncs" ) // disableUPnP indicates whether to attempt UPnP mapping. -var disableUPnP atomic.Value +var disableUPnP syncs.AtomicBool func init() { v, _ := strconv.ParseBool(os.Getenv("TS_DISABLE_UPNP")) - var toStore opt.Bool - toStore.Set(v) - disableUPnP.Store(toStore) + SetDisableUPnP(v) } // DisableUPnP reports the last reported value from control // whether UPnP portmapping should be disabled. -func DisableUPnP() opt.Bool { - v, _ := disableUPnP.Load().(opt.Bool) - return v +func DisableUPnP() bool { + return disableUPnP.Get() } -// SetDisableUPnP will set whether UPnP connections are permitted or not, -// intended to be set from control. -func SetDisableUPnP(v opt.Bool) { - old, ok := disableUPnP.Load().(opt.Bool) - if !ok || old != v { - disableUPnP.Store(v) - } +// SetDisableUPnP sets whether control says that UPnP should be +// disabled. +func SetDisableUPnP(v bool) { + disableUPnP.Set(v) } diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index bf51d2141..4adb31eb3 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -125,7 +125,7 @@ func addAnyPortMapping( // now. // Adapted from https://github.com/huin/goupnp/blob/master/GUIDE.md. func getUPnPClient(ctx context.Context, gw netaddr.IP) (upnpClient, error) { - if dis, ok := controlknobs.DisableUPnP().Get(); ok && dis { + if controlknobs.DisableUPnP() { return nil, nil } ctx, cancel := context.WithTimeout(ctx, 250*time.Millisecond) @@ -177,7 +177,7 @@ func (c *Client) getUPnPPortMapping( internal netaddr.IPPort, prevPort uint16, ) (external netaddr.IPPort, ok bool) { - if dis, ok := controlknobs.DisableUPnP().Get(); ok && dis { + if controlknobs.DisableUPnP() { return netaddr.IPPort{}, false } now := time.Now() diff --git a/types/opt/bool.go b/types/opt/bool.go index 7044fe2f3..3cdf08d27 100644 --- a/types/opt/bool.go +++ b/types/opt/bool.go @@ -29,13 +29,6 @@ func (b Bool) Get() (v bool, ok bool) { return v, err == nil } -func (b Bool) And(v bool) Bool { - if v { - return b - } - return "false" -} - // EqualBool reports whether b is equal to v. // If b is empty or not a valid bool, it reports false. func (b Bool) EqualBool(v bool) bool { diff --git a/types/opt/bool_test.go b/types/opt/bool_test.go index dfbefa911..f7bfc910c 100644 --- a/types/opt/bool_test.go +++ b/types/opt/bool_test.go @@ -87,26 +87,3 @@ func TestBoolEqualBool(t *testing.T) { } } - -func TestBoolAnd(t *testing.T) { - tests := []struct { - lhs Bool - rhs bool - want Bool - }{ - {"true", true, "true"}, - {"true", false, "false"}, - - {"false", true, "false"}, - {"false", false, "false"}, - - {"", true, ""}, - {"", false, "false"}, - } - for _, tt := range tests { - if got := tt.lhs.And(tt.rhs); got != tt.want { - t.Errorf("(%q).And(%v) = %v; want %v", string(tt.lhs), tt.rhs, got, tt.want) - } - } - -}