From bfabfe0ab321f1d4c13fa02fbfa47ea34d463bee Mon Sep 17 00:00:00 2001 From: Varun Anand Date: Wed, 6 Feb 2019 10:13:38 -0800 Subject: [PATCH 1/2] Update VPN capabilities when its underlying network set is null. Previously, they were only updated when underlying network set was non-null. This change also ensures that all the calls b/w ConnectivityService and Vpn that leads to updating capabilities are on ConnectivityService handler thread. Additionally, it also ensures that capabilities are propagated after VPN enters connected state. This change also updates VPN capabilities inline from ConnectivityService handler thread. Previously, there was an additional loop where Vpn would update capabilities via NetworkAgent thru AsyncChannel which posts back to CS handler thread, which could potentially lead to delays in updating VPN capabilities. Bug: 119129310 Bug: 118856062 Bug: 124268198 Test: atest FrameworksNetTests Test: manual - verified VPNs capabilities are getting updated and DownloadManager is working correctly. (cherry picked from commit 4fa80e8a2f03557221e0371a987e780df7788faa) Change-Id: Iae5f2024b19df04c31938815b52687781d016cde Merged-In: Id0abc4d304bb096e92479a118168690ccce634ed --- .../android/server/ConnectivityService.java | 55 ++++++++- .../server/ConnectivityServiceTest.java | 115 +++++++++++++++--- .../android/server/connectivity/VpnTest.java | 12 +- 3 files changed, 156 insertions(+), 26 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index c9f9ab675a..1c66c5a771 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -867,7 +867,8 @@ public class ConnectivityService extends IConnectivityManager.Stub mPermissionMonitor = new PermissionMonitor(mContext, mNetd); - //set up the listener for user state for creating user VPNs + // Set up the listener for user state for creating user VPNs. + // Should run on mHandler to avoid any races. IntentFilter intentFilter = new IntentFilter(); intentFilter.addAction(Intent.ACTION_USER_STARTED); intentFilter.addAction(Intent.ACTION_USER_STOPPED); @@ -875,7 +876,11 @@ public class ConnectivityService extends IConnectivityManager.Stub intentFilter.addAction(Intent.ACTION_USER_REMOVED); intentFilter.addAction(Intent.ACTION_USER_UNLOCKED); mContext.registerReceiverAsUser( - mUserIntentReceiver, UserHandle.ALL, intentFilter, null, null); + mUserIntentReceiver, + UserHandle.ALL, + intentFilter, + null /* broadcastPermission */, + mHandler); mContext.registerReceiverAsUser(mUserPresentReceiver, UserHandle.SYSTEM, new IntentFilter(Intent.ACTION_USER_PRESENT), null, null); @@ -3815,17 +3820,27 @@ public class ConnectivityService extends IConnectivityManager.Stub * handler thread through their agent, this is asynchronous. When the capabilities objects * are computed they will be up-to-date as they are computed synchronously from here and * this is running on the ConnectivityService thread. - * TODO : Fix this and call updateCapabilities inline to remove out-of-order events. */ private void updateAllVpnsCapabilities() { + Network defaultNetwork = getNetwork(getDefaultNetwork()); synchronized (mVpns) { for (int i = 0; i < mVpns.size(); i++) { final Vpn vpn = mVpns.valueAt(i); - vpn.updateCapabilities(); + NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); + updateVpnCapabilities(vpn, nc); } } } + private void updateVpnCapabilities(Vpn vpn, @Nullable NetworkCapabilities nc) { + ensureRunningOnConnectivityServiceThread(); + NetworkAgentInfo vpnNai = getNetworkAgentInfoForNetId(vpn.getNetId()); + if (vpnNai == null || nc == null) { + return; + } + updateCapabilities(vpnNai.getCurrentScore(), vpnNai, nc); + } + @Override public boolean updateLockdownVpn() { if (Binder.getCallingUid() != Process.SYSTEM_UID) { @@ -4132,21 +4147,27 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void onUserAdded(int userId) { + Network defaultNetwork = getNetwork(getDefaultNetwork()); synchronized (mVpns) { final int vpnsSize = mVpns.size(); for (int i = 0; i < vpnsSize; i++) { Vpn vpn = mVpns.valueAt(i); vpn.onUserAdded(userId); + NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); + updateVpnCapabilities(vpn, nc); } } } private void onUserRemoved(int userId) { + Network defaultNetwork = getNetwork(getDefaultNetwork()); synchronized (mVpns) { final int vpnsSize = mVpns.size(); for (int i = 0; i < vpnsSize; i++) { Vpn vpn = mVpns.valueAt(i); vpn.onUserRemoved(userId); + NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); + updateVpnCapabilities(vpn, nc); } } } @@ -4165,6 +4186,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private BroadcastReceiver mUserIntentReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { + ensureRunningOnConnectivityServiceThread(); final String action = intent.getAction(); final int userId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, UserHandle.USER_NULL); if (userId == UserHandle.USER_NULL) return; @@ -4650,6 +4672,19 @@ public class ConnectivityService extends IConnectivityManager.Stub return getNetworkForRequest(mDefaultRequest.requestId); } + @Nullable + private Network getNetwork(@Nullable NetworkAgentInfo nai) { + return nai != null ? nai.network : null; + } + + private void ensureRunningOnConnectivityServiceThread() { + if (mHandler.getLooper().getThread() != Thread.currentThread()) { + throw new IllegalStateException( + "Not running on ConnectivityService thread: " + + Thread.currentThread().getName()); + } + } + private boolean isDefaultNetwork(NetworkAgentInfo nai) { return nai == getDefaultNetwork(); } @@ -5197,6 +5232,8 @@ public class ConnectivityService extends IConnectivityManager.Stub updateTcpBufferSizes(newNetwork); mDnsManager.setDefaultDnsSystemProperties(newNetwork.linkProperties.getDnsServers()); notifyIfacesChangedForNetworkStats(); + // Fix up the NetworkCapabilities of any VPNs that don't specify underlying networks. + updateAllVpnsCapabilities(); } private void processListenRequests(NetworkAgentInfo nai, boolean capabilitiesChanged) { @@ -5630,6 +5667,10 @@ public class ConnectivityService extends IConnectivityManager.Stub // doing. updateSignalStrengthThresholds(networkAgent, "CONNECT", null); + if (networkAgent.isVPN()) { + updateAllVpnsCapabilities(); + } + // Consider network even though it is not yet validated. final long now = SystemClock.elapsedRealtime(); rematchNetworkAndRequests(networkAgent, ReapUnvalidatedNetworks.REAP, now); @@ -5823,7 +5864,11 @@ public class ConnectivityService extends IConnectivityManager.Stub success = mVpns.get(user).setUnderlyingNetworks(networks); } if (success) { - mHandler.post(() -> notifyIfacesChangedForNetworkStats()); + mHandler.post(() -> { + // Update VPN's capabilities based on updated underlying network set. + updateAllVpnsCapabilities(); + notifyIfacesChangedForNetworkStats(); + }); } return success; } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index c2c627d06e..0dcb21ab83 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -20,6 +20,7 @@ import static android.net.ConnectivityManager.CONNECTIVITY_ACTION; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OFF; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OPPORTUNISTIC; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_PROVIDER_HOSTNAME; +import static android.net.ConnectivityManager.NETID_UNSET; import static android.net.ConnectivityManager.TYPE_ETHERNET; import static android.net.ConnectivityManager.TYPE_MOBILE; import static android.net.ConnectivityManager.TYPE_MOBILE_FOTA; @@ -775,11 +776,14 @@ public class ConnectivityServiceTest { public void setUids(Set uids) { mNetworkCapabilities.setUids(uids); - updateCapabilities(); + updateCapabilities(null /* defaultNetwork */); } @Override public int getNetId() { + if (mMockNetworkAgent == null) { + return NETID_UNSET; + } return mMockNetworkAgent.getNetwork().netId; } @@ -800,12 +804,13 @@ public class ConnectivityServiceTest { } @Override - public void updateCapabilities() { - if (!mConnected) return; - super.updateCapabilities(); - // Because super.updateCapabilities will update the capabilities of the agent but not - // the mock agent, the mock agent needs to know about them. + public NetworkCapabilities updateCapabilities(Network defaultNetwork) { + if (!mConnected) return null; + super.updateCapabilities(defaultNetwork); + // Because super.updateCapabilities will update the capabilities of the agent but + // not the mock agent, the mock agent needs to know about them. copyCapabilitiesToNetworkAgent(); + return new NetworkCapabilities(mNetworkCapabilities); } private void copyCapabilitiesToNetworkAgent() { @@ -4218,6 +4223,7 @@ public class ConnectivityServiceTest { mMockVpn.setUids(ranges); vpnNetworkAgent.connect(false); mMockVpn.connect(); + mMockVpn.setUnderlyingNetworks(new Network[0]); genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -4250,6 +4256,7 @@ public class ConnectivityServiceTest { ranges.add(new UidRange(uid, uid)); mMockVpn.setUids(ranges); + vpnNetworkAgent.setUids(ranges); genericNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -4283,12 +4290,11 @@ public class ConnectivityServiceTest { } @Test - public void testVpnWithAndWithoutInternet() { + public void testVpnWithoutInternet() { final int uid = Process.myUid(); final TestNetworkCallback defaultCallback = new TestNetworkCallback(); mCm.registerDefaultNetworkCallback(defaultCallback); - defaultCallback.assertNoCallback(); mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(true); @@ -4310,11 +4316,30 @@ public class ConnectivityServiceTest { vpnNetworkAgent.disconnect(); defaultCallback.assertNoCallback(); - vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); + mCm.unregisterNetworkCallback(defaultCallback); + } + + @Test + public void testVpnWithInternet() { + final int uid = Process.myUid(); + + final TestNetworkCallback defaultCallback = new TestNetworkCallback(); + mCm.registerDefaultNetworkCallback(defaultCallback); + + mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); + mWiFiNetworkAgent.connect(true); + + defaultCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); + assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); + + MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); + final ArraySet ranges = new ArraySet<>(); + ranges.add(new UidRange(uid, uid)); mMockVpn.setNetworkAgent(vpnNetworkAgent); mMockVpn.setUids(ranges); vpnNetworkAgent.connect(true /* validated */, true /* hasInternet */); mMockVpn.connect(); + defaultCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); @@ -4322,14 +4347,6 @@ public class ConnectivityServiceTest { defaultCallback.expectCallback(CallbackState.LOST, vpnNetworkAgent); defaultCallback.expectAvailableCallbacksValidated(mWiFiNetworkAgent); - vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); - ranges.clear(); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(false /* validated */, true /* hasInternet */); - mMockVpn.connect(); - defaultCallback.assertNoCallback(); - mCm.unregisterNetworkCallback(defaultCallback); } @@ -4430,4 +4447,68 @@ public class ConnectivityServiceTest { mMockVpn.disconnect(); } + + @Test + public void testNullUnderlyingNetworks() { + final int uid = Process.myUid(); + + final TestNetworkCallback vpnNetworkCallback = new TestNetworkCallback(); + final NetworkRequest vpnNetworkRequest = new NetworkRequest.Builder() + .removeCapability(NET_CAPABILITY_NOT_VPN) + .addTransportType(TRANSPORT_VPN) + .build(); + NetworkCapabilities nc; + mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback); + vpnNetworkCallback.assertNoCallback(); + + final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); + final ArraySet ranges = new ArraySet<>(); + ranges.add(new UidRange(uid, uid)); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.connect(); + mMockVpn.setUids(ranges); + vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */); + + vpnNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); + nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); + assertTrue(nc.hasTransport(TRANSPORT_VPN)); + assertFalse(nc.hasTransport(TRANSPORT_CELLULAR)); + assertFalse(nc.hasTransport(TRANSPORT_WIFI)); + // By default, VPN is set to track default network (i.e. its underlying networks is null). + // In case of no default network, VPN is considered metered. + assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_METERED)); + + // Connect to Cell; Cell is the default network. + mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR); + mCellNetworkAgent.connect(true); + + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) + && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + // Connect to WiFi; WiFi is the new default. + mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); + mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); + mWiFiNetworkAgent.connect(true); + + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) + && caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + // Disconnect Cell. The default network did not change, so there shouldn't be any changes in + // the capabilities. + mCellNetworkAgent.disconnect(); + + // Disconnect wifi too. Now we have no default network. + mWiFiNetworkAgent.disconnect(); + + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && !caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) + && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + mMockVpn.disconnect(); + } } diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index e377a47253..a0a4ad1feb 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -457,7 +457,8 @@ public class VpnTest { final NetworkCapabilities caps = new NetworkCapabilities(); - Vpn.updateCapabilities(mConnectivityManager, new Network[] { }, caps); + Vpn.applyUnderlyingCapabilities( + mConnectivityManager, new Network[] {}, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); assertFalse(caps.hasTransport(TRANSPORT_WIFI)); @@ -467,7 +468,8 @@ public class VpnTest { assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.updateCapabilities(mConnectivityManager, new Network[] { mobile }, caps); + Vpn.applyUnderlyingCapabilities( + mConnectivityManager, new Network[] {mobile}, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertTrue(caps.hasTransport(TRANSPORT_CELLULAR)); assertFalse(caps.hasTransport(TRANSPORT_WIFI)); @@ -477,7 +479,8 @@ public class VpnTest { assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.updateCapabilities(mConnectivityManager, new Network[] { wifi }, caps); + Vpn.applyUnderlyingCapabilities( + mConnectivityManager, new Network[] {wifi}, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); assertTrue(caps.hasTransport(TRANSPORT_WIFI)); @@ -487,7 +490,8 @@ public class VpnTest { assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.updateCapabilities(mConnectivityManager, new Network[] { mobile, wifi }, caps); + Vpn.applyUnderlyingCapabilities( + mConnectivityManager, new Network[] {mobile, wifi}, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertTrue(caps.hasTransport(TRANSPORT_CELLULAR)); assertTrue(caps.hasTransport(TRANSPORT_WIFI)); From af8003c10276d58059b600b779f8fff71ea63c44 Mon Sep 17 00:00:00 2001 From: Varun Anand Date: Tue, 5 Mar 2019 19:23:50 +0000 Subject: [PATCH 2/2] Revert "Update VPN capabilities when its underlying network set is null." This reverts commit bfabfe0ab321f1d4c13fa02fbfa47ea34d463bee. Bug: 126245192 Reason for revert: This change can lead to a deadlock that was fixed in http://ag/6580635. However, platform PMs think that fixing this is risky enough as this is not a recent problem and has been in the field for 3/4 of the year. Note: The merged-in tag is used to avoid this change from getting merged into pi-dev-plus-aosp. This is to avoid merge conflicts since we mostly work in aosp/master which merges into pi-dev-plus-aosp. Change-Id: I3814bcec87efb059f50f00617406501aaeac3b4d Merged-In: Id0abc4d304bb096e92479a118168690ccce634ed (cherry picked from commit 6a050c7c50fa0838d7e29b8c6e244018044246db) --- .../android/server/ConnectivityService.java | 55 +-------- .../server/ConnectivityServiceTest.java | 115 +++--------------- .../android/server/connectivity/VpnTest.java | 12 +- 3 files changed, 26 insertions(+), 156 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 1c66c5a771..c9f9ab675a 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -867,8 +867,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mPermissionMonitor = new PermissionMonitor(mContext, mNetd); - // Set up the listener for user state for creating user VPNs. - // Should run on mHandler to avoid any races. + //set up the listener for user state for creating user VPNs IntentFilter intentFilter = new IntentFilter(); intentFilter.addAction(Intent.ACTION_USER_STARTED); intentFilter.addAction(Intent.ACTION_USER_STOPPED); @@ -876,11 +875,7 @@ public class ConnectivityService extends IConnectivityManager.Stub intentFilter.addAction(Intent.ACTION_USER_REMOVED); intentFilter.addAction(Intent.ACTION_USER_UNLOCKED); mContext.registerReceiverAsUser( - mUserIntentReceiver, - UserHandle.ALL, - intentFilter, - null /* broadcastPermission */, - mHandler); + mUserIntentReceiver, UserHandle.ALL, intentFilter, null, null); mContext.registerReceiverAsUser(mUserPresentReceiver, UserHandle.SYSTEM, new IntentFilter(Intent.ACTION_USER_PRESENT), null, null); @@ -3820,27 +3815,17 @@ public class ConnectivityService extends IConnectivityManager.Stub * handler thread through their agent, this is asynchronous. When the capabilities objects * are computed they will be up-to-date as they are computed synchronously from here and * this is running on the ConnectivityService thread. + * TODO : Fix this and call updateCapabilities inline to remove out-of-order events. */ private void updateAllVpnsCapabilities() { - Network defaultNetwork = getNetwork(getDefaultNetwork()); synchronized (mVpns) { for (int i = 0; i < mVpns.size(); i++) { final Vpn vpn = mVpns.valueAt(i); - NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); - updateVpnCapabilities(vpn, nc); + vpn.updateCapabilities(); } } } - private void updateVpnCapabilities(Vpn vpn, @Nullable NetworkCapabilities nc) { - ensureRunningOnConnectivityServiceThread(); - NetworkAgentInfo vpnNai = getNetworkAgentInfoForNetId(vpn.getNetId()); - if (vpnNai == null || nc == null) { - return; - } - updateCapabilities(vpnNai.getCurrentScore(), vpnNai, nc); - } - @Override public boolean updateLockdownVpn() { if (Binder.getCallingUid() != Process.SYSTEM_UID) { @@ -4147,27 +4132,21 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void onUserAdded(int userId) { - Network defaultNetwork = getNetwork(getDefaultNetwork()); synchronized (mVpns) { final int vpnsSize = mVpns.size(); for (int i = 0; i < vpnsSize; i++) { Vpn vpn = mVpns.valueAt(i); vpn.onUserAdded(userId); - NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); - updateVpnCapabilities(vpn, nc); } } } private void onUserRemoved(int userId) { - Network defaultNetwork = getNetwork(getDefaultNetwork()); synchronized (mVpns) { final int vpnsSize = mVpns.size(); for (int i = 0; i < vpnsSize; i++) { Vpn vpn = mVpns.valueAt(i); vpn.onUserRemoved(userId); - NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork); - updateVpnCapabilities(vpn, nc); } } } @@ -4186,7 +4165,6 @@ public class ConnectivityService extends IConnectivityManager.Stub private BroadcastReceiver mUserIntentReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { - ensureRunningOnConnectivityServiceThread(); final String action = intent.getAction(); final int userId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, UserHandle.USER_NULL); if (userId == UserHandle.USER_NULL) return; @@ -4672,19 +4650,6 @@ public class ConnectivityService extends IConnectivityManager.Stub return getNetworkForRequest(mDefaultRequest.requestId); } - @Nullable - private Network getNetwork(@Nullable NetworkAgentInfo nai) { - return nai != null ? nai.network : null; - } - - private void ensureRunningOnConnectivityServiceThread() { - if (mHandler.getLooper().getThread() != Thread.currentThread()) { - throw new IllegalStateException( - "Not running on ConnectivityService thread: " - + Thread.currentThread().getName()); - } - } - private boolean isDefaultNetwork(NetworkAgentInfo nai) { return nai == getDefaultNetwork(); } @@ -5232,8 +5197,6 @@ public class ConnectivityService extends IConnectivityManager.Stub updateTcpBufferSizes(newNetwork); mDnsManager.setDefaultDnsSystemProperties(newNetwork.linkProperties.getDnsServers()); notifyIfacesChangedForNetworkStats(); - // Fix up the NetworkCapabilities of any VPNs that don't specify underlying networks. - updateAllVpnsCapabilities(); } private void processListenRequests(NetworkAgentInfo nai, boolean capabilitiesChanged) { @@ -5667,10 +5630,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // doing. updateSignalStrengthThresholds(networkAgent, "CONNECT", null); - if (networkAgent.isVPN()) { - updateAllVpnsCapabilities(); - } - // Consider network even though it is not yet validated. final long now = SystemClock.elapsedRealtime(); rematchNetworkAndRequests(networkAgent, ReapUnvalidatedNetworks.REAP, now); @@ -5864,11 +5823,7 @@ public class ConnectivityService extends IConnectivityManager.Stub success = mVpns.get(user).setUnderlyingNetworks(networks); } if (success) { - mHandler.post(() -> { - // Update VPN's capabilities based on updated underlying network set. - updateAllVpnsCapabilities(); - notifyIfacesChangedForNetworkStats(); - }); + mHandler.post(() -> notifyIfacesChangedForNetworkStats()); } return success; } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 0dcb21ab83..c2c627d06e 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -20,7 +20,6 @@ import static android.net.ConnectivityManager.CONNECTIVITY_ACTION; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OFF; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OPPORTUNISTIC; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_PROVIDER_HOSTNAME; -import static android.net.ConnectivityManager.NETID_UNSET; import static android.net.ConnectivityManager.TYPE_ETHERNET; import static android.net.ConnectivityManager.TYPE_MOBILE; import static android.net.ConnectivityManager.TYPE_MOBILE_FOTA; @@ -776,14 +775,11 @@ public class ConnectivityServiceTest { public void setUids(Set uids) { mNetworkCapabilities.setUids(uids); - updateCapabilities(null /* defaultNetwork */); + updateCapabilities(); } @Override public int getNetId() { - if (mMockNetworkAgent == null) { - return NETID_UNSET; - } return mMockNetworkAgent.getNetwork().netId; } @@ -804,13 +800,12 @@ public class ConnectivityServiceTest { } @Override - public NetworkCapabilities updateCapabilities(Network defaultNetwork) { - if (!mConnected) return null; - super.updateCapabilities(defaultNetwork); - // Because super.updateCapabilities will update the capabilities of the agent but - // not the mock agent, the mock agent needs to know about them. + public void updateCapabilities() { + if (!mConnected) return; + super.updateCapabilities(); + // Because super.updateCapabilities will update the capabilities of the agent but not + // the mock agent, the mock agent needs to know about them. copyCapabilitiesToNetworkAgent(); - return new NetworkCapabilities(mNetworkCapabilities); } private void copyCapabilitiesToNetworkAgent() { @@ -4223,7 +4218,6 @@ public class ConnectivityServiceTest { mMockVpn.setUids(ranges); vpnNetworkAgent.connect(false); mMockVpn.connect(); - mMockVpn.setUnderlyingNetworks(new Network[0]); genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -4256,7 +4250,6 @@ public class ConnectivityServiceTest { ranges.add(new UidRange(uid, uid)); mMockVpn.setUids(ranges); - vpnNetworkAgent.setUids(ranges); genericNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -4290,11 +4283,12 @@ public class ConnectivityServiceTest { } @Test - public void testVpnWithoutInternet() { + public void testVpnWithAndWithoutInternet() { final int uid = Process.myUid(); final TestNetworkCallback defaultCallback = new TestNetworkCallback(); mCm.registerDefaultNetworkCallback(defaultCallback); + defaultCallback.assertNoCallback(); mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(true); @@ -4316,30 +4310,11 @@ public class ConnectivityServiceTest { vpnNetworkAgent.disconnect(); defaultCallback.assertNoCallback(); - mCm.unregisterNetworkCallback(defaultCallback); - } - - @Test - public void testVpnWithInternet() { - final int uid = Process.myUid(); - - final TestNetworkCallback defaultCallback = new TestNetworkCallback(); - mCm.registerDefaultNetworkCallback(defaultCallback); - - mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); - mWiFiNetworkAgent.connect(true); - - defaultCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); - assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - - MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); + vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); mMockVpn.setNetworkAgent(vpnNetworkAgent); mMockVpn.setUids(ranges); vpnNetworkAgent.connect(true /* validated */, true /* hasInternet */); mMockVpn.connect(); - defaultCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); @@ -4347,6 +4322,14 @@ public class ConnectivityServiceTest { defaultCallback.expectCallback(CallbackState.LOST, vpnNetworkAgent); defaultCallback.expectAvailableCallbacksValidated(mWiFiNetworkAgent); + vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); + ranges.clear(); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); + vpnNetworkAgent.connect(false /* validated */, true /* hasInternet */); + mMockVpn.connect(); + defaultCallback.assertNoCallback(); + mCm.unregisterNetworkCallback(defaultCallback); } @@ -4447,68 +4430,4 @@ public class ConnectivityServiceTest { mMockVpn.disconnect(); } - - @Test - public void testNullUnderlyingNetworks() { - final int uid = Process.myUid(); - - final TestNetworkCallback vpnNetworkCallback = new TestNetworkCallback(); - final NetworkRequest vpnNetworkRequest = new NetworkRequest.Builder() - .removeCapability(NET_CAPABILITY_NOT_VPN) - .addTransportType(TRANSPORT_VPN) - .build(); - NetworkCapabilities nc; - mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback); - vpnNetworkCallback.assertNoCallback(); - - final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.connect(); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */); - - vpnNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); - nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); - assertTrue(nc.hasTransport(TRANSPORT_VPN)); - assertFalse(nc.hasTransport(TRANSPORT_CELLULAR)); - assertFalse(nc.hasTransport(TRANSPORT_WIFI)); - // By default, VPN is set to track default network (i.e. its underlying networks is null). - // In case of no default network, VPN is considered metered. - assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_METERED)); - - // Connect to Cell; Cell is the default network. - mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR); - mCellNetworkAgent.connect(true); - - vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) - && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) - && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), - vpnNetworkAgent); - - // Connect to WiFi; WiFi is the new default. - mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); - mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); - mWiFiNetworkAgent.connect(true); - - vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) - && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) - && caps.hasCapability(NET_CAPABILITY_NOT_METERED), - vpnNetworkAgent); - - // Disconnect Cell. The default network did not change, so there shouldn't be any changes in - // the capabilities. - mCellNetworkAgent.disconnect(); - - // Disconnect wifi too. Now we have no default network. - mWiFiNetworkAgent.disconnect(); - - vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) - && !caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) - && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), - vpnNetworkAgent); - - mMockVpn.disconnect(); - } } diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index a0a4ad1feb..e377a47253 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -457,8 +457,7 @@ public class VpnTest { final NetworkCapabilities caps = new NetworkCapabilities(); - Vpn.applyUnderlyingCapabilities( - mConnectivityManager, new Network[] {}, caps); + Vpn.updateCapabilities(mConnectivityManager, new Network[] { }, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); assertFalse(caps.hasTransport(TRANSPORT_WIFI)); @@ -468,8 +467,7 @@ public class VpnTest { assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.applyUnderlyingCapabilities( - mConnectivityManager, new Network[] {mobile}, caps); + Vpn.updateCapabilities(mConnectivityManager, new Network[] { mobile }, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertTrue(caps.hasTransport(TRANSPORT_CELLULAR)); assertFalse(caps.hasTransport(TRANSPORT_WIFI)); @@ -479,8 +477,7 @@ public class VpnTest { assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.applyUnderlyingCapabilities( - mConnectivityManager, new Network[] {wifi}, caps); + Vpn.updateCapabilities(mConnectivityManager, new Network[] { wifi }, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); assertTrue(caps.hasTransport(TRANSPORT_WIFI)); @@ -490,8 +487,7 @@ public class VpnTest { assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - Vpn.applyUnderlyingCapabilities( - mConnectivityManager, new Network[] {mobile, wifi}, caps); + Vpn.updateCapabilities(mConnectivityManager, new Network[] { mobile, wifi }, caps); assertTrue(caps.hasTransport(TRANSPORT_VPN)); assertTrue(caps.hasTransport(TRANSPORT_CELLULAR)); assertTrue(caps.hasTransport(TRANSPORT_WIFI));