From 4a79eda41056bbacd768e9dff5fb979803cf7bb7 Mon Sep 17 00:00:00 2001 From: Jonathan Nobels Date: Wed, 12 Jun 2024 15:11:34 -0400 Subject: [PATCH] android/ui: switch lateinint app instance to optional updates tailscale/corp#12430 The app instance is set in onCreate, but this has a chance to race with the intent handlers throwing a null pointer exception. This should ultimately be corrected in the API interface on the caller side, on but that fix runs deep and still requires care on the part of the caller. This will, at least, error out gracefully rather than crashing the application process at the expense of an action potentially failing. Signed-off-by: Jonathan Nobels --- .../src/main/java/com/tailscale/ipn/App.kt | 21 ++- .../main/java/com/tailscale/ipn/IPNService.kt | 12 +- .../java/com/tailscale/ipn/MainActivity.kt | 25 ++- .../com/tailscale/ipn/QuickToggleService.java | 10 +- .../com/tailscale/ipn/StartVPNWorker.java | 8 +- .../java/com/tailscale/ipn/StopVPNWorker.java | 8 +- .../com/tailscale/ipn/UseExitNodeWorker.kt | 163 +++++++++--------- .../ipn/mdm/MDMSettingsDefinitions.kt | 4 +- .../tailscale/ipn/ui/util/AndroidTVUtil.kt | 6 +- .../ipn/ui/viewModel/IpnViewModel.kt | 4 +- 10 files changed, 142 insertions(+), 119 deletions(-) diff --git a/android/src/main/java/com/tailscale/ipn/App.kt b/android/src/main/java/com/tailscale/ipn/App.kt index 8f37e47..c2f350c 100644 --- a/android/src/main/java/com/tailscale/ipn/App.kt +++ b/android/src/main/java/com/tailscale/ipn/App.kt @@ -55,15 +55,15 @@ class App : UninitializedApp(), libtailscale.AppContext { .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) .build() - private lateinit var appInstance: App + private var appInstance: App? = null /** * Initializes the app (if necessary) and returns the singleton app instance. Always use this * function to obtain an App reference to make sure the app initializes. */ @JvmStatic - fun get(): App { - appInstance.initOnce() + fun get(): App? { + appInstance?.initOnce() return appInstance } } @@ -82,6 +82,8 @@ class App : UninitializedApp(), libtailscale.AppContext { override fun onCreate() { super.onCreate() + appInstance = this + createNotificationChannel( STATUS_CHANNEL_ID, getString(R.string.vpn_status), @@ -92,7 +94,7 @@ class App : UninitializedApp(), libtailscale.AppContext { getString(R.string.taildrop_file_transfers), getString(R.string.notifications_delivered_when_a_file_is_received_using_taildrop), NotificationManagerCompat.IMPORTANCE_DEFAULT) - appInstance = this + setUnprotectedInstance(this) } @@ -128,8 +130,9 @@ class App : UninitializedApp(), libtailscale.AppContext { applicationScope.launch { Notifier.state.collect { state -> val ableToStartVPN = state > Ipn.State.NeedsMachineAuth - // If VPN is stopped, show a disconnected notification. If it is running as a foregrround service, IPNService will show a connected notification. - if (state == Ipn.State.Stopped){ + // If VPN is stopped, show a disconnected notification. If it is running as a foregrround + // service, IPNService will show a connected notification. + if (state == Ipn.State.Stopped) { notifyStatus(false) } val vpnRunning = state == Ipn.State.Starting || state == Ipn.State.Running @@ -345,11 +348,11 @@ open class UninitializedApp : Application() { // File for shared preferences that are not encrypted. private const val UNENCRYPTED_PREFERENCES = "unencrypted" - private lateinit var appInstance: UninitializedApp + private var appInstance: UninitializedApp? = null lateinit var notificationManager: NotificationManagerCompat @JvmStatic - fun get(): UninitializedApp { + fun get(): UninitializedApp? { return appInstance } } @@ -389,7 +392,7 @@ open class UninitializedApp : Application() { } fun notifyStatus(vpnRunning: Boolean) { - notifyStatus(buildStatusNotification(vpnRunning)) + notifyStatus(buildStatusNotification(vpnRunning)) } fun notifyStatus(notification: Notification) { diff --git a/android/src/main/java/com/tailscale/ipn/IPNService.kt b/android/src/main/java/com/tailscale/ipn/IPNService.kt index 900483a..53ed49e 100644 --- a/android/src/main/java/com/tailscale/ipn/IPNService.kt +++ b/android/src/main/java/com/tailscale/ipn/IPNService.kt @@ -27,13 +27,13 @@ open class IPNService : VpnService(), libtailscale.IPNService { override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int = when (intent?.action) { ACTION_STOP_VPN -> { - App.get().setWantRunning(false) + App.get()?.setWantRunning(false) close() START_NOT_STICKY } ACTION_START_VPN -> { showForegroundNotification() - App.get().setWantRunning(true) + App.get()?.setWantRunning(true) Libtailscale.requestVPN(this) START_STICKY } @@ -41,15 +41,15 @@ open class IPNService : VpnService(), libtailscale.IPNService { // This means we were started by Android due to Always On VPN. // We show a non-foreground notification because we weren't // started as a foreground service. - App.get().notifyStatus(true) - App.get().setWantRunning(true) + App.get()?.notifyStatus(true) + App.get()?.setWantRunning(true) Libtailscale.requestVPN(this) START_STICKY } else -> { // This means that we were restarted after the service was killed // (potentially due to OOM). - if (UninitializedApp.get().isAbleToStartVPN()) { + if (UninitializedApp.get()?.isAbleToStartVPN() == true) { showForegroundNotification() App.get() Libtailscale.requestVPN(this) @@ -78,7 +78,7 @@ open class IPNService : VpnService(), libtailscale.IPNService { private fun showForegroundNotification() { startForeground( UninitializedApp.STATUS_NOTIFICATION_ID, - UninitializedApp.get().buildStatusNotification(true)) + UninitializedApp.get()?.buildStatusNotification(true)) } private fun configIntent(): PendingIntent { diff --git a/android/src/main/java/com/tailscale/ipn/MainActivity.kt b/android/src/main/java/com/tailscale/ipn/MainActivity.kt index a4b9631..e3c7dd5 100644 --- a/android/src/main/java/com/tailscale/ipn/MainActivity.kt +++ b/android/src/main/java/com/tailscale/ipn/MainActivity.kt @@ -11,7 +11,6 @@ import android.content.RestrictionsManager import android.content.pm.ActivityInfo import android.content.res.Configuration.SCREENLAYOUT_SIZE_LARGE import android.content.res.Configuration.SCREENLAYOUT_SIZE_MASK -import android.net.VpnService import android.os.Bundle import android.provider.Settings import android.util.Log @@ -117,7 +116,7 @@ class MainActivity : ComponentActivity() { if (granted) { Log.d("VpnPermission", "VPN permission granted") viewModel.setVpnPrepared(true) - App.get().startVPN() + App.get()?.startVPN() } else { Log.d("VpnPermission", "VPN permission denied") viewModel.setVpnPrepared(false) @@ -285,7 +284,7 @@ class MainActivity : ComponentActivity() { private fun login(urlString: String) { // Launch coroutine to listen for state changes. When the user completes login, relaunch // MainActivity to bring the app back to focus. - App.get().applicationScope.launch { + App.get()?.applicationScope?.launch { try { Notifier.state.collect { state -> if (state > Ipn.State.NeedsMachineAuth) { @@ -324,20 +323,20 @@ class MainActivity : ComponentActivity() { override fun onResume() { super.onResume() - val restrictionsManager = - this.getSystemService(Context.RESTRICTIONS_SERVICE) as RestrictionsManager - lifecycleScope.launch(Dispatchers.IO) { MDMSettings.update(App.get(), restrictionsManager) } - } - - override fun onStart() { - super.onStart() + App.get()?.let { + val restrictionsManager = + this.getSystemService(Context.RESTRICTIONS_SERVICE) as RestrictionsManager + lifecycleScope.launch(Dispatchers.IO) { MDMSettings.update(it, restrictionsManager) } + } } override fun onStop() { super.onStop() - val restrictionsManager = - this.getSystemService(Context.RESTRICTIONS_SERVICE) as RestrictionsManager - lifecycleScope.launch(Dispatchers.IO) { MDMSettings.update(App.get(), restrictionsManager) } + App.get()?.let { + val restrictionsManager = + this.getSystemService(Context.RESTRICTIONS_SERVICE) as RestrictionsManager + lifecycleScope.launch(Dispatchers.IO) { MDMSettings.update(it, restrictionsManager) } + } } private fun openApplicationSettings() { diff --git a/android/src/main/java/com/tailscale/ipn/QuickToggleService.java b/android/src/main/java/com/tailscale/ipn/QuickToggleService.java index 7b8355f..227ecfa 100644 --- a/android/src/main/java/com/tailscale/ipn/QuickToggleService.java +++ b/android/src/main/java/com/tailscale/ipn/QuickToggleService.java @@ -64,11 +64,17 @@ public class QuickToggleService extends TileService { public void onClick() { boolean r; synchronized (lock) { - r = UninitializedApp.get().isAbleToStartVPN(); + UninitializedApp app = UninitializedApp.get(); + if (app == null) { + return; + } + r = app.isAbleToStartVPN(); } if (r) { // Get the application to make sure it initializes - App.get(); + if (App.get() == null) { + return; + } onTileClick(); } else { // Start main activity. diff --git a/android/src/main/java/com/tailscale/ipn/StartVPNWorker.java b/android/src/main/java/com/tailscale/ipn/StartVPNWorker.java index d75e3fa..b81b8d9 100644 --- a/android/src/main/java/com/tailscale/ipn/StartVPNWorker.java +++ b/android/src/main/java/com/tailscale/ipn/StartVPNWorker.java @@ -28,8 +28,12 @@ public final class StartVPNWorker extends Worker { @Override public Result doWork() { UninitializedApp app = UninitializedApp.get(); - boolean ableToStartVPN = app.isAbleToStartVPN(); - if (ableToStartVPN) { + if (app == null) { + android.util.Log.e("StartVPNWorker", "App is not yet initialized, returning failure."); + return Result.failure(); + } + + if (app.isAbleToStartVPN()) { if (VpnService.prepare(app) == null) { // We're ready and have permissions, start the VPN app.startVPN(); diff --git a/android/src/main/java/com/tailscale/ipn/StopVPNWorker.java b/android/src/main/java/com/tailscale/ipn/StopVPNWorker.java index 7bb2e17..f2dec5d 100644 --- a/android/src/main/java/com/tailscale/ipn/StopVPNWorker.java +++ b/android/src/main/java/com/tailscale/ipn/StopVPNWorker.java @@ -23,7 +23,13 @@ public final class StopVPNWorker extends Worker { @NonNull @Override public Result doWork() { - UninitializedApp.get().stopVPN(); + UninitializedApp app = UninitializedApp.get(); + if (app == null) { + android.util.Log.e("StopVPNWorker", "App is not yet initialized, returning failure."); + return Result.failure(); + } + + app.stopVPN(); return Result.success(); } } diff --git a/android/src/main/java/com/tailscale/ipn/UseExitNodeWorker.kt b/android/src/main/java/com/tailscale/ipn/UseExitNodeWorker.kt index ff48dc3..31b0e92 100644 --- a/android/src/main/java/com/tailscale/ipn/UseExitNodeWorker.kt +++ b/android/src/main/java/com/tailscale/ipn/UseExitNodeWorker.kt @@ -10,7 +10,6 @@ import androidx.work.CoroutineWorker import androidx.work.Data import androidx.work.WorkerParameters import com.tailscale.ipn.UninitializedApp.Companion.STATUS_CHANNEL_ID -import com.tailscale.ipn.UninitializedApp.Companion.STATUS_EXIT_NODE_FAILURE_NOTIFICATION_ID import com.tailscale.ipn.ui.localapi.Client import com.tailscale.ipn.ui.model.Ipn import com.tailscale.ipn.ui.notifier.Notifier @@ -18,95 +17,97 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job -class UseExitNodeWorker( - appContext: Context, - workerParams: WorkerParameters -) : CoroutineWorker(appContext, workerParams) { - override suspend fun doWork(): Result { - val app = UninitializedApp.get() - suspend fun runAndGetResult(): String? { - val exitNodeName = inputData.getString(EXIT_NODE_NAME) - - val exitNodeId = if (exitNodeName.isNullOrEmpty()) { - null - } else { - if (!app.isAbleToStartVPN()) { - return app.getString(R.string.vpn_is_not_ready_to_start) - } - - val peers = - (Notifier.netmap.value - ?: run { return@runAndGetResult app.getString(R.string.tailscale_is_not_setup) }) - .Peers ?: run { return@runAndGetResult app.getString(R.string.no_peers_found) } - - val filteredPeers = peers.filter { - it.displayName == exitNodeName - }.toList() - - if (filteredPeers.isEmpty()) { - return app.getString(R.string.no_peers_with_name_found, exitNodeName) - } else if (filteredPeers.size > 1) { - return app.getString(R.string.multiple_peers_with_name_found, exitNodeName) - } else if (!filteredPeers[0].isExitNode) { - return app.getString( - R.string.peer_with_name_is_not_an_exit_node, - exitNodeName - ) - } - - filteredPeers[0].StableID - } +class UseExitNodeWorker(appContext: Context, workerParams: WorkerParameters) : + CoroutineWorker(appContext, workerParams) { + override suspend fun doWork(): Result { + val app = UninitializedApp.get() ?: return Result.failure() - val allowLanAccess = inputData.getBoolean(ALLOW_LAN_ACCESS, false) - val prefsOut = Ipn.MaskedPrefs() - prefsOut.ExitNodeID = exitNodeId - prefsOut.ExitNodeAllowLANAccess = allowLanAccess - - val scope = CoroutineScope(Dispatchers.Default + Job()) - var result: String? = null - Client(scope).editPrefs(prefsOut) { - result = if (it.isFailure) { - it.exceptionOrNull()?.message - } else { - null - } - } + suspend fun runAndGetResult(): String? { + val exitNodeName = inputData.getString(EXIT_NODE_NAME) - scope.coroutineContext[Job]?.join() + val exitNodeId = + if (exitNodeName.isNullOrEmpty()) { + null + } else { + if (app.isAbleToStartVPN() != true) { + return app.getString(R.string.vpn_is_not_ready_to_start) + } - return result - } + val peers = + (Notifier.netmap.value + ?: run { + return@runAndGetResult app.getString(R.string.tailscale_is_not_setup) + }) + .Peers + ?: run { + return@runAndGetResult app.getString(R.string.no_peers_found) + } + + val filteredPeers = peers.filter { it.displayName == exitNodeName }.toList() + + if (filteredPeers.isEmpty()) { + return app.getString(R.string.no_peers_with_name_found, exitNodeName) + } else if (filteredPeers.size > 1) { + return app.getString(R.string.multiple_peers_with_name_found, exitNodeName) + } else if (!filteredPeers[0].isExitNode) { + return app.getString(R.string.peer_with_name_is_not_an_exit_node, exitNodeName) + } - val result = runAndGetResult() + filteredPeers[0].StableID + } - return if (result != null) { - val intent = - Intent(app, MainActivity::class.java).apply { - flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK - } - val pendingIntent: PendingIntent = - PendingIntent.getActivity( - app, 1, intent, PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE) + val allowLanAccess = inputData.getBoolean(ALLOW_LAN_ACCESS, false) + val prefsOut = Ipn.MaskedPrefs() + prefsOut.ExitNodeID = exitNodeId + prefsOut.ExitNodeAllowLANAccess = allowLanAccess - val notification = NotificationCompat.Builder(app, STATUS_CHANNEL_ID) - .setSmallIcon(R.drawable.ic_notification) - .setContentTitle(app.getString(R.string.use_exit_node_intent_failed)) - .setContentText(result) - .setPriority(NotificationCompat.PRIORITY_DEFAULT) - .setContentIntent(pendingIntent) - .build() + val scope = CoroutineScope(Dispatchers.Default + Job()) + var result: String? = null + Client(scope).editPrefs(prefsOut) { + result = + if (it.isFailure) { + it.exceptionOrNull()?.message + } else { + null + } + } - app.notifyStatus(notification) + scope.coroutineContext[Job]?.join() - Result.failure(Data.Builder().putString(ERROR_KEY, result).build()) - } else { - Result.success() - } + return result } - companion object { - const val EXIT_NODE_NAME = "EXIT_NODE_NAME" - const val ALLOW_LAN_ACCESS = "ALLOW_LAN_ACCESS" - const val ERROR_KEY = "error" + val result = runAndGetResult() + + return if (result != null) { + val intent = + Intent(app, MainActivity::class.java).apply { + flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK + } + val pendingIntent: PendingIntent = + PendingIntent.getActivity( + app, 1, intent, PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE) + + val notification = + NotificationCompat.Builder(app, STATUS_CHANNEL_ID) + .setSmallIcon(R.drawable.ic_notification) + .setContentTitle(app.getString(R.string.use_exit_node_intent_failed)) + .setContentText(result) + .setPriority(NotificationCompat.PRIORITY_DEFAULT) + .setContentIntent(pendingIntent) + .build() + + app.notifyStatus(notification) + + Result.failure(Data.Builder().putString(ERROR_KEY, result).build()) + } else { + Result.success() } + } + + companion object { + const val EXIT_NODE_NAME = "EXIT_NODE_NAME" + const val ALLOW_LAN_ACCESS = "ALLOW_LAN_ACCESS" + const val ERROR_KEY = "error" + } } diff --git a/android/src/main/java/com/tailscale/ipn/mdm/MDMSettingsDefinitions.kt b/android/src/main/java/com/tailscale/ipn/mdm/MDMSettingsDefinitions.kt index 5c2cee4..07aa64f 100644 --- a/android/src/main/java/com/tailscale/ipn/mdm/MDMSettingsDefinitions.kt +++ b/android/src/main/java/com/tailscale/ipn/mdm/MDMSettingsDefinitions.kt @@ -44,7 +44,7 @@ class AlwaysNeverUserDecidesMDMSetting(key: String, localizedTitle: String) : override fun getFrom(bundle: Bundle?, app: App): AlwaysNeverUserDecides { val storedString = bundle?.getString(key) - ?: App.get().getEncryptedPrefs().getString(key, null) + ?: App.get()?.getEncryptedPrefs()?.getString(key, null) ?: "user-decides" return when (storedString) { "always" -> { @@ -64,7 +64,7 @@ class ShowHideMDMSetting(key: String, localizedTitle: String) : MDMSetting(ShowHide.Show, key, localizedTitle) { override fun getFrom(bundle: Bundle?, app: App): ShowHide { val storedString = - bundle?.getString(key) ?: App.get().getEncryptedPrefs().getString(key, null) ?: "show" + bundle?.getString(key) ?: App.get()?.getEncryptedPrefs()?.getString(key, null) ?: "show" return when (storedString) { "hide" -> { ShowHide.Hide diff --git a/android/src/main/java/com/tailscale/ipn/ui/util/AndroidTVUtil.kt b/android/src/main/java/com/tailscale/ipn/ui/util/AndroidTVUtil.kt index bad37f3..48aa884 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/util/AndroidTVUtil.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/util/AndroidTVUtil.kt @@ -14,7 +14,11 @@ import com.tailscale.ipn.ui.util.AndroidTVUtil.isAndroidTV object AndroidTVUtil { fun isAndroidTV(): Boolean { - val pm = UninitializedApp.get().packageManager + // Not ideal, but this is invoked from a Composable, at which point, + // we *will* have an Uninitialized app instance. If this method is being + // called extremely early in the app lifecycle, you may get the wrong + // answer here. + val pm = UninitializedApp.get()?.packageManager ?: return false return (pm.hasSystemFeature(PackageManager.FEATURE_TELEVISION) || pm.hasSystemFeature(PackageManager.FEATURE_LEANBACK)) } diff --git a/android/src/main/java/com/tailscale/ipn/ui/viewModel/IpnViewModel.kt b/android/src/main/java/com/tailscale/ipn/ui/viewModel/IpnViewModel.kt index e0e8be3..5557c29 100644 --- a/android/src/main/java/com/tailscale/ipn/ui/viewModel/IpnViewModel.kt +++ b/android/src/main/java/com/tailscale/ipn/ui/viewModel/IpnViewModel.kt @@ -152,11 +152,11 @@ open class IpnViewModel : ViewModel() { } fun startVPN() { - UninitializedApp.get().startVPN() + UninitializedApp.get()?.startVPN() } fun stopVPN() { - UninitializedApp.get().stopVPN() + UninitializedApp.get()?.stopVPN() } // Login/Logout