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 <bradfitz@tailscale.com>

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)
 		},

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)

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

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"
 		}
-	}
-
-	return tailcfg.Hostinfo{
-		IPNVersion: version.LONG,
-		Hostname:   hostname,
-		OS:         os,
+	default:
+		return os
 	}
 }
 
-func (c *Direct) SetHostinfo(hi tailcfg.Hostinfo) {
+func NewHostinfo() *tailcfg.Hostinfo {
+	hostname, _ := os.Hostname()
+	return &tailcfg.Hostinfo{
+		IPNVersion: version.LONG,
+		Hostname:   hostname,
+		OS:         hostinfoOS(),
+	}
+}
+
+// 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 {

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

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 {

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 {