From 1cf151d917805471db6b36cb4e51fe6aef5d32ee Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Thu, 11 Aug 2022 01:19:01 +0000 Subject: [PATCH 1/2] Invert the order of event sending and VpnRunner.exit() Invert the order to prevent confusing VPN disconnected notification being sent before the runner actually exits. Bug: 235322391 Test: atest FrameworksNetTests Change-Id: Ie83c5ee35509f6ba10465fd9867c596a956e189a --- .../com/android/server/connectivity/Vpn.java | 54 +++++++++++++++---- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 16a060af66ad1..b374e2c9232b3 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -752,7 +752,7 @@ public class Vpn { return true; } - private boolean sendEventToVpnManagerApp(@NonNull String category, int errorClass, + private Intent buildVpnManagerEventIntent(@NonNull String category, int errorClass, int errorCode, @NonNull final String packageName, @Nullable final String sessionKey, @NonNull final VpnProfileState profileState, @Nullable final Network underlyingNetwork, @Nullable final NetworkCapabilities nc, @Nullable final LinkProperties lp) { @@ -771,6 +771,20 @@ public class Vpn { intent.putExtra(VpnManager.EXTRA_ERROR_CODE, errorCode); } + return intent; + } + + private boolean sendEventToVpnManagerApp(@NonNull String category, int errorClass, + int errorCode, @NonNull final String packageName, @Nullable final String sessionKey, + @NonNull final VpnProfileState profileState, @Nullable final Network underlyingNetwork, + @Nullable final NetworkCapabilities nc, @Nullable final LinkProperties lp) { + final Intent intent = buildVpnManagerEventIntent(category, errorClass, errorCode, + packageName, sessionKey, profileState, underlyingNetwork, nc, lp); + return sendEventToVpnManagerApp(intent, packageName); + } + + private boolean sendEventToVpnManagerApp(@NonNull final Intent intent, + @NonNull final String packageName) { // Allow VpnManager app to temporarily run background services to handle this error. // If an app requires anything beyond this grace period, they MUST either declare // themselves as a foreground service, or schedule a job/workitem. @@ -1182,12 +1196,25 @@ public class Vpn { mContext.unbindService(mConnection); cleanupVpnStateLocked(); } else if (mVpnRunner != null) { - if (!VpnConfig.LEGACY_VPN.equals(mPackage)) { - notifyVpnManagerVpnStopped(mPackage, mOwnerUID); + // Build intent first because the sessionKey will be reset after performing + // VpnRunner.exit(). Also, cache mOwnerUID even if ownerUID will not be changed in + // VpnRunner.exit() to prevent design being changed in the future. + // TODO(b/230548427): Remove SDK check once VPN related stuff are decoupled from + // ConnectivityServiceTest. + final int ownerUid = mOwnerUID; + Intent intent = null; + if (SdkLevel.isAtLeastT() && isVpnApp(mPackage)) { + intent = buildVpnManagerEventIntent( + VpnManager.CATEGORY_EVENT_DEACTIVATED_BY_USER, + -1 /* errorClass */, -1 /* errorCode*/, mPackage, + getSessionKeyLocked(), makeVpnProfileStateLocked(), + null /* underlyingNetwork */, null /* nc */, null /* lp */); } - // cleanupVpnStateLocked() is called from mVpnRunner.exit() mVpnRunner.exit(); + if (intent != null && isVpnApp(mPackage)) { + notifyVpnManagerVpnStopped(mPackage, ownerUid, intent); + } } try { @@ -4042,13 +4069,23 @@ public class Vpn { // To stop the VPN profile, the caller must be the current prepared package and must be // running an Ikev2VpnProfile. if (isCurrentIkev2VpnLocked(packageName)) { - notifyVpnManagerVpnStopped(packageName, mOwnerUID); + // Build intent first because the sessionKey will be reset after performing + // VpnRunner.exit(). Also, cache mOwnerUID even if ownerUID will not be changed in + // VpnRunner.exit() to prevent design being changed in the future. + final int ownerUid = mOwnerUID; + final Intent intent = buildVpnManagerEventIntent( + VpnManager.CATEGORY_EVENT_DEACTIVATED_BY_USER, + -1 /* errorClass */, -1 /* errorCode*/, packageName, + getSessionKeyLocked(), makeVpnProfileStateLocked(), + null /* underlyingNetwork */, null /* nc */, null /* lp */); mVpnRunner.exit(); + notifyVpnManagerVpnStopped(packageName, ownerUid, intent); } } - private synchronized void notifyVpnManagerVpnStopped(String packageName, int ownerUID) { + private synchronized void notifyVpnManagerVpnStopped(String packageName, int ownerUID, + Intent intent) { mAppOpsManager.finishOp( AppOpsManager.OPSTR_ESTABLISH_VPN_MANAGER, ownerUID, packageName, null); // The underlying network, NetworkCapabilities and LinkProperties are not @@ -4057,10 +4094,7 @@ public class Vpn { // TODO(b/230548427): Remove SDK check once VPN related stuff are decoupled from // ConnectivityServiceTest. if (SdkLevel.isAtLeastT()) { - sendEventToVpnManagerApp(VpnManager.CATEGORY_EVENT_DEACTIVATED_BY_USER, - -1 /* errorClass */, -1 /* errorCode*/, packageName, - getSessionKeyLocked(), makeVpnProfileStateLocked(), - null /* underlyingNetwork */, null /* nc */, null /* lp */); + sendEventToVpnManagerApp(intent, packageName); } } From 5a0ea79c41350e0764d6ab546e60a84de2b66509 Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Tue, 16 Aug 2022 08:13:00 +0000 Subject: [PATCH 2/2] 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).