From 5a0ea79c41350e0764d6ab546e60a84de2b66509 Mon Sep 17 00:00:00 2001
From: chiachangwang <chiachangwang@google.com>
Date: Tue, 16 Aug 2022 08:13:00 +0000
Subject: [PATCH] Skip events on stale Ikev2VpnRunner

The exit() of of VpnRunner will do exitVpnRunner() and
cleanupVpnStateLocked(). But the exitVpnRunner() posts a runnable
to the executor. This means that disconnectVpnRunner() might run
concurrently with cleanupVpnStateLocked(), and it might even
complete after cleanupVpnStateLocked() finishes. After exiting
the runner, the states are reset. The remaining events in the
executor are irrelavent and should be skipped to prevent
accessing any of the outer class's members.

Also add missing synchronized block by verify that there is no
other field of the outer Vpn class that is used inside
Ikev2VpnRunner implicitly.

Bug: 235322391
Test: atest FrameworksNetTests
Change-Id: I9eed58b2e96ebaf33e557a42e83525a74a4697d8
---
 .../com/android/server/connectivity/Vpn.java  | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java
index b374e2c9232b3..a148f9c51aabf 100644
--- a/services/core/java/com/android/server/connectivity/Vpn.java
+++ b/services/core/java/com/android/server/connectivity/Vpn.java
@@ -2913,6 +2913,9 @@ public class Vpn {
                 final LinkProperties lp;
 
                 synchronized (Vpn.this) {
+                    // Ignore stale runner.
+                    if (mVpnRunner != this) return;
+
                     mInterface = interfaceName;
                     mConfig.mtu = maxMtu;
                     mConfig.interfaze = mInterface;
@@ -3014,6 +3017,9 @@ public class Vpn {
 
             try {
                 synchronized (Vpn.this) {
+                    // Ignore stale runner.
+                    if (mVpnRunner != this) return;
+
                     mConfig.underlyingNetworks = new Network[] {network};
                     mNetworkCapabilities =
                             new NetworkCapabilities.Builder(mNetworkCapabilities)
@@ -3103,7 +3109,12 @@ public class Vpn {
 
                 // Clear mInterface to prevent Ikev2VpnRunner being cleared when
                 // interfaceRemoved() is called.
-                mInterface = null;
+                synchronized (Vpn.this) {
+                    // Ignore stale runner.
+                    if (mVpnRunner != this) return;
+
+                    mInterface = null;
+                }
                 // Without MOBIKE, we have no way to seamlessly migrate. Close on old
                 // (non-default) network, and start the new one.
                 resetIkeState();
@@ -3288,6 +3299,9 @@ public class Vpn {
         /** Marks the state as FAILED, and disconnects. */
         private void markFailedAndDisconnect(Exception exception) {
             synchronized (Vpn.this) {
+                // Ignore stale runner.
+                if (mVpnRunner != this) return;
+
                 updateState(DetailedState.FAILED, exception.getMessage());
             }
 
@@ -3372,6 +3386,9 @@ public class Vpn {
             }
 
             synchronized (Vpn.this) {
+                // Ignore stale runner.
+                if (mVpnRunner != this) return;
+
                 // TODO(b/230548427): Remove SDK check once VPN related stuff are
                 //  decoupled from ConnectivityServiceTest.
                 if (SdkLevel.isAtLeastT() && category != null && isVpnApp(mPackage)) {
@@ -3398,6 +3415,9 @@ public class Vpn {
             Log.d(TAG, "Resetting state for token: " + mCurrentToken);
 
             synchronized (Vpn.this) {
+                // Ignore stale runner.
+                if (mVpnRunner != this) return;
+
                 // Since this method handles non-fatal errors only, set mInterface to null to
                 // prevent the NetworkManagementEventObserver from killing this VPN based on the
                 // interface going down (which we expect).