From a2a81c5f3422c235df3904a83971b9adc5557b3e Mon Sep 17 00:00:00 2001 From: kari-ts Date: Wed, 25 Sep 2024 11:32:11 -0400 Subject: [PATCH] android: don't set vpnService to nil when state is Stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We are currently setting vpnService.service to nil: -any time there’s an error with updateTUN -when we exit out of runBackend -if the config is the default config (aka when the ipn state is Stopped) When it gets set to nil, we don’t handle state or config updates by calling updateTUN until after startVPN is called again. The second case never happens because there’s no condition to break out of the loop in runBackend and ctx is uncancelable per the doc for context.Background() In the third case, we should not establish the VPN; the state is already in the correct Stopped state, but there’s no need to set the service to nil and prevent updateTUN from being called. The quick settings tile bug is caused by this third case, where because the saved prefs starts the app up in the Stopped state, the config is set to the default config, and the service is set to nil, and we can't updateTUN until there’s another startVPN call. This PR: -cleans up the updateTUN error handling to be more consistent -removes the IPNService parameter from updateTUN so that vpnService.service is not set to nil in the third case -updates IPNService to use stopSelf and not stopForeground when we disconnect the VPN; the latter only disconnects if there is a memory need Fixes tailscale/tailscale#12489 Signed-off-by: kari-ts --- .../main/java/com/tailscale/ipn/IPNService.kt | 6 +- libtailscale/backend.go | 62 +++++++++++-------- libtailscale/interfaces.go | 2 + libtailscale/net.go | 10 +-- 4 files changed, 44 insertions(+), 36 deletions(-) diff --git a/android/src/main/java/com/tailscale/ipn/IPNService.kt b/android/src/main/java/com/tailscale/ipn/IPNService.kt index f7cc396e27..509e24bee1 100644 --- a/android/src/main/java/com/tailscale/ipn/IPNService.kt +++ b/android/src/main/java/com/tailscale/ipn/IPNService.kt @@ -73,10 +73,14 @@ open class IPNService : VpnService(), libtailscale.IPNService { override fun close() { app.setWantRunning(false) { updateVpnStatus(false) } Notifier.setState(Ipn.State.Stopping) - stopForeground(STOP_FOREGROUND_REMOVE) + disconnectVPN() Libtailscale.serviceDisconnect(this) } + override fun disconnectVPN(){ + stopSelf() + } + override fun onDestroy() { close() super.onDestroy() diff --git a/libtailscale/backend.go b/libtailscale/backend.go index 247a802a7b..dc5df4c2f5 100644 --- a/libtailscale/backend.go +++ b/libtailscale/backend.go @@ -11,6 +11,7 @@ import ( "net/http" "os" "path/filepath" + "reflect" "runtime/debug" "sync" "sync/atomic" @@ -165,36 +166,24 @@ func (a *App) runBackend(ctx context.Context) error { select { case s := <-stateCh: state = s - if cfg.rcfg != nil && state >= ipn.Starting && vpnService.service != nil { + if state >= ipn.Starting && vpnService.service != nil && b.isConfigNonNilAndDifferent(cfg.rcfg, cfg.dcfg) { // On state change, check if there are router or config changes requiring an update to VPNBuilder - if err := b.updateTUN(vpnService.service, cfg.rcfg, cfg.dcfg); err != nil { + if err := b.updateTUN(cfg.rcfg, cfg.dcfg); err != nil { if errors.Is(err, errMultipleUsers) { // TODO: surface error to user } - log.Printf("VPN update failed: %v", err) - - mp := new(ipn.MaskedPrefs) - mp.WantRunning = false - mp.WantRunningSet = true - - _, err := a.EditPrefs(*mp) - if err != nil { - log.Printf("localapi edit prefs error %v", err) - } - - b.lastCfg = nil - b.CloseTUNs() + a.closeVpnService(err, b) } } case n := <-netmapCh: networkMap = n case c := <-configs: cfg = c - if b == nil || vpnService.service == nil || cfg.rcfg == nil { + if vpnService.service == nil || !b.isConfigNonNilAndDifferent(cfg.rcfg, cfg.dcfg) { configErrs <- nil break } - configErrs <- b.updateTUN(vpnService.service, cfg.rcfg, cfg.dcfg) + configErrs <- b.updateTUN(cfg.rcfg, cfg.dcfg) case s := <-onVPNRequested: if vpnService.service != nil && vpnService.service.ID() == s.ID() { // Still the same VPN instance, do nothing @@ -230,12 +219,9 @@ func (a *App) runBackend(ctx context.Context) error { if networkMap != nil { // TODO } - if cfg.rcfg != nil && state >= ipn.Starting { - if err := b.updateTUN(vpnService.service, cfg.rcfg, cfg.dcfg); err != nil { - log.Printf("VPN update failed: %v", err) - vpnService.service.Close() - b.lastCfg = nil - b.CloseTUNs() + if state >= ipn.Starting && b.isConfigNonNilAndDifferent(cfg.rcfg, cfg.dcfg) { + if err := b.updateTUN(cfg.rcfg, cfg.dcfg); err != nil { + a.closeVpnService(err, b) } } case s := <-onDisconnect: @@ -245,9 +231,7 @@ func (a *App) runBackend(ctx context.Context) error { vpnService.service = nil } case i := <-onDNSConfigChanged: - if b != nil { - go b.NetworkChanged(i) - } + go b.NetworkChanged(i) } } } @@ -346,3 +330,29 @@ func (a *App) newBackend(dataDir, directFileRoot string, appCtx AppContext, stor }() return b, nil } + +func (b *backend) isConfigNonNilAndDifferent(rcfg *router.Config, dcfg *dns.OSConfig) bool { + if reflect.DeepEqual(rcfg, b.lastCfg) && reflect.DeepEqual(dcfg, b.lastDNSCfg) { + b.logger.Logf("isConfigNonNilAndDifferent: no change to Routes or DNS, ignore") + return false + } + return rcfg != nil +} + +func (a *App) closeVpnService(err error, b *backend) { + log.Printf("VPN update failed: %v", err) + + mp := new(ipn.MaskedPrefs) + mp.WantRunning = false + mp.WantRunningSet = true + + if _, localApiErr := a.EditPrefs(*mp); localApiErr != nil { + log.Printf("localapi edit prefs error %v", localApiErr) + } + + b.lastCfg = nil + b.CloseTUNs() + + vpnService.service.DisconnectVPN() + vpnService.service = nil +} diff --git a/libtailscale/interfaces.go b/libtailscale/interfaces.go index 0275d6ff4a..6460c9f3c2 100644 --- a/libtailscale/interfaces.go +++ b/libtailscale/interfaces.go @@ -80,6 +80,8 @@ type IPNService interface { Close() + DisconnectVPN() + UpdateVpnStatus(bool) } diff --git a/libtailscale/net.go b/libtailscale/net.go index c4cb0a8381..85d2ef6cbf 100644 --- a/libtailscale/net.go +++ b/libtailscale/net.go @@ -9,7 +9,6 @@ import ( "log" "net" "net/netip" - "reflect" "runtime/debug" "strings" "syscall" @@ -125,12 +124,7 @@ var googleDNSServers = []netip.Addr{ netip.MustParseAddr("2001:4860:4860::8844"), } -func (b *backend) updateTUN(service IPNService, rcfg *router.Config, dcfg *dns.OSConfig) error { - if reflect.DeepEqual(rcfg, b.lastCfg) && reflect.DeepEqual(dcfg, b.lastDNSCfg) { - b.logger.Logf("updateTUN: no change to Routes or DNS, ignore") - return nil - } - +func (b *backend) updateTUN(rcfg *router.Config, dcfg *dns.OSConfig) error { b.logger.Logf("updateTUN: changed") defer b.logger.Logf("updateTUN: finished") @@ -147,7 +141,6 @@ func (b *backend) updateTUN(service IPNService, rcfg *router.Config, dcfg *dns.O if len(rcfg.LocalAddrs) == 0 { return nil } - vpnService.service = service builder := vpnService.service.NewBuilder() b.logger.Logf("updateTUN: got new builder") @@ -258,7 +251,6 @@ func closeFileDescriptor() error { func (b *backend) CloseTUNs() { b.lastCfg = nil b.devices.Shutdown() - vpnService.service = nil } // ifname is the interface name retrieved from LinkProperties on network change. If a network is lost, an empty string is passed in.