From 98d12c7de95ae4ea74028fe32f11293f39fd99c8 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 12 Feb 2021 10:14:01 +0900 Subject: [PATCH] Minor fixes to LockdownVpnTracker. 1. Remove the legacy network type. This is only used for logging. - Replace the logcat logging with the display transport - Remove the EventLogTags logging, since it's likely not actually used by anyone. 2. Remove code that checks for NetworkInfo objects in state FAILED, since LockdownVpnTracker can never have received any such NetworkInfo from ConnectivityService since lollipop. Bug: 173331190 Test: passes existing tests in ConnectivityServiceTest Change-Id: I66ed71e51ba18b95862f3a0a5df2775eecea501e --- .../android/server/ConnectivityService.java | 15 +++++++---- .../server/net/LockdownVpnTracker.java | 27 +++---------------- 2 files changed, 13 insertions(+), 29 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index d744d34085163..94bc44bdf20fd 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -746,7 +746,7 @@ public class ConnectivityService extends IConnectivityManager.Stub + Arrays.toString(vpnNai.declaredUnderlyingNetworks)); return; } - final NetworkAgentInfo underlyingNai = mService.getNetworkAgentInfoForNetwork( + final NetworkAgentInfo underlyingNai = mService.getNetworkAgentInfoForNetwork( vpnNai.declaredUnderlyingNetworks[0]); if (underlyingNai == null) return; @@ -4824,12 +4824,15 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!mLockdownEnabled) { return null; } - // The legacy lockdown VPN always only applies to UID 0. + // The legacy lockdown VPN always only applies to userId 0. final NetworkAgentInfo nai = getVpnForUid(Process.FIRST_APPLICATION_UID); if (nai == null || !isLegacyLockdownNai(nai)) return null; // The legacy lockdown VPN must always have exactly one underlying network. - if (nai.declaredUnderlyingNetworks == null || nai.declaredUnderlyingNetworks.length != 1) { + // This code may run on any thread and declaredUnderlyingNetworks may change, so store it in + // a local variable. There is no need to make a copy because its contents cannot change. + final Network[] underlying = nai.declaredUnderlyingNetworks; + if (underlying == null || underlying.length != 1) { return null; } @@ -4839,8 +4842,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // Report that the VPN is not connected, so when the state of NetworkInfo objects // overwritten by getLegacyLockdownState will be set to CONNECTING and not CONNECTED. final NetworkAgentInfo defaultNetwork = getDefaultNetwork(); - if (defaultNetwork == null - || !defaultNetwork.network.equals(nai.declaredUnderlyingNetworks[0])) { + if (defaultNetwork == null || !defaultNetwork.network.equals(underlying[0])) { return null; } @@ -4899,6 +4901,9 @@ public class ConnectivityService extends IConnectivityManager.Stub private BroadcastReceiver mIntentReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { + // TODO: make BroadcastInterceptingContext use the Handler passed in to registerReceiver + // and put this back. + // ensureRunningOnConnectivityServiceThread(); final String action = intent.getAction(); final int userId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, UserHandle.USER_NULL); diff --git a/services/core/java/com/android/server/net/LockdownVpnTracker.java b/services/core/java/com/android/server/net/LockdownVpnTracker.java index a3d20023ca60a..3cc32bef0e677 100644 --- a/services/core/java/com/android/server/net/LockdownVpnTracker.java +++ b/services/core/java/com/android/server/net/LockdownVpnTracker.java @@ -16,7 +16,6 @@ package com.android.server.net; -import static android.net.ConnectivityManager.TYPE_NONE; import static android.net.NetworkCapabilities.TRANSPORT_VPN; import static android.net.VpnManager.NOTIFICATION_CHANNEL_VPN; import static android.provider.Settings.ACTION_VPN_SETTINGS; @@ -34,7 +33,6 @@ import android.net.LinkAddress; import android.net.LinkProperties; import android.net.Network; import android.net.NetworkInfo; -import android.net.NetworkInfo.DetailedState; import android.net.NetworkRequest; import android.os.Handler; import android.security.KeyStore; @@ -45,7 +43,6 @@ import com.android.internal.R; import com.android.internal.messages.nano.SystemMessageProto.SystemMessage; import com.android.internal.net.VpnConfig; import com.android.internal.net.VpnProfile; -import com.android.server.EventLogTags; import com.android.server.connectivity.Vpn; import java.util.List; @@ -58,9 +55,6 @@ import java.util.Objects; public class LockdownVpnTracker { private static final String TAG = "LockdownVpnTracker"; - /** Number of VPN attempts before waiting for user intervention. */ - private static final int MAX_ERROR_COUNT = 4; - public static final String ACTION_LOCKDOWN_RESET = "com.android.server.action.LOCKDOWN_RESET"; @NonNull private final Context mContext; @@ -83,6 +77,7 @@ public class LockdownVpnTracker { private Network mNetwork = null; private LinkProperties mLinkProperties = null; + @Override public void onLinkPropertiesChanged(Network network, LinkProperties lp) { boolean networkChanged = false; if (!network.equals(mNetwork)) { @@ -100,6 +95,7 @@ public class LockdownVpnTracker { } } + @Override public void onLost(Network network) { // The default network has gone down. mNetwork = null; @@ -134,8 +130,6 @@ public class LockdownVpnTracker { @Nullable private String mAcceptedEgressIface; - private int mErrorCount; - public LockdownVpnTracker(@NonNull Context context, @NonNull Handler handler, @NonNull KeyStore keyStore, @@ -165,7 +159,6 @@ public class LockdownVpnTracker { */ private void handleStateChangedLocked() { final Network network = mDefaultNetworkCallback.getNetwork(); - final NetworkInfo egressInfo = mCm.getNetworkInfo(network); // Only for logging final LinkProperties egressProp = mDefaultNetworkCallback.getLinkProperties(); final NetworkInfo vpnInfo = mVpn.getNetworkInfo(); @@ -176,11 +169,9 @@ public class LockdownVpnTracker { final boolean egressChanged = egressProp == null || !TextUtils.equals(mAcceptedEgressIface, egressProp.getInterfaceName()); - final int egressType = (egressInfo == null) ? TYPE_NONE : egressInfo.getType(); final String egressIface = (egressProp == null) ? null : egressProp.getInterfaceName(); - Log.d(TAG, "handleStateChanged: egress=" + egressType - + " " + mAcceptedEgressIface + "->" + egressIface); + Log.d(TAG, "handleStateChanged: egress=" + mAcceptedEgressIface + "->" + egressIface); if (egressDisconnected || egressChanged) { mAcceptedEgressIface = null; @@ -190,15 +181,6 @@ public class LockdownVpnTracker { hideNotification(); return; } - if (vpnInfo.getDetailedState() == DetailedState.FAILED) { - EventLogTags.writeLockdownVpnError(egressType); - } - - if (mErrorCount > MAX_ERROR_COUNT) { - // Cannot happen because ConnectivityService never sees a NetworkInfo in state FAILED. - showNotification(R.string.vpn_lockdown_error, R.drawable.vpn_disconnected); - return; - } // At this point, |network| is known to be non-null. if (!vpnInfo.isConnectedOrConnecting()) { @@ -209,7 +191,6 @@ public class LockdownVpnTracker { } Log.d(TAG, "Active network connected; starting VPN"); - EventLogTags.writeLockdownVpnConnecting(egressType); showNotification(R.string.vpn_lockdown_connecting, R.drawable.vpn_disconnected); mAcceptedEgressIface = egressIface; @@ -243,7 +224,6 @@ public class LockdownVpnTracker { Log.d(TAG, "VPN connected using iface=" + iface + ", sourceAddr=" + sourceAddrs.toString()); - EventLogTags.writeLockdownVpnConnected(egressType); showNotification(R.string.vpn_lockdown_connected, R.drawable.vpn_connected); } } @@ -280,7 +260,6 @@ public class LockdownVpnTracker { Log.d(TAG, "shutdownLocked()"); mAcceptedEgressIface = null; - mErrorCount = 0; mVpn.stopVpnRunnerPrivileged(); mVpn.setLockdown(false);