From d8de11a01b277c041f76f8d6594673f7e1d4aa21 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 25 Feb 2020 10:04:20 -0800 Subject: [PATCH] control: make Hostinfo accessed by pointer Fix potential races in copying aliased slices by value. Also few little doc updates. Signed-off-by: Brad Fitzpatrick --- cmd/relaynode/relaynode.go | 2 +- cmd/taillogin/taillogin.go | 2 +- control/controlclient/auto.go | 13 ++++++---- control/controlclient/direct.go | 42 ++++++++++++++++++++------------- ipn/backend.go | 5 ++++ ipn/local.go | 19 ++++++++------- tailcfg/tailcfg.go | 27 +++++++++++++-------- 7 files changed, 68 insertions(+), 42 deletions(-) diff --git a/cmd/relaynode/relaynode.go b/cmd/relaynode/relaynode.go index 1ed588f1b..c1c106a64 100644 --- a/cmd/relaynode/relaynode.go +++ b/cmd/relaynode/relaynode.go @@ -148,7 +148,7 @@ func main() { c, err := controlclient.New(controlclient.Options{ Persist: cfg, ServerURL: *server, - Hostinfo: &hi, + Hostinfo: hi, NewDecompressor: func() (controlclient.Decompressor, error) { return zstd.NewReader(nil) }, diff --git a/cmd/taillogin/taillogin.go b/cmd/taillogin/taillogin.go index 483e82c7a..5bdfaed64 100644 --- a/cmd/taillogin/taillogin.go +++ b/cmd/taillogin/taillogin.go @@ -47,7 +47,7 @@ func main() { c, err := controlclient.New(controlclient.Options{ Persist: cfg, ServerURL: *server, - Hostinfo: &hi, + Hostinfo: hi, }) if err != nil { log.Fatal(err) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index 1ae3946e2..e7648ae60 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -64,9 +64,9 @@ type Status struct { LoginFinished *empty.Message Err string URL string - Persist *Persist // locally persisted configuration - NetMap *NetworkMap // server-pushed configuration - Hostinfo tailcfg.Hostinfo // current Hostinfo data + Persist *Persist // locally persisted configuration + NetMap *NetworkMap // server-pushed configuration + Hostinfo *tailcfg.Hostinfo // current Hostinfo data state state } @@ -115,7 +115,7 @@ type Client struct { loggedIn bool // true if currently logged in loginGoal *LoginGoal // non-nil if some login activity is desired synced bool // true if our netmap is up-to-date - hostinfo tailcfg.Hostinfo + hostinfo *tailcfg.Hostinfo inPollNetMap bool // true if currently running a PollNetMap inSendStatus int // number of sendStatus calls currently in progress state state @@ -489,7 +489,10 @@ func (c *Client) SetStatusFunc(fn func(Status)) { c.mu.Unlock() } -func (c *Client) SetHostinfo(hi tailcfg.Hostinfo) { +func (c *Client) SetHostinfo(hi *tailcfg.Hostinfo) { + if hi == nil { + panic("nil Hostinfo") + } c.direct.SetHostinfo(hi) // Send new Hostinfo to server c.cancelMapSafely() diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 380900d86..98d1196fe 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -85,17 +85,17 @@ type Direct struct { persist Persist tryingNewKey wgcfg.PrivateKey expiry *time.Time - hostinfo tailcfg.Hostinfo + hostinfo *tailcfg.Hostinfo // always non-nil endpoints []string localPort uint16 // or zero to mean auto } type Options struct { - Persist Persist // initial persistent data - HTTPC *http.Client // HTTP client used to talk to tailcontrol - ServerURL string // URL of the tailcontrol server - TimeNow func() time.Time // time.Now implementation used by Client - Hostinfo *tailcfg.Hostinfo + Persist Persist // initial persistent data + HTTPC *http.Client // HTTP client used to talk to tailcontrol + ServerURL string // URL of the tailcontrol server + TimeNow func() time.Time // time.Now implementation used by Client + Hostinfo *tailcfg.Hostinfo // non-nil passes ownership, nil means to use default using os.Hostname, etc NewDecompressor func() (Decompressor, error) KeepAlive bool Logf logger.Logf @@ -120,9 +120,9 @@ func NewDirect(opts Options) (*Direct, error) { } if opts.Logf == nil { // TODO(apenwarr): remove this default and fail instead. + // TODO(bradfitz): ... but then it shouldn't be in Options. opts.Logf = log.Printf } - c := &Direct{ httpc: opts.HTTPC, serverURL: opts.ServerURL, @@ -135,38 +135,46 @@ func NewDirect(opts Options) (*Direct, error) { if opts.Hostinfo == nil { c.SetHostinfo(NewHostinfo()) } else { - c.SetHostinfo(*opts.Hostinfo) + c.SetHostinfo(opts.Hostinfo) } - return c, nil } -func NewHostinfo() tailcfg.Hostinfo { - hostname, _ := os.Hostname() +func hostinfoOS() string { os := runtime.GOOS switch os { case "darwin": switch runtime.GOARCH { case "arm", "arm64": - os = "iOS" + return "iOS" default: - os = "macOS" + return "macOS" } + default: + return os } +} - return tailcfg.Hostinfo{ +func NewHostinfo() *tailcfg.Hostinfo { + hostname, _ := os.Hostname() + return &tailcfg.Hostinfo{ IPNVersion: version.LONG, Hostname: hostname, - OS: os, + OS: hostinfoOS(), } } -func (c *Direct) SetHostinfo(hi tailcfg.Hostinfo) { +// SetHostinfo clones the provided Hostinfo and remembers it for the +// next update. +func (c *Direct) SetHostinfo(hi *tailcfg.Hostinfo) { + if hi == nil { + panic("nil Hostinfo") + } c.mu.Lock() defer c.mu.Unlock() c.logf("Hostinfo: %v\n", hi) - c.hostinfo = hi + c.hostinfo = hi.Copy() } func (c *Direct) GetPersist() Persist { diff --git a/ipn/backend.go b/ipn/backend.go index 1695e7076..10a907911 100644 --- a/ipn/backend.go +++ b/ipn/backend.go @@ -37,8 +37,11 @@ type EngineStatus struct { type NetworkMap = controlclient.NetworkMap +// Notify is a communication from a backend (e.g. tailscaled) to a frontend +// (cmd/tailscale, iOS, macOS, Win Tasktray). // In any given notification, any or all of these may be nil, meaning // that they have not changed. +// They are JSON-encoded on the wire, despite the lack of struct tags. type Notify struct { Version string // version number of IPN backend ErrMessage *string // critical error message, if any @@ -49,6 +52,8 @@ type Notify struct { Engine *EngineStatus // wireguard engine stats BrowseToURL *string // UI should open a browser right now BackendLogID *string // public logtail id used by backend + + // type is mirrored in xcode/Shared/IPN.swift } // StateKey is an opaque identifier for a set of LocalBackend state diff --git a/ipn/local.go b/ipn/local.go index 8b79e1759..cf83d5ca1 100644 --- a/ipn/local.go +++ b/ipn/local.go @@ -24,11 +24,12 @@ import ( ) // LocalBackend is the scaffolding between the Tailscale cloud control -// plane and the local network stack. +// plane and the local network stack, wiring up NetworkMap updates +// from the cloud to the local WireGuard engine. type LocalBackend struct { logf logger.Logf - notify func(n Notify) - c *controlclient.Client + notify func(Notify) + c *controlclient.Client // TODO: appears to be (inconsistently) guarded by mu e wgengine.Engine store StateStore serverURL string @@ -42,7 +43,7 @@ type LocalBackend struct { stateKey StateKey prefs *Prefs state State - hiCache tailcfg.Hostinfo + hiCache *tailcfg.Hostinfo netMapCache *controlclient.NetworkMap engineStatus EngineStatus endPoints []string @@ -146,7 +147,9 @@ func (b *LocalBackend) Start(opts Options) error { hi.FrontendLogID = opts.FrontendLogID b.mu.Lock() - hi.Services = b.hiCache.Services // keep any previous session + if b.hiCache != nil { + hi.Services = b.hiCache.Services // keep any previous session + } b.hiCache = hi b.state = NoState @@ -176,7 +179,7 @@ func (b *LocalBackend) Start(opts Options) error { }, Persist: *persist, ServerURL: b.serverURL, - Hostinfo: &hi, + Hostinfo: hi, KeepAlive: true, NewDecompressor: b.newDecompressor, }) @@ -519,12 +522,12 @@ func (b *LocalBackend) SetPrefs(new *Prefs) { oldHi := b.hiCache newHi := oldHi.Copy() newHi.RoutableIPs = append([]wgcfg.CIDR(nil), b.prefs.AdvertiseRoutes...) - b.hiCache = *newHi + b.hiCache = newHi cli := b.c b.mu.Unlock() if cli != nil && !oldHi.Equal(newHi) { - cli.SetHostinfo(*newHi) + cli.SetHostinfo(newHi) } if old.WantRunning != new.WantRunning { diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 096cf8ab4..45a0f1f4a 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -208,10 +208,14 @@ type Service struct { // require changes to Hostinfo.Copy. } +// Hostinfo contains a summary of a Tailscale host. +// +// Because it contains pointers (slices), this type should not be used +// as a value type. type Hostinfo struct { // TODO(crawshaw): mark all these fields ",omitempty" when all the // iOS apps are updated with the latest swift version of this struct. - IPNVersion string // version number of this code + IPNVersion string // version of this code FrontendLogID string // logtail ID of frontend instance BackendLogID string // logtail ID of backend instance OS string // operating system the client runs on @@ -225,12 +229,12 @@ type Hostinfo struct { // Copy makes a deep copy of Hostinfo. // The result aliases no memory with the original. -func (hinfo *Hostinfo) Copy() (res *Hostinfo) { +func (h *Hostinfo) Copy() (res *Hostinfo) { res = new(Hostinfo) - *res = *hinfo + *res = *h - res.RoutableIPs = append([]wgcfg.CIDR{}, res.RoutableIPs...) - res.Services = append([]Service{}, res.Services...) + res.RoutableIPs = append([]wgcfg.CIDR{}, h.RoutableIPs...) + res.Services = append([]Service{}, h.Services...) return res } @@ -255,19 +259,22 @@ type RegisterRequest struct { } Expiry time.Time // requested key expiry, server policy may override Followup string // response waits until AuthURL is visited - Hostinfo Hostinfo + Hostinfo *Hostinfo } // Copy makes a deep copy of RegisterRequest. // The result aliases no memory with the original. func (req *RegisterRequest) Copy() *RegisterRequest { - res := *req - res.Hostinfo = *res.Hostinfo.Copy() + res := new(RegisterRequest) + *res = *req + if res.Hostinfo != nil { + res.Hostinfo = res.Hostinfo.Copy() + } if res.Auth.Oauth2Token != nil { tok := *res.Auth.Oauth2Token res.Auth.Oauth2Token = &tok } - return &res + return res } // RegisterResponse is returned by the server in response to a RegisterRequest. @@ -293,7 +300,7 @@ type MapRequest struct { NodeKey NodeKey Endpoints []string Stream bool // if true, multiple MapResponse objects are returned - Hostinfo Hostinfo + Hostinfo *Hostinfo } type MapResponse struct {