From df50bb850ea0cadfe9244c5fcc2f2f139d5cdb25 Mon Sep 17 00:00:00 2001
From: Benedict Wong <benedictwong@google.com>
Date: Fri, 24 Jan 2020 21:15:20 -0800
Subject: [PATCH] Ensure all VPN runners clean up state when exiting

This CL tweaks the cleanup flow to ensure that VPN runners exit
properly and clean up state. Previously, if a VPN exited before an
interface was created and the Interface Observer started watching the
virtual interface, some state (eg mConfig) might not get cleaned up.

Also as a result of this change, the LegacyVpn no longer implicitly
relies on the NetworkManagementEventObserver's watching for interface
removed to cleanup mConfig, mStatusIntent, mNetworkCapabilities, and the
VPN runner itself, but rather clears the state immediately.

Bug: 144246767
Test: FrameworksNetTests passing
Change-Id: Ide9daebca9a3fba025e7da5e3fe1d20d7bfdca02
---
 .../com/android/server/connectivity/Vpn.java  | 65 +++++++++++--------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java
index 3e831d187edc7..8692436f92e4b 100644
--- a/services/core/java/com/android/server/connectivity/Vpn.java
+++ b/services/core/java/com/android/server/connectivity/Vpn.java
@@ -794,10 +794,10 @@ public class Vpn {
                     // ignore
                 }
                 mContext.unbindService(mConnection);
-                mConnection = null;
+                cleanupVpnStateLocked();
             } else if (mVpnRunner != null) {
+                // cleanupVpnStateLocked() is called from mVpnRunner.exit()
                 mVpnRunner.exit();
-                mVpnRunner = null;
             }
 
             try {
@@ -1539,24 +1539,30 @@ public class Vpn {
         public void interfaceRemoved(String interfaze) {
             synchronized (Vpn.this) {
                 if (interfaze.equals(mInterface) && jniCheck(interfaze) == 0) {
-                    mStatusIntent = null;
-                    mNetworkCapabilities.setUids(null);
-                    mConfig = null;
-                    mInterface = null;
                     if (mConnection != null) {
                         mContext.unbindService(mConnection);
-                        mConnection = null;
-                        agentDisconnect();
+                        cleanupVpnStateLocked();
                     } else if (mVpnRunner != null) {
-                        // agentDisconnect must be called from mVpnRunner.exit()
+                        // cleanupVpnStateLocked() is called from mVpnRunner.exit()
                         mVpnRunner.exit();
-                        mVpnRunner = null;
                     }
                 }
             }
         }
     };
 
+    private void cleanupVpnStateLocked() {
+        mStatusIntent = null;
+        mNetworkCapabilities.setUids(null);
+        mConfig = null;
+        mInterface = null;
+
+        // Unconditionally clear both VpnService and VpnRunner fields.
+        mVpnRunner = null;
+        mConnection = null;
+        agentDisconnect();
+    }
+
     private void enforceControlPermission() {
         mContext.enforceCallingPermission(Manifest.permission.CONTROL_VPN, "Unauthorized Caller");
     }
@@ -2046,7 +2052,25 @@ public class Vpn {
 
         public abstract void run();
 
-        protected abstract void exit();
+        /**
+         * Disconnects the NetworkAgent and cleans up all state related to the VpnRunner.
+         *
+         * <p>All outer Vpn instance state is cleaned up in cleanupVpnStateLocked()
+         */
+        protected abstract void exitVpnRunner();
+
+        /**
+         * Triggers the cleanup of the VpnRunner, and additionally cleans up Vpn instance-wide state
+         *
+         * <p>This method ensures that simple calls to exit() will always clean up global state
+         * properly.
+         */
+        protected final void exit() {
+            synchronized (Vpn.this) {
+                exitVpnRunner();
+                cleanupVpnStateLocked();
+            }
+        }
     }
 
     interface IkeV2VpnRunnerCallback {
@@ -2375,17 +2399,6 @@ public class Vpn {
             }
         }
 
-        /**
-         * Triggers cleanup of outer class' state
-         *
-         * <p>Can be called from any thread, as it does not mutate state in the Ikev2VpnRunner.
-         */
-        private void cleanupVpnState() {
-            synchronized (Vpn.this) {
-                agentDisconnect();
-            }
-        }
-
         /**
          * Cleans up all Ikev2VpnRunner internal state
          *
@@ -2405,10 +2418,7 @@ public class Vpn {
         }
 
         @Override
-        public void exit() {
-            // Cleanup outer class' state immediately, otherwise race conditions may ensue.
-            cleanupVpnState();
-
+        public void exitVpnRunner() {
             mExecutor.execute(() -> {
                 shutdownVpnRunner();
             });
@@ -2507,10 +2517,9 @@ public class Vpn {
 
         /** Tears down this LegacyVpn connection */
         @Override
-        public void exit() {
+        public void exitVpnRunner() {
             // We assume that everything is reset after stopping the daemons.
             interrupt();
-            agentDisconnect();
             try {
                 mContext.unregisterReceiver(mBroadcastReceiver);
             } catch (IllegalArgumentException e) {}