Fix: VPN is off but key is displayed in the status bar

There seem to be a narrow race condition in which it's
possible to turn the VPN off where it won't null out
the config. It's possible to make this happen by trying
to migrate to a network that won't carry the VPN packets
(UDP 4500 or ESP, respectively) and turning off the VPN
while the IKE session is trying to establish.

Alternatively, this might be a visibility issue where
mConfig is nulled out by a thread, but the binder
thread reading it from #getVpnConfig returns the old
object, not yet nulled out. To fix this, make mConfig
guarded by "this", which it almost was already.

Bug: 289187571
Test: manual
      Turning off underlying networks, VPN stays "connected"
      in settings
      Adding a settings VPN, making sure the status is correct
      Try and fail to cause the race condition
(cherry picked from https://android-review.googlesource.com/q/commit:25181986301828f8be5101bb6b08a72dda99b0ad)
Merged-In: I1c19eb33f0c1d0f1ccca6ac17a709f8bc1ad9f94
Change-Id: I1c19eb33f0c1d0f1ccca6ac17a709f8bc1ad9f94

services/core/java/com/android/server/connectivity/Vpn.java

@@ -380,6 +380,7 @@ public class Vpn {
     private final INetworkManagementService mNms;
     private final INetd mNetd;
     @VisibleForTesting
+    @GuardedBy("this")
     protected VpnConfig mConfig;
     private final NetworkProvider mNetworkProvider;
     @VisibleForTesting
@@ -1598,6 +1599,8 @@ public class Vpn {
         return network;
     }
 
+    // TODO : this is not synchronized(this) but reads from mConfig, which is dangerous
+    // This file makes an effort to avoid partly initializing mConfig, but this is still not great
     private LinkProperties makeLinkProperties() {
         // The design of disabling IPv6 is only enabled for IKEv2 VPN because it needs additional
         // logic to handle IPv6 only VPN, and the IPv6 only VPN may be restarted when its MTU
@@ -1679,6 +1682,7 @@ public class Vpn {
      * registering a new NetworkAgent. This is not always possible if the new VPN configuration
      * has certain changes, in which case this method would just return {@code false}.
      */
+    // TODO : this method is not synchronized(this) but reads from mConfig
     private boolean updateLinkPropertiesInPlaceIfPossible(NetworkAgent agent, VpnConfig oldConfig) {
         // NetworkAgentConfig cannot be updated without registering a new NetworkAgent.
         // Strictly speaking, bypassability is affected by lockdown and therefore it's possible
@@ -2269,7 +2273,12 @@ public class Vpn {
      */
     public synchronized VpnConfig getVpnConfig() {
         enforceControlPermission();
-        return mConfig;
+        // Constructor of VpnConfig cannot take a null parameter. Return null directly if mConfig is
+        // null
+        if (mConfig == null) return null;
+        // mConfig is guarded by "this" and can be modified by another thread as soon as
+        // this method returns, so this method must return a copy.
+        return new VpnConfig(mConfig);
     }
 
     @Deprecated
@@ -2315,6 +2324,7 @@ public class Vpn {
         }
     };
 
+    @GuardedBy("this")
     private void cleanupVpnStateLocked() {
         mStatusIntent = null;
         resetNetworkCapabilities();
@@ -2837,9 +2847,7 @@ public class Vpn {
         }
 
         final boolean isLegacyVpn = mVpnRunner instanceof LegacyVpnRunner;
-
         mVpnRunner.exit();
-        mVpnRunner = null;
 
         // LegacyVpn uses daemons that must be shut down before new ones are brought up.
         // The same limitation does not apply to Platform VPNs.
@@ -3084,6 +3092,7 @@ public class Vpn {
                     }
         };
 
+        // GuardedBy("Vpn.this") (annotation can't be applied to constructor)
         IkeV2VpnRunner(
                 @NonNull Ikev2VpnProfile profile, @NonNull ScheduledThreadPoolExecutor executor) {
             super(TAG);
@@ -3710,11 +3719,14 @@ public class Vpn {
         }
 
         public void updateVpnTransportInfoAndNetCap(int keepaliveDelaySec) {
-            final VpnTransportInfo info = new VpnTransportInfo(
+            final VpnTransportInfo info;
-                    getActiveVpnType(),
+            synchronized (Vpn.this) {
-                    mConfig.session,
+                info = new VpnTransportInfo(
-                    mConfig.allowBypass && !mLockdown,
+                        getActiveVpnType(),
-                    areLongLivedTcpConnectionsExpensive(keepaliveDelaySec));
+                        mConfig.session,
+                        mConfig.allowBypass && !mLockdown,
+                        areLongLivedTcpConnectionsExpensive(keepaliveDelaySec));
+            }
             final boolean ncUpdateRequired = !info.equals(mNetworkCapabilities.getTransportInfo());
             if (ncUpdateRequired) {
                 mNetworkCapabilities = new NetworkCapabilities.Builder(mNetworkCapabilities)
@@ -4220,7 +4232,7 @@ public class Vpn {
          * consistency of the Ikev2VpnRunner fields.
          */
         private void disconnectVpnRunner() {
-            mEventChanges.log("[VPNRunner] Disconnect runner, underlying network" + mActiveNetwork);
+            mEventChanges.log("[VPNRunner] Disconnect runner, underlying net " + mActiveNetwork);
             mActiveNetwork = null;
             mUnderlyingNetworkCapabilities = null;
             mUnderlyingLinkProperties = null;
@@ -4293,6 +4305,7 @@ public class Vpn {
             }
         };
 
+        // GuardedBy("Vpn.this") (annotation can't be applied to constructor)
         LegacyVpnRunner(VpnConfig config, String[] racoon, String[] mtpd, VpnProfile profile) {
             super(TAG);
             if (racoon == null && mtpd == null) {
@@ -4500,46 +4513,46 @@ public class Vpn {
                 }
 
                 // Set the interface and the addresses in the config.
-                mConfig.interfaze = parameters[0].trim();
-
-                mConfig.addLegacyAddresses(parameters[1]);
-                // Set the routes if they are not set in the config.
-                if (mConfig.routes == null || mConfig.routes.isEmpty()) {
-                    mConfig.addLegacyRoutes(parameters[2]);
-                }
-
-                // Set the DNS servers if they are not set in the config.
-                if (mConfig.dnsServers == null || mConfig.dnsServers.size() == 0) {
-                    String dnsServers = parameters[3].trim();
-                    if (!dnsServers.isEmpty()) {
-                        mConfig.dnsServers = Arrays.asList(dnsServers.split(" "));
-                    }
-                }
-
-                // Set the search domains if they are not set in the config.
-                if (mConfig.searchDomains == null || mConfig.searchDomains.size() == 0) {
-                    String searchDomains = parameters[4].trim();
-                    if (!searchDomains.isEmpty()) {
-                        mConfig.searchDomains = Arrays.asList(searchDomains.split(" "));
-                    }
-                }
-
-                // Add a throw route for the VPN server endpoint, if one was specified.
-                if (endpointAddress instanceof Inet4Address) {
-                    mConfig.routes.add(new RouteInfo(
-                            new IpPrefix(endpointAddress, 32), null /*gateway*/,
-                            null /*iface*/, RTN_THROW));
-                } else if (endpointAddress instanceof Inet6Address) {
-                    mConfig.routes.add(new RouteInfo(
-                            new IpPrefix(endpointAddress, 128), null /*gateway*/,
-                            null /*iface*/, RTN_THROW));
-                } else {
-                    Log.e(TAG, "Unknown IP address family for VPN endpoint: "
-                            + endpointAddress);
-                }
-
-                // Here is the last step and it must be done synchronously.
                 synchronized (Vpn.this) {
+                    mConfig.interfaze = parameters[0].trim();
+
+                    mConfig.addLegacyAddresses(parameters[1]);
+                    // Set the routes if they are not set in the config.
+                    if (mConfig.routes == null || mConfig.routes.isEmpty()) {
+                        mConfig.addLegacyRoutes(parameters[2]);
+                    }
+
+                    // Set the DNS servers if they are not set in the config.
+                    if (mConfig.dnsServers == null || mConfig.dnsServers.size() == 0) {
+                        String dnsServers = parameters[3].trim();
+                        if (!dnsServers.isEmpty()) {
+                            mConfig.dnsServers = Arrays.asList(dnsServers.split(" "));
+                        }
+                    }
+
+                    // Set the search domains if they are not set in the config.
+                    if (mConfig.searchDomains == null || mConfig.searchDomains.size() == 0) {
+                        String searchDomains = parameters[4].trim();
+                        if (!searchDomains.isEmpty()) {
+                            mConfig.searchDomains = Arrays.asList(searchDomains.split(" "));
+                        }
+                    }
+
+                    // Add a throw route for the VPN server endpoint, if one was specified.
+                    if (endpointAddress instanceof Inet4Address) {
+                        mConfig.routes.add(new RouteInfo(
+                                new IpPrefix(endpointAddress, 32), null /*gateway*/,
+                                null /*iface*/, RTN_THROW));
+                    } else if (endpointAddress instanceof Inet6Address) {
+                        mConfig.routes.add(new RouteInfo(
+                                new IpPrefix(endpointAddress, 128), null /*gateway*/,
+                                null /*iface*/, RTN_THROW));
+                    } else {
+                        Log.e(TAG, "Unknown IP address family for VPN endpoint: "
+                                + endpointAddress);
+                    }
+
+                    // Here is the last step and it must be done synchronously.
                     // Set the start time
                     mConfig.startTime = SystemClock.elapsedRealtime();
 
@@ -4773,25 +4786,26 @@ public class Vpn {
 
         try {
             // Build basic config
-            mConfig = new VpnConfig();
+            final VpnConfig config = new VpnConfig();
             if (VpnConfig.LEGACY_VPN.equals(packageName)) {
-                mConfig.legacy = true;
+                config.legacy = true;
-                mConfig.session = profile.name;
+                config.session = profile.name;
-                mConfig.user = profile.key;
+                config.user = profile.key;
 
                 // TODO: Add support for configuring meteredness via Settings. Until then, use a
                 // safe default.
-                mConfig.isMetered = true;
+                config.isMetered = true;
             } else {
-                mConfig.user = packageName;
+                config.user = packageName;
-                mConfig.isMetered = profile.isMetered;
+                config.isMetered = profile.isMetered;
             }
-            mConfig.startTime = SystemClock.elapsedRealtime();
+            config.startTime = SystemClock.elapsedRealtime();
-            mConfig.proxyInfo = profile.proxy;
+            config.proxyInfo = profile.proxy;
-            mConfig.requiresInternetValidation = profile.requiresInternetValidation;
+            config.requiresInternetValidation = profile.requiresInternetValidation;
-            mConfig.excludeLocalRoutes = profile.excludeLocalRoutes;
+            config.excludeLocalRoutes = profile.excludeLocalRoutes;
-            mConfig.allowBypass = profile.isBypassable;
+            config.allowBypass = profile.isBypassable;
-            mConfig.disallowedApplications = getAppExclusionList(mPackage);
+            config.disallowedApplications = getAppExclusionList(mPackage);
+            mConfig = config;
 
             switch (profile.type) {
                 case VpnProfile.TYPE_IKEV2_IPSEC_USER_PASS:
@@ -4805,6 +4819,7 @@ public class Vpn {
                     mVpnRunner.start();
                     break;
                 default:
+                    mConfig = null;
                     updateState(DetailedState.FAILED, "Invalid platform VPN type");
                     Log.d(TAG, "Unknown VPN profile type: " + profile.type);
                     break;