From c3106feee74e6e9579efa8401beabd72e81cf3a7 Mon Sep 17 00:00:00 2001 From: junyulai Date: Tue, 19 Mar 2019 18:50:23 +0800 Subject: [PATCH 01/20] Reveal the call trace of failed test cases which run in executors Currently, the fails in testTcpSocketKeepalives are triggered by fail() inside the executor, which is hiding the actual call trace but only message remains. And it made the fail case hard to debug. So this commit is to bubble up the Exception by using a custom functional interface. Bug: 123987272 Test: 1. atest FrameworksNetTests 2. manually fail the test case and see the call trace Change-Id: I125e673938a5e9d1de86f83c1a732227a4bd3207 --- .../server/ConnectivityServiceTest.java | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 92a865a3dc..bda2cb6c6f 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -212,7 +212,6 @@ import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Consumer; import java.util.function.Predicate; /** @@ -4022,8 +4021,13 @@ public class ConnectivityServiceTest { callback3.expectStopped(); } + @FunctionalInterface + private interface ThrowingConsumer { + void accept(T t) throws Exception; + } + // Helper method to prepare the executor and run test - private void runTestWithSerialExecutors(Consumer functor) { + private void runTestWithSerialExecutors(ThrowingConsumer functor) throws Exception { final ExecutorService executorSingleThread = Executors.newSingleThreadExecutor(); final Executor executorInline = (Runnable r) -> r.run(); functor.accept(executorSingleThread); @@ -4032,15 +4036,9 @@ public class ConnectivityServiceTest { } @Test - public void testNattSocketKeepalives() { - runTestWithSerialExecutors(executor -> { - try { - doTestNattSocketKeepalivesWithExecutor(executor); - doTestNattSocketKeepalivesFdWithExecutor(executor); - } catch (Exception e) { - fail(e.getMessage()); - } - }); + public void testNattSocketKeepalives() throws Exception { + runTestWithSerialExecutors(executor -> doTestNattSocketKeepalivesWithExecutor(executor)); + runTestWithSerialExecutors(executor -> doTestNattSocketKeepalivesFdWithExecutor(executor)); } private void doTestNattSocketKeepalivesWithExecutor(Executor executor) throws Exception { @@ -4210,14 +4208,8 @@ public class ConnectivityServiceTest { } @Test - public void testTcpSocketKeepalives() { - runTestWithSerialExecutors(executor -> { - try { - doTestTcpSocketKeepalivesWithExecutor(executor); - } catch (Exception e) { - fail(e.getMessage()); - } - }); + public void testTcpSocketKeepalives() throws Exception { + runTestWithSerialExecutors(executor -> doTestTcpSocketKeepalivesWithExecutor(executor)); } private void doTestTcpSocketKeepalivesWithExecutor(Executor executor) throws Exception { From 9df53363493e268ae6bcdcea51b6fdbfcd0834fd Mon Sep 17 00:00:00 2001 From: lucaslin Date: Tue, 26 Mar 2019 17:49:49 +0800 Subject: [PATCH 02/20] Fix flaky test for ConnectivityServiceTest#testPartialConnectivity There are 2 problems will make testPartialConnectivity flaky: 1. If we call setNetworkValid() before expectCapabilitiesWith(), there may be a timing issue that network will become VALID before NetworkMonitor send PARTIAL_CONNECTIVITY to ConnectivityService. Solution: We should set network to valid after ConnectivityService received NETWORK_TEST_RESULT_PARTIAL_CONNECTIVITY to ensure NetworkMonitor will send PARTIAL_CONNECTIVITY to ConnectivityService first then send VALID. 2. When test case call explicitlySelected(true) first then call connect(true), NetworkMonitor will report the network validation test result twice because ConnectivityServiceTest() will trigger notifyNetworkTested() when setAcceptPartialConnectivity() is called, it may cause a timing that before the second test result send to ConnectivityService, connect() already called setNetworkInvalid. So, NET_CAPABILITY_VALIDATED will be removed and ConnectivityService will trigger onCapabilitiesChanged() unexpectedly. Solution: Don't trigger notifyNetworkTested() when setAcceptPartialConnectivity() is called. If there is needed, use mCm.reportNetworkConnectivity() to report the test result instead. Bug: 128426024 Test: 1. atest FrameworksNetTests: \ ConnectivityServiceTest#testPartialConnectivity \ --generate-new-metrics 1000 Change-Id: I7200528378201a3c7c09a78ff827b41f2741dfa1 --- .../server/ConnectivityServiceTest.java | 55 +++++++++++++------ 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index d3616aa89f..dc62add256 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -495,7 +495,6 @@ public class ConnectivityServiceTest { try { doAnswer(validateAnswer).when(mNetworkMonitor).notifyNetworkConnected(); doAnswer(validateAnswer).when(mNetworkMonitor).forceReevaluation(anyInt()); - doAnswer(validateAnswer).when(mNetworkMonitor).setAcceptPartialConnectivity(); } catch (RemoteException e) { fail(e.getMessage()); } @@ -2550,8 +2549,7 @@ public class ConnectivityServiceTest { verifyActiveNetwork(TRANSPORT_CELLULAR); } - // TODO(b/128426024): deflake and re-enable - // @Test + @Test public void testPartialConnectivity() { // Register network callback. NetworkRequest request = new NetworkRequest.Builder() @@ -2575,20 +2573,24 @@ public class ConnectivityServiceTest { assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); callback.assertNoCallback(); + // With HTTPS probe disabled, NetworkMonitor should pass the network validation with http + // probe. + mWiFiNetworkAgent.setNetworkValid(); // If the user chooses yes to use this partial connectivity wifi, switch the default // network to wifi and check if wifi becomes valid or not. mCm.setAcceptPartialConnectivity(mWiFiNetworkAgent.getNetwork(), true /* accept */, false /* always */); - // With https probe disabled, NetworkMonitor should pass the network validation with http - // probe. - mWiFiNetworkAgent.setNetworkValid(); + // If user accepts partial connectivity network, + // NetworkMonitor#setAcceptPartialConnectivity() should be called too. waitForIdle(); try { - verify(mWiFiNetworkAgent.mNetworkMonitor, - timeout(TIMEOUT_MS).times(1)).setAcceptPartialConnectivity(); + verify(mWiFiNetworkAgent.mNetworkMonitor, times(1)).setAcceptPartialConnectivity(); } catch (RemoteException e) { fail(e.getMessage()); } + // Need a trigger point to let NetworkMonitor tell ConnectivityService that network is + // validated. + mCm.reportNetworkConnectivity(mWiFiNetworkAgent.getNetwork(), true); callback.expectCallback(CallbackState.LOSING, mCellNetworkAgent); NetworkCapabilities nc = callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, mWiFiNetworkAgent); @@ -2618,6 +2620,15 @@ public class ConnectivityServiceTest { // acceptUnvalidated is also used as setting for accepting partial networks. mWiFiNetworkAgent.explicitlySelected(true /* acceptUnvalidated */); mWiFiNetworkAgent.connect(true); + // If user accepted partial connectivity network before, + // NetworkMonitor#setAcceptPartialConnectivity() will be called in + // ConnectivityService#updateNetworkInfo(). + waitForIdle(); + try { + verify(mWiFiNetworkAgent.mNetworkMonitor, times(1)).setAcceptPartialConnectivity(); + } catch (RemoteException e) { + fail(e.getMessage()); + } callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); callback.expectCallback(CallbackState.LOSING, mCellNetworkAgent); nc = callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, mWiFiNetworkAgent); @@ -2632,23 +2643,33 @@ public class ConnectivityServiceTest { // NET_CAPABILITY_PARTIAL_CONNECTIVITY. mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); mWiFiNetworkAgent.explicitlySelected(true /* acceptUnvalidated */); + // Current design cannot send multi-testResult from NetworkMonitor to ConnectivityService. + // So, if user accepts partial connectivity, NetworkMonitor will send PARTIAL_CONNECTIVITY + // to ConnectivityService first then send VALID. Once NetworkMonitor support + // multi-testResult, this test case also need to be changed to meet the new design. mWiFiNetworkAgent.connectWithPartialConnectivity(); - callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); - // TODO: If the user accepted partial connectivity, we shouldn't switch to wifi until - // NetworkMonitor detects partial connectivity - assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); - mWiFiNetworkAgent.setNetworkValid(); + // If user accepted partial connectivity network before, + // NetworkMonitor#setAcceptPartialConnectivity() will be called in + // ConnectivityService#updateNetworkInfo(). waitForIdle(); try { - verify(mWiFiNetworkAgent.mNetworkMonitor, - timeout(TIMEOUT_MS).times(1)).setAcceptPartialConnectivity(); + verify(mWiFiNetworkAgent.mNetworkMonitor, times(1)).setAcceptPartialConnectivity(); } catch (RemoteException e) { fail(e.getMessage()); } + callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); callback.expectCallback(CallbackState.LOSING, mCellNetworkAgent); - callback.expectCapabilitiesWith(NET_CAPABILITY_PARTIAL_CONNECTIVITY, mWiFiNetworkAgent); - // Wifi should be the default network. + // TODO: If the user accepted partial connectivity, we shouldn't switch to wifi until + // NetworkMonitor detects partial connectivity assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + callback.expectCapabilitiesWith(NET_CAPABILITY_PARTIAL_CONNECTIVITY, mWiFiNetworkAgent); + mWiFiNetworkAgent.setNetworkValid(); + // Need a trigger point to let NetworkMonitor tell ConnectivityService that network is + // validated. + mCm.reportNetworkConnectivity(mWiFiNetworkAgent.getNetwork(), true); + callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, mWiFiNetworkAgent); + mWiFiNetworkAgent.disconnect(); + callback.expectCallback(CallbackState.LOST, mWiFiNetworkAgent); } @Test From 0c4742527baad6aebffe46da8087b0e5add13fc0 Mon Sep 17 00:00:00 2001 From: Chenbo Feng Date: Fri, 8 Mar 2019 11:55:12 -0800 Subject: [PATCH 03/20] Fix the INTERNET related permissions Change the INTERNET permission implementation so it only block socket creation when non of the packages under the same uid have internet permission. Fix the UPDATE_DEVICE_STATS permission so only the uid that own the permission can change it. Bug: 111560570 Test: CtsNetTestCasesUpdateStatsPermission CtsNetTestCasesInternetPermission Change-Id: I42385526c191d4429f486cde01293b27fcc1374b --- .../connectivity/PermissionMonitor.java | 64 ++++++++----------- 1 file changed, 28 insertions(+), 36 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/PermissionMonitor.java b/services/core/java/com/android/server/connectivity/PermissionMonitor.java index 123564eb4f..30771eb7df 100644 --- a/services/core/java/com/android/server/connectivity/PermissionMonitor.java +++ b/services/core/java/com/android/server/connectivity/PermissionMonitor.java @@ -22,6 +22,7 @@ import static android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS; import static android.Manifest.permission.INTERNET; import static android.Manifest.permission.NETWORK_STACK; import static android.Manifest.permission.UPDATE_DEVICE_STATS; +import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_GRANTED; import static android.content.pm.PackageManager.GET_PERMISSIONS; import static android.content.pm.PackageManager.MATCH_ANY_USER; import static android.os.Process.INVALID_UID; @@ -43,7 +44,6 @@ import android.os.RemoteException; import android.os.UserHandle; import android.os.UserManager; import android.util.Log; -import android.util.Slog; import android.util.SparseIntArray; import com.android.internal.annotations.VisibleForTesting; @@ -83,41 +83,32 @@ public class PermissionMonitor { private final Map mApps = new HashMap<>(); private class PackageListObserver implements PackageManagerInternal.PackageListObserver { - @Override - public void onPackageAdded(String packageName, int uid) { - final PackageInfo app = getPackageInfo(packageName); - if (app == null) { - Slog.wtf(TAG, "Failed to get information of installed package: " + packageName); - return; - } - if (uid == INVALID_UID) { - Slog.wtf(TAG, "Failed to get the uid of installed package: " + packageName - + "uid: " + uid); - return; - } - if (app.requestedPermissions == null) { - return; - } - sendPackagePermissionsForUid(uid, - getNetdPermissionMask(app.requestedPermissions)); - } - @Override - public void onPackageRemoved(String packageName, int uid) { + private int getPermissionForUid(int uid) { int permission = 0; - // If there are still packages remain under the same uid, check the permission of the - // remaining packages. We only remove the permission for a given uid when all packages - // for that uid no longer have that permission. + // Check all the packages for this UID. The UID has the permission if any of the + // packages in it has the permission. String[] packages = mPackageManager.getPackagesForUid(uid); if (packages != null && packages.length > 0) { for (String name : packages) { final PackageInfo app = getPackageInfo(name); if (app != null && app.requestedPermissions != null) { - permission |= getNetdPermissionMask(app.requestedPermissions); + permission |= getNetdPermissionMask(app.requestedPermissions, + app.requestedPermissionsFlags); } } } - sendPackagePermissionsForUid(uid, permission); + return permission; + } + + @Override + public void onPackageAdded(String packageName, int uid) { + sendPackagePermissionsForUid(uid, getPermissionForUid(uid)); + } + + @Override + public void onPackageRemoved(String packageName, int uid) { + sendPackagePermissionsForUid(uid, getPermissionForUid(uid)); } } @@ -167,12 +158,9 @@ public class PermissionMonitor { } //TODO: unify the management of the permissions into one codepath. - if (app.requestedPermissions != null) { - int otherNetdPerms = getNetdPermissionMask(app.requestedPermissions); - if (otherNetdPerms != 0) { - netdPermsUids.put(uid, netdPermsUids.get(uid) | otherNetdPerms); - } - } + int otherNetdPerms = getNetdPermissionMask(app.requestedPermissions, + app.requestedPermissionsFlags); + netdPermsUids.put(uid, netdPermsUids.get(uid) | otherNetdPerms); } List users = mUserManager.getUsers(true); // exclude dying users @@ -403,13 +391,17 @@ public class PermissionMonitor { } } - private static int getNetdPermissionMask(String[] requestedPermissions) { + private static int getNetdPermissionMask(String[] requestedPermissions, + int[] requestedPermissionsFlags) { int permissions = 0; - for (String permissionName : requestedPermissions) { - if (permissionName.equals(INTERNET)) { + if (requestedPermissions == null || requestedPermissionsFlags == null) return permissions; + for (int i = 0; i < requestedPermissions.length; i++) { + if (requestedPermissions[i].equals(INTERNET) + && ((requestedPermissionsFlags[i] & REQUESTED_PERMISSION_GRANTED) != 0)) { permissions |= INetd.PERMISSION_INTERNET; } - if (permissionName.equals(UPDATE_DEVICE_STATS)) { + if (requestedPermissions[i].equals(UPDATE_DEVICE_STATS) + && ((requestedPermissionsFlags[i] & REQUESTED_PERMISSION_GRANTED) != 0)) { permissions |= INetd.PERMISSION_UPDATE_DEVICE_STATS; } } From d72e9ab720ccc3524650f9472065cd5171d1f067 Mon Sep 17 00:00:00 2001 From: Chenbo Feng Date: Tue, 26 Mar 2019 14:36:34 -0700 Subject: [PATCH 04/20] Get the permission information for native services For native services such as mediaserver and audioserver, the permission information cannot be retrieved from getInstalledPackages. Instead, the high level permission information is avalaible in systemConfigs. With those permission information, netd can store the complete list of uids that have UPDATE_DEVICE_STATS permission. Bug: 128944261 Test: dumpsys netd trafficcontroller Change-Id: I0331d5a3a5b927a351fcfe6689ef1ba2b993db0c --- .../connectivity/PermissionMonitor.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/services/core/java/com/android/server/connectivity/PermissionMonitor.java b/services/core/java/com/android/server/connectivity/PermissionMonitor.java index 30771eb7df..0c559346bc 100644 --- a/services/core/java/com/android/server/connectivity/PermissionMonitor.java +++ b/services/core/java/com/android/server/connectivity/PermissionMonitor.java @@ -43,12 +43,15 @@ import android.os.INetworkManagementService; import android.os.RemoteException; import android.os.UserHandle; import android.os.UserManager; +import android.util.ArraySet; import android.util.Log; +import android.util.SparseArray; import android.util.SparseIntArray; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.ArrayUtils; import com.android.server.LocalServices; +import com.android.server.SystemConfig; import java.util.ArrayList; import java.util.HashMap; @@ -170,6 +173,23 @@ public class PermissionMonitor { } } + final SparseArray> systemPermission = + SystemConfig.getInstance().getSystemPermissions(); + for (int i = 0; i < systemPermission.size(); i++) { + ArraySet perms = systemPermission.valueAt(i); + int uid = systemPermission.keyAt(i); + int netdPermission = 0; + // Get the uids of native services that have UPDATE_DEVICE_STATS permission. + if (perms != null) { + netdPermission |= perms.contains(UPDATE_DEVICE_STATS) + ? INetd.PERMISSION_UPDATE_DEVICE_STATS : 0; + } + // For internet permission, the native services have their own selinux domains and + // sepolicy will control the socket creation during run time. netd cannot block the + // socket creation based on the permission information here. + netdPermission |= INetd.PERMISSION_INTERNET; + netdPermsUids.put(uid, netdPermsUids.get(uid) | netdPermission); + } log("Users: " + mUsers.size() + ", Apps: " + mApps.size()); update(mUsers, mApps, true); sendPackagePermissionsToNetd(netdPermsUids); From 8b381dd91ba7e88b065cb4a0f725f00846adaa0a Mon Sep 17 00:00:00 2001 From: junyulai Date: Wed, 27 Mar 2019 10:31:11 +0800 Subject: [PATCH 05/20] Fix keepalive don't get removed when lower layer error Currently, if the lower layer, e.g. wifi, didn't successfully start keepalive by any reason. Due to the startedState changed to NOT_STARTED first, the logic inside stop() will skip the removing process and cause leak. Thus, moving the changing of startedState to proper place first to unblock subsequent changes first. Bug: 123988249 Bug: 129371366 Test: atest FrameworksNetTests Change-Id: I4bba01bacc80e1dac2023ef831b5ade5501894e4 --- .../java/com/android/server/connectivity/KeepaliveTracker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java index ce887eb4f0..722c1a0d9a 100644 --- a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java +++ b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java @@ -480,7 +480,6 @@ public class KeepaliveTracker { } } else { // Keepalive successfully stopped, or error. - ki.mStartedState = KeepaliveInfo.NOT_STARTED; if (reason == SUCCESS) { // The message indicated success stopping : don't call handleStopKeepalive. if (DBG) Log.d(TAG, "Successfully stopped keepalive " + slot + " on " + nai.name()); @@ -490,6 +489,7 @@ public class KeepaliveTracker { handleStopKeepalive(nai, slot, reason); if (DBG) Log.d(TAG, "Keepalive " + slot + " on " + nai.name() + " error " + reason); } + ki.mStartedState = KeepaliveInfo.NOT_STARTED; } } From 5b5447e421f28cc7088d3b0a89a8a80e16c42e91 Mon Sep 17 00:00:00 2001 From: chen xu Date: Tue, 26 Mar 2019 18:24:43 -0700 Subject: [PATCH 06/20] support msim for captiveportal notification Bug: 123025093 Test: Manual Change-Id: Ie04f3ae9a825ab75077a94b108ac92075b6d4753 --- .../NetworkNotificationManager.java | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java b/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java index 828a1e5886..ac3d6def6f 100644 --- a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java +++ b/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java @@ -19,6 +19,7 @@ package com.android.server.connectivity; import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; +import static android.telephony.SubscriptionManager.DEFAULT_SUBSCRIPTION_ID; import android.app.Notification; import android.app.NotificationManager; @@ -26,9 +27,12 @@ import android.app.PendingIntent; import android.content.Context; import android.content.Intent; import android.content.res.Resources; +import android.net.NetworkSpecifier; +import android.net.StringNetworkSpecifier; import android.net.wifi.WifiInfo; import android.os.UserHandle; import android.telephony.AccessNetworkConstants.TransportType; +import android.telephony.SubscriptionManager; import android.telephony.TelephonyManager; import android.text.TextUtils; import android.util.Slog; @@ -195,7 +199,20 @@ public class NetworkNotificationManager { title = r.getString(R.string.network_available_sign_in, 0); // TODO: Change this to pull from NetworkInfo once a printable // name has been added to it - details = mTelephonyManager.getNetworkOperatorName(); + NetworkSpecifier specifier = nai.networkCapabilities.getNetworkSpecifier(); + int subId = SubscriptionManager.DEFAULT_SUBSCRIPTION_ID; + if (specifier instanceof StringNetworkSpecifier) { + try { + subId = Integer.parseInt( + ((StringNetworkSpecifier) specifier).specifier); + } catch (NumberFormatException e) { + Slog.e(TAG, "NumberFormatException on " + + ((StringNetworkSpecifier) specifier).specifier); + } + } + + details = mTelephonyManager.createForSubscriptionId(subId) + .getNetworkOperatorName(); break; default: title = r.getString(R.string.network_available_sign_in, 0); From 8141319b4d1801b0194f194dfd6b82940eafff51 Mon Sep 17 00:00:00 2001 From: Luke Huang Date: Sat, 16 Mar 2019 00:31:46 +0800 Subject: [PATCH 07/20] Use IDnsResolver instead of INetd for resolver related binder commands migrate resolver related commands from INetd to IDnsResolver Bug: 126141549 Test: atest FrameworksNetTests ConnectivityServiceTest Nat464XlatTest atest DnsManagerTest Change-Id: I559c0c1304d53dde408c062e1a52e742595e7cbe --- .../android/server/ConnectivityService.java | 26 +++-- .../server/connectivity/DnsManager.java | 24 ++-- .../server/connectivity/Nat464Xlat.java | 10 +- .../server/connectivity/NetworkAgentInfo.java | 5 +- .../server/ConnectivityServiceTest.java | 103 +++++++++++------- .../server/connectivity/DnsManagerTest.java | 6 +- .../connectivity/LingerMonitorTest.java | 4 +- .../server/connectivity/Nat464XlatTest.java | 12 +- 8 files changed, 120 insertions(+), 70 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index bca2df47cd..c559191401 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -63,6 +63,7 @@ import android.net.ConnectionInfo; import android.net.ConnectivityManager; import android.net.ICaptivePortal; import android.net.IConnectivityManager; +import android.net.IDnsResolver; import android.net.IIpConnectivityMetrics; import android.net.INetd; import android.net.INetdEventCallback; @@ -294,6 +295,8 @@ public class ConnectivityService extends IConnectivityManager.Stub private INetworkManagementService mNMS; @VisibleForTesting + protected IDnsResolver mDnsResolver; + @VisibleForTesting protected INetd mNetd; private INetworkStatsService mStatsService; private INetworkPolicyManager mPolicyManager; @@ -525,6 +528,11 @@ public class ConnectivityService extends IConnectivityManager.Stub return sMagicDecoderRing.get(what, Integer.toString(what)); } + private static IDnsResolver getDnsResolver() { + return IDnsResolver.Stub + .asInterface(ServiceManager.getService("dnsresolver")); + } + /** Handler thread used for both of the handlers below. */ @VisibleForTesting protected final HandlerThread mHandlerThread; @@ -810,13 +818,14 @@ public class ConnectivityService extends IConnectivityManager.Stub public ConnectivityService(Context context, INetworkManagementService netManager, INetworkStatsService statsService, INetworkPolicyManager policyManager) { - this(context, netManager, statsService, policyManager, new IpConnectivityLog()); + this(context, netManager, statsService, policyManager, + getDnsResolver(), new IpConnectivityLog()); } @VisibleForTesting protected ConnectivityService(Context context, INetworkManagementService netManager, INetworkStatsService statsService, INetworkPolicyManager policyManager, - IpConnectivityLog logger) { + IDnsResolver dnsresolver, IpConnectivityLog logger) { if (DBG) log("ConnectivityService starting up"); mSystemProperties = getSystemProperties(); @@ -853,6 +862,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mPolicyManagerInternal = checkNotNull( LocalServices.getService(NetworkPolicyManagerInternal.class), "missing NetworkPolicyManagerInternal"); + mDnsResolver = checkNotNull(dnsresolver, "missing IDnsResolver"); mProxyTracker = makeProxyTracker(); mNetd = NetdService.getInstance(); @@ -1006,7 +1016,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mMultipathPolicyTracker = new MultipathPolicyTracker(mContext, mHandler); - mDnsManager = new DnsManager(mContext, mNMS, mSystemProperties); + mDnsManager = new DnsManager(mContext, mDnsResolver, mSystemProperties); registerPrivateDnsSettingsCallbacks(); } @@ -3021,9 +3031,9 @@ public class ConnectivityService extends IConnectivityManager.Stub // NetworkFactories, so network traffic isn't interrupted for an unnecessarily // long time. try { - mNMS.removeNetwork(nai.network.netId); - } catch (Exception e) { - loge("Exception removing network: " + e); + mNetd.networkDestroy(nai.network.netId); + } catch (RemoteException | ServiceSpecificException e) { + loge("Exception destroying network: " + e); } mDnsManager.removeNetwork(nai.network); } @@ -5372,8 +5382,8 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkCapabilities nc = new NetworkCapabilities(networkCapabilities); final NetworkAgentInfo nai = new NetworkAgentInfo(messenger, new AsyncChannel(), new Network(reserveNetId()), new NetworkInfo(networkInfo), lp, nc, currentScore, - mContext, mTrackerHandler, new NetworkMisc(networkMisc), this, mNetd, mNMS, - factorySerialNumber); + mContext, mTrackerHandler, new NetworkMisc(networkMisc), this, mNetd, mDnsResolver, + mNMS, factorySerialNumber); // Make sure the network capabilities reflect what the agent info says. nai.networkCapabilities = mixInCapabilities(nai, nc); final String extraInfo = networkInfo.getExtraInfo(); diff --git a/services/core/java/com/android/server/connectivity/DnsManager.java b/services/core/java/com/android/server/connectivity/DnsManager.java index d8bb635f2c..1913635f80 100644 --- a/services/core/java/com/android/server/connectivity/DnsManager.java +++ b/services/core/java/com/android/server/connectivity/DnsManager.java @@ -30,13 +30,15 @@ import static android.provider.Settings.Global.PRIVATE_DNS_SPECIFIER; import android.content.ContentResolver; import android.content.Context; import android.content.Intent; +import android.net.IDnsResolver; import android.net.LinkProperties; import android.net.Network; import android.net.NetworkUtils; import android.net.Uri; import android.net.shared.PrivateDnsConfig; import android.os.Binder; -import android.os.INetworkManagementService; +import android.os.RemoteException; +import android.os.ServiceSpecificException; import android.os.UserHandle; import android.provider.Settings; import android.text.TextUtils; @@ -229,7 +231,7 @@ public class DnsManager { private final Context mContext; private final ContentResolver mContentResolver; - private final INetworkManagementService mNMS; + private final IDnsResolver mDnsResolver; private final MockableSystemProperties mSystemProperties; // TODO: Replace these Maps with SparseArrays. private final Map mPrivateDnsMap; @@ -243,10 +245,10 @@ public class DnsManager { private String mPrivateDnsMode; private String mPrivateDnsSpecifier; - public DnsManager(Context ctx, INetworkManagementService nms, MockableSystemProperties sp) { + public DnsManager(Context ctx, IDnsResolver dnsResolver, MockableSystemProperties sp) { mContext = ctx; mContentResolver = mContext.getContentResolver(); - mNMS = nms; + mDnsResolver = dnsResolver; mSystemProperties = sp; mPrivateDnsMap = new HashMap<>(); mPrivateDnsValidationMap = new HashMap<>(); @@ -260,6 +262,12 @@ public class DnsManager { } public void removeNetwork(Network network) { + try { + mDnsResolver.clearResolverConfiguration(network.netId); + } catch (RemoteException | ServiceSpecificException e) { + Slog.e(TAG, "Error clearing DNS configuration: " + e); + return; + } mPrivateDnsMap.remove(network.netId); mPrivateDnsValidationMap.remove(network.netId); } @@ -344,10 +352,12 @@ public class DnsManager { Slog.d(TAG, String.format("setDnsConfigurationForNetwork(%d, %s, %s, %s, %s, %s)", netId, Arrays.toString(assignedServers), Arrays.toString(domainStrs), Arrays.toString(params), tlsHostname, Arrays.toString(tlsServers))); + final String[] tlsFingerprints = new String[0]; try { - mNMS.setDnsConfigurationForNetwork( - netId, assignedServers, domainStrs, params, tlsHostname, tlsServers); - } catch (Exception e) { + mDnsResolver.setResolverConfiguration( + netId, assignedServers, domainStrs, params, + tlsHostname, tlsServers, tlsFingerprints); + } catch (RemoteException | ServiceSpecificException e) { Slog.e(TAG, "Error setting DNS configuration: " + e); return; } diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java index 262ba7a475..66bd27c1a7 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java @@ -17,6 +17,7 @@ package com.android.server.connectivity; import android.net.ConnectivityManager; +import android.net.IDnsResolver; import android.net.INetd; import android.net.InetAddresses; import android.net.InterfaceConfiguration; @@ -65,6 +66,7 @@ public class Nat464Xlat extends BaseNetworkObserver { NetworkInfo.State.SUSPENDED, }; + private final IDnsResolver mDnsResolver; private final INetd mNetd; private final INetworkManagementService mNMService; @@ -84,7 +86,9 @@ public class Nat464Xlat extends BaseNetworkObserver { private Inet6Address mIPv6Address; private State mState = State.IDLE; - public Nat464Xlat(NetworkAgentInfo nai, INetd netd, INetworkManagementService nmService) { + public Nat464Xlat(NetworkAgentInfo nai, INetd netd, IDnsResolver dnsResolver, + INetworkManagementService nmService) { + mDnsResolver = dnsResolver; mNetd = netd; mNMService = nmService; mNetwork = nai; @@ -269,7 +273,7 @@ public class Nat464Xlat extends BaseNetworkObserver { private void startPrefixDiscovery() { try { - mNetd.resolverStartPrefix64Discovery(getNetId()); + mDnsResolver.startPrefix64Discovery(getNetId()); mState = State.DISCOVERING; } catch (RemoteException | ServiceSpecificException e) { Slog.e(TAG, "Error starting prefix discovery on netId " + getNetId() + ": " + e); @@ -278,7 +282,7 @@ public class Nat464Xlat extends BaseNetworkObserver { private void stopPrefixDiscovery() { try { - mNetd.resolverStopPrefix64Discovery(getNetId()); + mDnsResolver.stopPrefix64Discovery(getNetId()); } catch (RemoteException | ServiceSpecificException e) { Slog.e(TAG, "Error stopping prefix discovery on netId " + getNetId() + ": " + e); } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 8f2825ca72..e3fdbe84a1 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -17,6 +17,7 @@ package com.android.server.connectivity; import android.content.Context; +import android.net.IDnsResolver; import android.net.INetd; import android.net.INetworkMonitor; import android.net.LinkProperties; @@ -255,7 +256,7 @@ public class NetworkAgentInfo implements Comparable { public NetworkAgentInfo(Messenger messenger, AsyncChannel ac, Network net, NetworkInfo info, LinkProperties lp, NetworkCapabilities nc, int score, Context context, Handler handler, NetworkMisc misc, ConnectivityService connService, INetd netd, - INetworkManagementService nms, int factorySerialNumber) { + IDnsResolver dnsResolver, INetworkManagementService nms, int factorySerialNumber) { this.messenger = messenger; asyncChannel = ac; network = net; @@ -263,7 +264,7 @@ public class NetworkAgentInfo implements Comparable { linkProperties = lp; networkCapabilities = nc; currentScore = score; - clatd = new Nat464Xlat(this, netd, nms); + clatd = new Nat464Xlat(this, netd, dnsResolver, nms); mConnService = connService; mContext = context; mHandler = handler; diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index a95db22423..a478424137 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -104,6 +104,7 @@ import android.net.ConnectivityManager.PacketKeepalive; import android.net.ConnectivityManager.PacketKeepaliveCallback; import android.net.ConnectivityManager.TooManyRequestsException; import android.net.ConnectivityThread; +import android.net.IDnsResolver; import android.net.INetd; import android.net.INetworkMonitor; import android.net.INetworkMonitorCallbacks; @@ -240,6 +241,7 @@ public class ConnectivityServiceTest { private static final String CLAT_PREFIX = "v4-"; private static final String MOBILE_IFNAME = "test_rmnet_data0"; private static final String WIFI_IFNAME = "test_wlan0"; + private static final String[] EMPTY_STRING_ARRAY = new String[0]; private MockContext mServiceContext; private WrappedConnectivityService mService; @@ -256,6 +258,7 @@ public class ConnectivityServiceTest { @Mock INetworkManagementService mNetworkManagementService; @Mock INetworkStatsService mStatsService; @Mock INetworkPolicyManager mNpm; + @Mock IDnsResolver mMockDnsResolver; @Mock INetd mMockNetd; @Mock NetworkStackClient mNetworkStack; @@ -1053,8 +1056,8 @@ public class ConnectivityServiceTest { public WrappedConnectivityService(Context context, INetworkManagementService netManager, INetworkStatsService statsService, INetworkPolicyManager policyManager, - IpConnectivityLog log, INetd netd) { - super(context, netManager, statsService, policyManager, log); + IpConnectivityLog log, INetd netd, IDnsResolver dnsResolver) { + super(context, netManager, statsService, policyManager, dnsResolver, log); mNetd = netd; mLingerDelayMs = TEST_LINGER_DELAY_MS; } @@ -1218,7 +1221,8 @@ public class ConnectivityServiceTest { mStatsService, mNpm, mock(IpConnectivityLog.class), - mMockNetd); + mMockNetd, + mMockDnsResolver); final ArgumentCaptor policyListenerCaptor = ArgumentCaptor.forClass(INetworkPolicyListener.class); @@ -4772,14 +4776,14 @@ public class ConnectivityServiceTest { ArgumentCaptor tlsServers = ArgumentCaptor.forClass(String[].class); // Clear any interactions that occur as a result of CS starting up. - reset(mNetworkManagementService); + reset(mMockDnsResolver); - final String[] EMPTY_STRING_ARRAY = new String[0]; mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR); waitForIdle(); - verify(mNetworkManagementService, never()).setDnsConfigurationForNetwork( - anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), eq(EMPTY_STRING_ARRAY)); - verifyNoMoreInteractions(mNetworkManagementService); + verify(mMockDnsResolver, never()).setResolverConfiguration( + anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), + eq(EMPTY_STRING_ARRAY), eq(EMPTY_STRING_ARRAY)); + verifyNoMoreInteractions(mMockDnsResolver); final LinkProperties cellLp = new LinkProperties(); cellLp.setInterfaceName(MOBILE_IFNAME); @@ -4796,28 +4800,29 @@ public class ConnectivityServiceTest { mCellNetworkAgent.connect(false); waitForIdle(); // CS tells netd about the empty DNS config for this network. - verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork( - anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), eq(EMPTY_STRING_ARRAY)); - reset(mNetworkManagementService); + verify(mMockDnsResolver, atLeastOnce()).setResolverConfiguration( + anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), + eq(EMPTY_STRING_ARRAY), eq(EMPTY_STRING_ARRAY)); + reset(mMockDnsResolver); cellLp.addDnsServer(InetAddress.getByName("2001:db8::1")); mCellNetworkAgent.sendLinkProperties(cellLp); waitForIdle(); - verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork( + verify(mMockDnsResolver, atLeastOnce()).setResolverConfiguration( anyInt(), mStringArrayCaptor.capture(), any(), any(), - eq(""), tlsServers.capture()); + eq(""), tlsServers.capture(), eq(EMPTY_STRING_ARRAY)); assertEquals(1, mStringArrayCaptor.getValue().length); assertTrue(ArrayUtils.contains(mStringArrayCaptor.getValue(), "2001:db8::1")); // Opportunistic mode. assertTrue(ArrayUtils.contains(tlsServers.getValue(), "2001:db8::1")); - reset(mNetworkManagementService); + reset(mMockDnsResolver); cellLp.addDnsServer(InetAddress.getByName("192.0.2.1")); mCellNetworkAgent.sendLinkProperties(cellLp); waitForIdle(); - verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork( + verify(mMockDnsResolver, atLeastOnce()).setResolverConfiguration( anyInt(), mStringArrayCaptor.capture(), any(), any(), - eq(""), tlsServers.capture()); + eq(""), tlsServers.capture(), eq(EMPTY_STRING_ARRAY)); assertEquals(2, mStringArrayCaptor.getValue().length); assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(), new String[]{"2001:db8::1", "192.0.2.1"})); @@ -4825,7 +4830,7 @@ public class ConnectivityServiceTest { assertEquals(2, tlsServers.getValue().length); assertTrue(ArrayUtils.containsAll(tlsServers.getValue(), new String[]{"2001:db8::1", "192.0.2.1"})); - reset(mNetworkManagementService); + reset(mMockDnsResolver); final String TLS_SPECIFIER = "tls.example.com"; final String TLS_SERVER6 = "2001:db8:53::53"; @@ -4835,22 +4840,21 @@ public class ConnectivityServiceTest { new PrivateDnsConfig(TLS_SPECIFIER, TLS_IPS).toParcel()); waitForIdle(); - verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork( + verify(mMockDnsResolver, atLeastOnce()).setResolverConfiguration( anyInt(), mStringArrayCaptor.capture(), any(), any(), - eq(TLS_SPECIFIER), eq(TLS_SERVERS)); + eq(TLS_SPECIFIER), eq(TLS_SERVERS), eq(EMPTY_STRING_ARRAY)); assertEquals(2, mStringArrayCaptor.getValue().length); assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(), new String[]{"2001:db8::1", "192.0.2.1"})); - reset(mNetworkManagementService); + reset(mMockDnsResolver); } @Test public void testPrivateDnsSettingsChange() throws Exception { - final String[] EMPTY_STRING_ARRAY = new String[0]; ArgumentCaptor tlsServers = ArgumentCaptor.forClass(String[].class); // Clear any interactions that occur as a result of CS starting up. - reset(mNetworkManagementService); + reset(mMockDnsResolver); // The default on Android is opportunistic mode ("Automatic"). setPrivateDnsSettings(PRIVATE_DNS_MODE_OPPORTUNISTIC, "ignored.example.com"); @@ -4863,9 +4867,10 @@ public class ConnectivityServiceTest { mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR); waitForIdle(); // CS tells netd about the empty DNS config for this network. - verify(mNetworkManagementService, never()).setDnsConfigurationForNetwork( - anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), eq(EMPTY_STRING_ARRAY)); - verifyNoMoreInteractions(mNetworkManagementService); + verify(mMockDnsResolver, never()).setResolverConfiguration( + anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), + eq(EMPTY_STRING_ARRAY), eq(EMPTY_STRING_ARRAY)); + verifyNoMoreInteractions(mMockDnsResolver); final LinkProperties cellLp = new LinkProperties(); cellLp.setInterfaceName(MOBILE_IFNAME); @@ -4884,9 +4889,9 @@ public class ConnectivityServiceTest { mCellNetworkAgent.sendLinkProperties(cellLp); mCellNetworkAgent.connect(false); waitForIdle(); - verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork( + verify(mMockDnsResolver, atLeastOnce()).setResolverConfiguration( anyInt(), mStringArrayCaptor.capture(), any(), any(), - eq(""), tlsServers.capture()); + eq(""), tlsServers.capture(), eq(EMPTY_STRING_ARRAY)); assertEquals(2, mStringArrayCaptor.getValue().length); assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(), new String[]{"2001:db8::1", "192.0.2.1"})); @@ -4894,7 +4899,7 @@ public class ConnectivityServiceTest { assertEquals(2, tlsServers.getValue().length); assertTrue(ArrayUtils.containsAll(tlsServers.getValue(), new String[]{"2001:db8::1", "192.0.2.1"})); - reset(mNetworkManagementService); + reset(mMockDnsResolver); cellNetworkCallback.expectCallback(CallbackState.AVAILABLE, mCellNetworkAgent); cellNetworkCallback.expectCallback(CallbackState.NETWORK_CAPABILITIES, mCellNetworkAgent); @@ -4906,26 +4911,26 @@ public class ConnectivityServiceTest { assertNull(((LinkProperties)cbi.arg).getPrivateDnsServerName()); setPrivateDnsSettings(PRIVATE_DNS_MODE_OFF, "ignored.example.com"); - verify(mNetworkManagementService, times(1)).setDnsConfigurationForNetwork( + verify(mMockDnsResolver, times(1)).setResolverConfiguration( anyInt(), mStringArrayCaptor.capture(), any(), any(), - eq(""), eq(EMPTY_STRING_ARRAY)); + eq(""), eq(EMPTY_STRING_ARRAY), eq(EMPTY_STRING_ARRAY)); assertEquals(2, mStringArrayCaptor.getValue().length); assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(), new String[]{"2001:db8::1", "192.0.2.1"})); - reset(mNetworkManagementService); + reset(mMockDnsResolver); cellNetworkCallback.assertNoCallback(); setPrivateDnsSettings(PRIVATE_DNS_MODE_OPPORTUNISTIC, "ignored.example.com"); - verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork( + verify(mMockDnsResolver, atLeastOnce()).setResolverConfiguration( anyInt(), mStringArrayCaptor.capture(), any(), any(), - eq(""), tlsServers.capture()); + eq(""), tlsServers.capture(), eq(EMPTY_STRING_ARRAY)); assertEquals(2, mStringArrayCaptor.getValue().length); assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(), new String[]{"2001:db8::1", "192.0.2.1"})); assertEquals(2, tlsServers.getValue().length); assertTrue(ArrayUtils.containsAll(tlsServers.getValue(), new String[]{"2001:db8::1", "192.0.2.1"})); - reset(mNetworkManagementService); + reset(mMockDnsResolver); cellNetworkCallback.assertNoCallback(); setPrivateDnsSettings(PRIVATE_DNS_MODE_PROVIDER_HOSTNAME, "strict.example.com"); @@ -5756,6 +5761,7 @@ public class ConnectivityServiceTest { cellLp.addRoute(new RouteInfo((IpPrefix) null, myIpv6.getAddress(), MOBILE_IFNAME)); cellLp.addRoute(new RouteInfo(myIpv6, null, MOBILE_IFNAME)); reset(mNetworkManagementService); + reset(mMockDnsResolver); when(mNetworkManagementService.getInterfaceConfig(CLAT_PREFIX + MOBILE_IFNAME)) .thenReturn(getClatInterfaceConfig(myIpv4)); @@ -5763,7 +5769,7 @@ public class ConnectivityServiceTest { mCellNetworkAgent.sendLinkProperties(cellLp); mCellNetworkAgent.connect(true); networkCallback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); - verify(mMockNetd, times(1)).resolverStartPrefix64Discovery(cellNetId); + verify(mMockDnsResolver, times(1)).startPrefix64Discovery(cellNetId); // Switching default network updates TCP buffer sizes. verifyTcpBufferSizeChange(ConnectivityService.DEFAULT_TCP_BUFFER_SIZES); @@ -5773,17 +5779,22 @@ public class ConnectivityServiceTest { cellLp.addLinkAddress(myIpv4); mCellNetworkAgent.sendLinkProperties(cellLp); networkCallback.expectCallback(CallbackState.LINK_PROPERTIES, mCellNetworkAgent); - verify(mMockNetd, times(1)).resolverStopPrefix64Discovery(cellNetId); + verify(mMockDnsResolver, times(1)).stopPrefix64Discovery(cellNetId); + verify(mMockDnsResolver, atLeastOnce()).setResolverConfiguration( + eq(cellNetId), eq(EMPTY_STRING_ARRAY), any(), any(), + eq(""), eq(EMPTY_STRING_ARRAY), eq(EMPTY_STRING_ARRAY)); verifyNoMoreInteractions(mMockNetd); + verifyNoMoreInteractions(mMockDnsResolver); reset(mMockNetd); + reset(mMockDnsResolver); // Remove IPv4 address. Expect prefix discovery to be started again. cellLp.removeLinkAddress(myIpv4); cellLp.removeRoute(new RouteInfo(myIpv4, null, MOBILE_IFNAME)); mCellNetworkAgent.sendLinkProperties(cellLp); networkCallback.expectCallback(CallbackState.LINK_PROPERTIES, mCellNetworkAgent); - verify(mMockNetd, times(1)).resolverStartPrefix64Discovery(cellNetId); + verify(mMockDnsResolver, times(1)).startPrefix64Discovery(cellNetId); // When NAT64 prefix discovery succeeds, LinkProperties are updated and clatd is started. Nat464Xlat clat = mService.getNat464Xlat(mCellNetworkAgent); @@ -5813,6 +5824,12 @@ public class ConnectivityServiceTest { assertNotEquals(stackedLpsAfterChange, Collections.EMPTY_LIST); assertEquals(makeClatLinkProperties(myIpv4), stackedLpsAfterChange.get(0)); + verify(mMockDnsResolver, times(1)).setResolverConfiguration( + eq(cellNetId), mStringArrayCaptor.capture(), any(), any(), + eq(""), eq(EMPTY_STRING_ARRAY), eq(EMPTY_STRING_ARRAY)); + assertEquals(1, mStringArrayCaptor.getValue().length); + assertTrue(ArrayUtils.contains(mStringArrayCaptor.getValue(), "8.8.8.8")); + // Add ipv4 address, expect that clatd and prefix discovery are stopped and stacked // linkproperties are cleaned up. cellLp.addLinkAddress(myIpv4); @@ -5820,7 +5837,7 @@ public class ConnectivityServiceTest { mCellNetworkAgent.sendLinkProperties(cellLp); networkCallback.expectCallback(CallbackState.LINK_PROPERTIES, mCellNetworkAgent); verify(mMockNetd, times(1)).clatdStop(MOBILE_IFNAME); - verify(mMockNetd, times(1)).resolverStopPrefix64Discovery(cellNetId); + verify(mMockDnsResolver, times(1)).stopPrefix64Discovery(cellNetId); // As soon as stop is called, the linkproperties lose the stacked interface. networkCallback.expectCallback(CallbackState.LINK_PROPERTIES, mCellNetworkAgent); @@ -5835,7 +5852,9 @@ public class ConnectivityServiceTest { networkCallback.assertNoCallback(); verifyNoMoreInteractions(mMockNetd); + verifyNoMoreInteractions(mMockDnsResolver); reset(mMockNetd); + reset(mMockDnsResolver); // Stopping prefix discovery causes netd to tell us that the NAT64 prefix is gone. mService.mNetdEventCallback.onNat64PrefixEvent(cellNetId, false /* added */, @@ -5849,7 +5868,7 @@ public class ConnectivityServiceTest { cellLp.removeDnsServer(InetAddress.getByName("8.8.8.8")); mCellNetworkAgent.sendLinkProperties(cellLp); networkCallback.expectCallback(CallbackState.LINK_PROPERTIES, mCellNetworkAgent); - verify(mMockNetd, times(1)).resolverStartPrefix64Discovery(cellNetId); + verify(mMockDnsResolver, times(1)).startPrefix64Discovery(cellNetId); mService.mNetdEventCallback.onNat64PrefixEvent(cellNetId, true /* added */, kNat64PrefixString, 96); networkCallback.expectCallback(CallbackState.LINK_PROPERTIES, mCellNetworkAgent); @@ -5932,6 +5951,7 @@ public class ConnectivityServiceTest { // Disconnect cell reset(mNetworkManagementService); + reset(mMockNetd); mCellNetworkAgent.disconnect(); networkCallback.expectCallback(CallbackState.LOST, mCellNetworkAgent); // LOST callback is triggered earlier than removing idle timer. Broadcast should also be @@ -5939,8 +5959,9 @@ public class ConnectivityServiceTest { // unexpectedly before network being removed. waitForIdle(); verify(mNetworkManagementService, times(0)).removeIdleTimer(eq(MOBILE_IFNAME)); - verify(mNetworkManagementService, times(1)).removeNetwork( - eq(mCellNetworkAgent.getNetwork().netId)); + verify(mMockNetd, times(1)).networkDestroy(eq(mCellNetworkAgent.getNetwork().netId)); + verify(mMockDnsResolver, times(1)) + .clearResolverConfiguration(eq(mCellNetworkAgent.getNetwork().netId)); // Disconnect wifi ConditionVariable cv = waitForConnectivityBroadcasts(1); diff --git a/tests/net/java/com/android/server/connectivity/DnsManagerTest.java b/tests/net/java/com/android/server/connectivity/DnsManagerTest.java index 15ba43df83..8fa0ab979a 100644 --- a/tests/net/java/com/android/server/connectivity/DnsManagerTest.java +++ b/tests/net/java/com/android/server/connectivity/DnsManagerTest.java @@ -29,13 +29,13 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.when; import android.content.Context; +import android.net.IDnsResolver; import android.net.IpPrefix; import android.net.LinkAddress; import android.net.LinkProperties; import android.net.Network; import android.net.RouteInfo; import android.net.shared.PrivateDnsConfig; -import android.os.INetworkManagementService; import android.provider.Settings; import android.test.mock.MockContentResolver; @@ -73,7 +73,7 @@ public class DnsManagerTest { MockContentResolver mContentResolver; @Mock Context mCtx; - @Mock INetworkManagementService mNMService; + @Mock IDnsResolver mMockDnsResolver; @Mock MockableSystemProperties mSystemProperties; @Before @@ -83,7 +83,7 @@ public class DnsManagerTest { mContentResolver.addProvider(Settings.AUTHORITY, new FakeSettingsProvider()); when(mCtx.getContentResolver()).thenReturn(mContentResolver); - mDnsManager = new DnsManager(mCtx, mNMService, mSystemProperties); + mDnsManager = new DnsManager(mCtx, mMockDnsResolver, mSystemProperties); // Clear the private DNS settings Settings.Global.putString(mContentResolver, PRIVATE_DNS_DEFAULT_MODE, ""); diff --git a/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java b/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java index 6de4aa1be1..142769f613 100644 --- a/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java +++ b/tests/net/java/com/android/server/connectivity/LingerMonitorTest.java @@ -32,6 +32,7 @@ import android.app.PendingIntent; import android.content.Context; import android.content.res.Resources; import android.net.ConnectivityManager; +import android.net.IDnsResolver; import android.net.INetd; import android.net.Network; import android.net.NetworkCapabilities; @@ -69,6 +70,7 @@ public class LingerMonitorTest { LingerMonitor mMonitor; @Mock ConnectivityService mConnService; + @Mock IDnsResolver mDnsResolver; @Mock INetd mNetd; @Mock INetworkManagementService mNMS; @Mock Context mCtx; @@ -353,7 +355,7 @@ public class LingerMonitorTest { caps.addCapability(0); caps.addTransportType(transport); NetworkAgentInfo nai = new NetworkAgentInfo(null, null, new Network(netId), info, null, - caps, 50, mCtx, null, mMisc, mConnService, mNetd, mNMS, + caps, 50, mCtx, null, mMisc, mConnService, mNetd, mDnsResolver, mNMS, NetworkFactory.SerialNumber.NONE); nai.everValidated = true; return nai; diff --git a/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java b/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java index cc09fb7ba6..b709af1a02 100644 --- a/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java +++ b/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java @@ -27,6 +27,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import android.net.ConnectivityManager; +import android.net.IDnsResolver; import android.net.INetd; import android.net.InterfaceConfiguration; import android.net.IpPrefix; @@ -63,6 +64,7 @@ public class Nat464XlatTest { @Mock ConnectivityService mConnectivity; @Mock NetworkMisc mMisc; + @Mock IDnsResolver mDnsResolver; @Mock INetd mNetd; @Mock INetworkManagementService mNms; @Mock InterfaceConfiguration mConfig; @@ -72,7 +74,7 @@ public class Nat464XlatTest { Handler mHandler; Nat464Xlat makeNat464Xlat() { - return new Nat464Xlat(mNai, mNetd, mNms) { + return new Nat464Xlat(mNai, mNetd, mDnsResolver, mNms) { @Override protected int getNetId() { return NETID; } @@ -205,7 +207,7 @@ public class Nat464XlatTest { verify(mNms).unregisterObserver(eq(nat)); assertTrue(c.getValue().getStackedLinks().isEmpty()); assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE)); - verify(mNetd).resolverStopPrefix64Discovery(eq(NETID)); + verify(mDnsResolver).stopPrefix64Discovery(eq(NETID)); assertIdle(nat); // Stacked interface removed notification arrives and is ignored. @@ -331,7 +333,7 @@ public class Nat464XlatTest { verify(mNetd).clatdStop(eq(BASE_IFACE)); verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture()); verify(mNms).unregisterObserver(eq(nat)); - verify(mNetd).resolverStopPrefix64Discovery(eq(NETID)); + verify(mDnsResolver).stopPrefix64Discovery(eq(NETID)); assertTrue(c.getValue().getStackedLinks().isEmpty()); assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE)); assertIdle(nat); @@ -358,7 +360,7 @@ public class Nat464XlatTest { verify(mNetd).clatdStop(eq(BASE_IFACE)); verify(mNms).unregisterObserver(eq(nat)); - verify(mNetd).resolverStopPrefix64Discovery(eq(NETID)); + verify(mDnsResolver).stopPrefix64Discovery(eq(NETID)); assertIdle(nat); // In-flight interface up notification arrives: no-op @@ -390,7 +392,7 @@ public class Nat464XlatTest { verify(mNetd).clatdStop(eq(BASE_IFACE)); verify(mNms).unregisterObserver(eq(nat)); - verify(mNetd).resolverStopPrefix64Discovery(eq(NETID)); + verify(mDnsResolver).stopPrefix64Discovery(eq(NETID)); assertIdle(nat); verifyNoMoreInteractions(mNetd, mNms, mConnectivity); From 828dad188ceb4c08ead7968924889e088b9b3dda Mon Sep 17 00:00:00 2001 From: junyulai Date: Wed, 27 Mar 2019 11:00:37 +0800 Subject: [PATCH 08/20] Block unpriviledged apps which create keepalives with null fd Currently, socketKeepalive implementation is accepting null fd due to backward compatibility with legacy packet keepalive API. However, due to lack of the fd, the service cannot guarantee the port is not reused by another app if the caller release the port for any reason. Thus, grant the null fd access only for priviledged apps. This commit also address some comments from aosp/918533. Bug: 126699232 Test: atest FrameworksNetTests Change-Id: I0baf582ff4ca8af6082c3754e8dfbcd867f39792 --- core/java/android/net/ConnectivityManager.java | 3 +++ .../server/connectivity/KeepaliveTracker.java | 17 +++++++++++++---- .../android/server/ConnectivityServiceTest.java | 17 +++++++++++------ 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index ae93cf0197..4a64128f14 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -1934,6 +1934,8 @@ public class ConnectivityManager { @NonNull Callback callback) { ParcelFileDescriptor dup; try { + // Dup is needed here as the pfd inside the socket is owned by the IpSecService, + // which cannot be obtained by the app process. dup = ParcelFileDescriptor.dup(socket.getFileDescriptor()); } catch (IOException ignored) { // Construct an invalid fd, so that if the user later calls start(), it will fail with @@ -1975,6 +1977,7 @@ public class ConnectivityManager { @NonNull Callback callback) { ParcelFileDescriptor dup; try { + // TODO: Consider remove unnecessary dup. dup = pfd.dup(); } catch (IOException ignored) { // Construct an invalid fd, so that if the user later calls start(), it will fail with diff --git a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java index 722c1a0d9a..d7a57b992e 100644 --- a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java +++ b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java @@ -154,12 +154,19 @@ public class KeepaliveTracker { // keepalives are sent cannot be reused by another app even if the fd gets closed by // the user. A null is acceptable here for backward compatibility of PacketKeepalive // API. - // TODO: don't accept null fd after legacy packetKeepalive API is removed. try { if (fd != null) { mFd = Os.dup(fd); } else { - Log.d(TAG, "uid/pid " + mUid + "/" + mPid + " calls with null fd"); + Log.d(TAG, toString() + " calls with null fd"); + if (!mPrivileged) { + throw new SecurityException( + "null fd is not allowed for unprivileged access."); + } + if (mType == TYPE_TCP) { + throw new IllegalArgumentException( + "null fd is not allowed for tcp socket keepalives."); + } mFd = null; } } catch (ErrnoException e) { @@ -531,7 +538,8 @@ public class KeepaliveTracker { try { ki = new KeepaliveInfo(cb, nai, packet, intervalSeconds, KeepaliveInfo.TYPE_NATT, fd); - } catch (InvalidSocketException e) { + } catch (InvalidSocketException | IllegalArgumentException | SecurityException e) { + Log.e(TAG, "Fail to construct keepalive", e); notifyErrorCallback(cb, ERROR_INVALID_SOCKET); return; } @@ -570,7 +578,8 @@ public class KeepaliveTracker { try { ki = new KeepaliveInfo(cb, nai, packet, intervalSeconds, KeepaliveInfo.TYPE_TCP, fd); - } catch (InvalidSocketException e) { + } catch (InvalidSocketException | IllegalArgumentException | SecurityException e) { + Log.e(TAG, "Fail to construct keepalive e=" + e); notifyErrorCallback(cb, ERROR_INVALID_SOCKET); return; } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 92a865a3dc..8740f079e1 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -4047,8 +4047,6 @@ public class ConnectivityServiceTest { // TODO: 1. Move this outside of ConnectivityServiceTest. // 2. Make test to verify that Nat-T keepalive socket is created by IpSecService. // 3. Mock ipsec service. - // 4. Find a free port instead of a fixed port. - final int srcPort = 12345; final InetAddress myIPv4 = InetAddress.getByName("192.0.2.129"); final InetAddress notMyIPv4 = InetAddress.getByName("192.0.2.35"); final InetAddress myIPv6 = InetAddress.getByName("2001:db8::1"); @@ -4059,7 +4057,8 @@ public class ConnectivityServiceTest { final int invalidKaInterval = 9; final IpSecManager mIpSec = (IpSecManager) mContext.getSystemService(Context.IPSEC_SERVICE); - final UdpEncapsulationSocket testSocket = mIpSec.openUdpEncapsulationSocket(srcPort); + final UdpEncapsulationSocket testSocket = mIpSec.openUdpEncapsulationSocket(); + final int srcPort = testSocket.getPort(); LinkProperties lp = new LinkProperties(); lp.setInterfaceName("wlan12"); @@ -4179,6 +4178,7 @@ public class ConnectivityServiceTest { // Check that keepalive slots start from 1 and increment. The first one gets slot 1. mWiFiNetworkAgent.setExpectedKeepaliveSlot(1); + int srcPort2 = 0; try (SocketKeepalive ka = mCm.createSocketKeepalive( myNet, testSocket, myIPv4, dstIPv4, executor, callback)) { ka.start(validKaInterval); @@ -4186,7 +4186,8 @@ public class ConnectivityServiceTest { // The second one gets slot 2. mWiFiNetworkAgent.setExpectedKeepaliveSlot(2); - final UdpEncapsulationSocket testSocket2 = mIpSec.openUdpEncapsulationSocket(6789); + final UdpEncapsulationSocket testSocket2 = mIpSec.openUdpEncapsulationSocket(); + srcPort2 = testSocket2.getPort(); TestSocketKeepaliveCallback callback2 = new TestSocketKeepaliveCallback(executor); try (SocketKeepalive ka2 = mCm.createSocketKeepalive( myNet, testSocket2, myIPv4, dstIPv4, executor, callback2)) { @@ -4204,6 +4205,10 @@ public class ConnectivityServiceTest { } } + // Check that there is no port leaked after all keepalives and sockets are closed. + assertFalse(isUdpPortInUse(srcPort)); + assertFalse(isUdpPortInUse(srcPort2)); + mWiFiNetworkAgent.disconnect(); waitFor(mWiFiNetworkAgent.getDisconnectedCV()); mWiFiNetworkAgent = null; @@ -4292,7 +4297,6 @@ public class ConnectivityServiceTest { } private void doTestNattSocketKeepalivesFdWithExecutor(Executor executor) throws Exception { - final int srcPort = 12345; final InetAddress myIPv4 = InetAddress.getByName("192.0.2.129"); final InetAddress anyIPv4 = InetAddress.getByName("0.0.0.0"); final InetAddress dstIPv4 = InetAddress.getByName("8.8.8.8"); @@ -4311,7 +4315,8 @@ public class ConnectivityServiceTest { // Prepare the target file descriptor, keep only one instance. final IpSecManager mIpSec = (IpSecManager) mContext.getSystemService(Context.IPSEC_SERVICE); - final UdpEncapsulationSocket testSocket = mIpSec.openUdpEncapsulationSocket(srcPort); + final UdpEncapsulationSocket testSocket = mIpSec.openUdpEncapsulationSocket(); + final int srcPort = testSocket.getPort(); final ParcelFileDescriptor testPfd = ParcelFileDescriptor.dup(testSocket.getFileDescriptor()); testSocket.close(); From e17bb2dff47c5769e17262877e71f75e12c24890 Mon Sep 17 00:00:00 2001 From: Luke Huang Date: Tue, 26 Mar 2019 15:50:10 +0800 Subject: [PATCH 09/20] Fix cancellation race problem for aysnc DNS API This problem might cause double-close fd and result in app crash or unexpected behaviour Bug: 129317069 Test: atest DnsResolverTest manual test with delaying response callback/cancel Change-Id: I223234f527edafc51d34fa6be390419c05def8d8 --- core/java/android/net/DnsResolver.java | 60 +++++++++++++++----------- core/jni/android_net_NetUtils.cpp | 2 + 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/core/java/android/net/DnsResolver.java b/core/java/android/net/DnsResolver.java index 59802514c7..f9e0af227a 100644 --- a/core/java/android/net/DnsResolver.java +++ b/core/java/android/net/DnsResolver.java @@ -205,6 +205,7 @@ public final class DnsResolver { if (cancellationSignal != null && cancellationSignal.isCanceled()) { return; } + final Object lock = new Object(); final FileDescriptor queryfd; try { queryfd = resNetworkSend((network != null @@ -214,8 +215,8 @@ public final class DnsResolver { return; } - maybeAddCancellationSignal(cancellationSignal, queryfd); - registerFDListener(executor, queryfd, callback); + maybeAddCancellationSignal(cancellationSignal, queryfd, lock); + registerFDListener(executor, queryfd, callback, cancellationSignal, lock); } /** @@ -242,6 +243,7 @@ public final class DnsResolver { if (cancellationSignal != null && cancellationSignal.isCanceled()) { return; } + final Object lock = new Object(); final FileDescriptor queryfd; try { queryfd = resNetworkQuery((network != null @@ -251,31 +253,37 @@ public final class DnsResolver { return; } - maybeAddCancellationSignal(cancellationSignal, queryfd); - registerFDListener(executor, queryfd, callback); + maybeAddCancellationSignal(cancellationSignal, queryfd, lock); + registerFDListener(executor, queryfd, callback, cancellationSignal, lock); } private void registerFDListener(@NonNull Executor executor, - @NonNull FileDescriptor queryfd, @NonNull AnswerCallback answerCallback) { + @NonNull FileDescriptor queryfd, @NonNull AnswerCallback answerCallback, + @Nullable CancellationSignal cancellationSignal, @NonNull Object lock) { Looper.getMainLooper().getQueue().addOnFileDescriptorEventListener( queryfd, FD_EVENTS, (fd, events) -> { executor.execute(() -> { - byte[] answerbuf = null; - try { - answerbuf = resNetworkResult(fd); - } catch (ErrnoException e) { - Log.e(TAG, "resNetworkResult:" + e.toString()); - answerCallback.onQueryException(e); - return; - } + synchronized (lock) { + if (cancellationSignal != null && cancellationSignal.isCanceled()) { + return; + } + byte[] answerbuf = null; + try { + answerbuf = resNetworkResult(fd); + } catch (ErrnoException e) { + Log.e(TAG, "resNetworkResult:" + e.toString()); + answerCallback.onQueryException(e); + return; + } - try { - answerCallback.onAnswer( - answerCallback.parser.parse(answerbuf)); - } catch (ParseException e) { - answerCallback.onParseException(e); + try { + answerCallback.onAnswer( + answerCallback.parser.parse(answerbuf)); + } catch (ParseException e) { + answerCallback.onParseException(e); + } } }); // Unregister this fd listener @@ -284,14 +292,16 @@ public final class DnsResolver { } private void maybeAddCancellationSignal(@Nullable CancellationSignal cancellationSignal, - @NonNull FileDescriptor queryfd) { + @NonNull FileDescriptor queryfd, @NonNull Object lock) { if (cancellationSignal == null) return; - cancellationSignal.setOnCancelListener( - () -> { - Looper.getMainLooper().getQueue() - .removeOnFileDescriptorEventListener(queryfd); - resNetworkCancel(queryfd); - }); + cancellationSignal.setOnCancelListener(() -> { + synchronized (lock) { + if (!queryfd.valid()) return; + Looper.getMainLooper().getQueue() + .removeOnFileDescriptorEventListener(queryfd); + resNetworkCancel(queryfd); + } + }); } private static class DnsAddressAnswer extends DnsPacket { diff --git a/core/jni/android_net_NetUtils.cpp b/core/jni/android_net_NetUtils.cpp index d7a981ed3e..82acf6fb43 100644 --- a/core/jni/android_net_NetUtils.cpp +++ b/core/jni/android_net_NetUtils.cpp @@ -470,6 +470,7 @@ static jbyteArray android_net_utils_resNetworkResult(JNIEnv *env, jobject thiz, std::vector buf(MAXPACKETSIZE, 0); int res = resNetworkResult(fd, &rcode, buf.data(), MAXPACKETSIZE); + jniSetFileDescriptorOfFD(env, javaFd, -1); if (res < 0) { throwErrnoException(env, "resNetworkResult", -res); return nullptr; @@ -490,6 +491,7 @@ static jbyteArray android_net_utils_resNetworkResult(JNIEnv *env, jobject thiz, static void android_net_utils_resNetworkCancel(JNIEnv *env, jobject thiz, jobject javaFd) { int fd = jniGetFDFromFileDescriptor(env, javaFd); resNetworkCancel(fd); + jniSetFileDescriptorOfFD(env, javaFd, -1); } static jobject android_net_utils_getTcpRepairWindow(JNIEnv *env, jobject thiz, jobject javaFd) { From 612520f5448b6d46c05ca60fb99c8d45433a9474 Mon Sep 17 00:00:00 2001 From: Varun Anand Date: Sun, 17 Feb 2019 23:43:25 -0800 Subject: [PATCH 10/20] Take all VPN underlying networks into account when migrating traffic for VPN uid. Bug: 113122541 Bug: 120145746 Test: atest FrameworksNetTests Test: Manually verified on device that stats from VPN UID are moved appropriately based on its declared underlying network set. Test: vogar --mode app_process --benchmark NetworkStatsBenchmark.java Change-Id: I9d8d0cc58d18002c1c96f8ddff780ef8dc452d21 --- .../android/server/ConnectivityService.java | 27 ++-- .../java/android/net/NetworkStatsTest.java | 4 +- .../server/net/NetworkStatsServiceTest.java | 142 +++++++++++++++++- 3 files changed, 154 insertions(+), 19 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 4416b4d103..a746a87559 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -4305,7 +4305,7 @@ public class ConnectivityService extends IConnectivityManager.Stub /** * @return VPN information for accounting, or null if we can't retrieve all required - * information, e.g primary underlying iface. + * information, e.g underlying ifaces. */ @Nullable private VpnInfo createVpnInfo(Vpn vpn) { @@ -4317,17 +4317,24 @@ public class ConnectivityService extends IConnectivityManager.Stub // see VpnService.setUnderlyingNetworks()'s javadoc about how to interpret // the underlyingNetworks list. if (underlyingNetworks == null) { - NetworkAgentInfo defaultNetwork = getDefaultNetwork(); - if (defaultNetwork != null && defaultNetwork.linkProperties != null) { - info.primaryUnderlyingIface = getDefaultNetwork().linkProperties.getInterfaceName(); - } - } else if (underlyingNetworks.length > 0) { - LinkProperties linkProperties = getLinkProperties(underlyingNetworks[0]); - if (linkProperties != null) { - info.primaryUnderlyingIface = linkProperties.getInterfaceName(); + NetworkAgentInfo defaultNai = getDefaultNetwork(); + if (defaultNai != null && defaultNai.linkProperties != null) { + underlyingNetworks = new Network[] { defaultNai.network }; } } - return info.primaryUnderlyingIface == null ? null : info; + if (underlyingNetworks != null && underlyingNetworks.length > 0) { + List interfaces = new ArrayList<>(); + for (Network network : underlyingNetworks) { + LinkProperties lp = getLinkProperties(network); + if (lp != null) { + interfaces.add(lp.getInterfaceName()); + } + } + if (!interfaces.isEmpty()) { + info.underlyingIfaces = interfaces.toArray(new String[interfaces.size()]); + } + } + return info.underlyingIfaces == null ? null : info; } /** diff --git a/tests/net/java/android/net/NetworkStatsTest.java b/tests/net/java/android/net/NetworkStatsTest.java index b5b0384ca5..9ed6cb9b45 100644 --- a/tests/net/java/android/net/NetworkStatsTest.java +++ b/tests/net/java/android/net/NetworkStatsTest.java @@ -569,7 +569,7 @@ public class NetworkStatsTest { .addValues(underlyingIface, tunUid, SET_FOREGROUND, TAG_NONE, METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO, 0L, 0L, 0L, 0L, 0L); - assertTrue(delta.toString(), delta.migrateTun(tunUid, tunIface, underlyingIface)); + delta.migrateTun(tunUid, tunIface, new String[] {underlyingIface}); assertEquals(20, delta.size()); // tunIface and TEST_IFACE entries are not changed. @@ -650,7 +650,7 @@ public class NetworkStatsTest { .addValues(underlyingIface, tunUid, SET_DEFAULT, TAG_NONE, METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO, 75500L, 37L, 130000L, 70L, 0L); - assertTrue(delta.migrateTun(tunUid, tunIface, underlyingIface)); + delta.migrateTun(tunUid, tunIface, new String[]{underlyingIface}); assertEquals(9, delta.size()); // tunIface entries should not be changed. diff --git a/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java b/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java index bce526d3ae..c46534c8d0 100644 --- a/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java +++ b/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java @@ -927,7 +927,7 @@ public class NetworkStatsServiceTest { // WiFi network is connected and VPN is using WiFi (which has TEST_IFACE). expectDefaultSettings(); NetworkState[] networkStates = new NetworkState[] {buildWifiState(), buildVpnState()}; - VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(TEST_IFACE)}; + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; expectNetworkStatsUidDetail(buildEmptyStats()); expectBandwidthControlCheck(); @@ -947,8 +947,10 @@ public class NetworkStatsServiceTest { expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 3) .addValues(TUN_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1000L, 100L, 1000L, 100L, 1L) .addValues(TUN_IFACE, UID_BLUE, SET_DEFAULT, TAG_NONE, 500L, 50L, 500L, 50L, 1L) - .addValues( - TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 1650L, 150L, 1650L, 150L, 2L)); + // VPN received 1650 bytes over WiFi in background (SET_DEFAULT). + .addValues(TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 1650L, 150L, 0L, 0L, 1L) + // VPN sent 1650 bytes over WiFi in foreground (SET_FOREGROUND). + .addValues(TEST_IFACE, UID_VPN, SET_FOREGROUND, TAG_NONE, 0L, 0L, 1650L, 150L, 1L)); forcePollAndWaitForIdle(); @@ -962,7 +964,7 @@ public class NetworkStatsServiceTest { // WiFi network is connected and VPN is using WiFi (which has TEST_IFACE). expectDefaultSettings(); NetworkState[] networkStates = new NetworkState[] {buildWifiState(), buildVpnState()}; - VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(TEST_IFACE)}; + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; expectNetworkStatsUidDetail(buildEmptyStats()); expectBandwidthControlCheck(); @@ -992,6 +994,132 @@ public class NetworkStatsServiceTest { assertUidTotal(sTemplateWifi, UID_VPN, 0L, 0L, 0L, 0L, 0); } + @Test + public void vpnWithTwoUnderlyingIfaces_packetDuplication() throws Exception { + // WiFi and Cell networks are connected and VPN is using WiFi (which has TEST_IFACE) and + // Cell (which has TEST_IFACE2) and has declared both of them in its underlying network set. + // Additionally, VPN is duplicating traffic across both WiFi and Cell. + expectDefaultSettings(); + NetworkState[] networkStates = + new NetworkState[] { + buildWifiState(), buildMobile4gState(TEST_IFACE2), buildVpnState() + }; + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE, TEST_IFACE2})}; + expectNetworkStatsUidDetail(buildEmptyStats()); + expectBandwidthControlCheck(); + + mService.forceUpdateIfaces( + new Network[] {WIFI_NETWORK, VPN_NETWORK}, + vpnInfos, + networkStates, + getActiveIface(networkStates)); + // create some traffic (assume 10 bytes of MTU for VPN interface and 1 byte encryption + // overhead per packet): + // 1000 bytes (100 packets) were sent/received by UID_RED and UID_BLUE over VPN. + // VPN sent/received 4400 bytes (400 packets) over both WiFi and Cell (8800 bytes in total). + // Of 8800 bytes over WiFi/Cell, expect: + // - 500 bytes rx/tx each over WiFi/Cell attributed to both UID_RED and UID_BLUE. + // - 1200 bytes rx/tx each over WiFi/Cell for VPN_UID. + incrementCurrentTime(HOUR_IN_MILLIS); + expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 4) + .addValues(TUN_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1000L, 100L, 1000L, 100L, 2L) + .addValues(TUN_IFACE, UID_BLUE, SET_DEFAULT, TAG_NONE, 1000L, 100L, 1000L, 100L, 2L) + .addValues(TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 2200L, 200L, 2200L, 200L, 2L) + .addValues( + TEST_IFACE2, UID_VPN, SET_DEFAULT, TAG_NONE, 2200L, 200L, 2200L, 200L, 2L)); + + forcePollAndWaitForIdle(); + + assertUidTotal(sTemplateWifi, UID_RED, 500L, 50L, 500L, 50L, 1); + assertUidTotal(sTemplateWifi, UID_BLUE, 500L, 50L, 500L, 50L, 1); + assertUidTotal(sTemplateWifi, UID_VPN, 1200L, 100L, 1200L, 100L, 2); + + assertUidTotal(buildTemplateMobileWildcard(), UID_RED, 500L, 50L, 500L, 50L, 1); + assertUidTotal(buildTemplateMobileWildcard(), UID_BLUE, 500L, 50L, 500L, 50L, 1); + assertUidTotal(buildTemplateMobileWildcard(), UID_VPN, 1200L, 100L, 1200L, 100L, 2); + } + + @Test + public void vpnWithTwoUnderlyingIfaces_splitTraffic() throws Exception { + // WiFi and Cell networks are connected and VPN is using WiFi (which has TEST_IFACE) and + // Cell (which has TEST_IFACE2) and has declared both of them in its underlying network set. + // Additionally, VPN is arbitrarily splitting traffic across WiFi and Cell. + expectDefaultSettings(); + NetworkState[] networkStates = + new NetworkState[] { + buildWifiState(), buildMobile4gState(TEST_IFACE2), buildVpnState() + }; + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE, TEST_IFACE2})}; + expectNetworkStatsUidDetail(buildEmptyStats()); + expectBandwidthControlCheck(); + + mService.forceUpdateIfaces( + new Network[] {WIFI_NETWORK, VPN_NETWORK}, + vpnInfos, + networkStates, + getActiveIface(networkStates)); + // create some traffic (assume 10 bytes of MTU for VPN interface and 1 byte encryption + // overhead per packet): + // 1000 bytes (100 packets) were sent/received by UID_RED over VPN. + // VPN sent/received 660 bytes (60 packets) over WiFi and 440 bytes (40 packets) over Cell. + // For UID_RED, expect 600 bytes attributed over WiFi and 400 bytes over Cell for both + // rx/tx. + // For UID_VPN, expect 60 bytes attributed over WiFi and 40 bytes over Cell for both rx/tx. + incrementCurrentTime(HOUR_IN_MILLIS); + expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 3) + .addValues(TUN_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1000L, 100L, 1000L, 100L, 2L) + .addValues(TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 660L, 60L, 660L, 60L, 1L) + .addValues(TEST_IFACE2, UID_VPN, SET_DEFAULT, TAG_NONE, 440L, 40L, 440L, 40L, 1L)); + + forcePollAndWaitForIdle(); + + assertUidTotal(sTemplateWifi, UID_RED, 600L, 60L, 600L, 60L, 1); + assertUidTotal(sTemplateWifi, UID_VPN, 60L, 0L, 60L, 0L, 1); + + assertUidTotal(buildTemplateMobileWildcard(), UID_RED, 400L, 40L, 400L, 40L, 1); + assertUidTotal(buildTemplateMobileWildcard(), UID_VPN, 40L, 0L, 40L, 0L, 1); + } + + @Test + public void vpnWithTwoUnderlyingIfaces_splitTrafficWithCompression() throws Exception { + // WiFi and Cell networks are connected and VPN is using WiFi (which has TEST_IFACE) and + // Cell (which has TEST_IFACE2) and has declared both of them in its underlying network set. + // Additionally, VPN is arbitrarily splitting compressed traffic across WiFi and Cell. + expectDefaultSettings(); + NetworkState[] networkStates = + new NetworkState[] { + buildWifiState(), buildMobile4gState(TEST_IFACE2), buildVpnState() + }; + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE, TEST_IFACE2})}; + expectNetworkStatsUidDetail(buildEmptyStats()); + expectBandwidthControlCheck(); + + mService.forceUpdateIfaces( + new Network[] {WIFI_NETWORK, VPN_NETWORK}, + vpnInfos, + networkStates, + getActiveIface(networkStates)); + // create some traffic (assume 10 bytes of MTU for VPN interface: + // 1000 bytes (100 packets) were sent/received by UID_RED over VPN. + // VPN sent/received 600 bytes (60 packets) over WiFi and 200 bytes (20 packets) over Cell. + // For UID_RED, expect 600 bytes attributed over WiFi and 200 bytes over Cell for both + // rx/tx. + // UID_VPN gets nothing attributed to it (avoiding negative stats). + incrementCurrentTime(HOUR_IN_MILLIS); + expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 4) + .addValues(TUN_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1000L, 100L, 1000L, 100L, 1L) + .addValues(TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 600L, 60L, 600L, 60L, 0L) + .addValues(TEST_IFACE2, UID_VPN, SET_DEFAULT, TAG_NONE, 200L, 20L, 200L, 20L, 0L)); + + forcePollAndWaitForIdle(); + + assertUidTotal(sTemplateWifi, UID_RED, 600L, 60L, 600L, 60L, 0); + assertUidTotal(sTemplateWifi, UID_VPN, 0L, 0L, 0L, 0L, 0); + + assertUidTotal(buildTemplateMobileWildcard(), UID_RED, 200L, 20L, 200L, 20L, 0); + assertUidTotal(buildTemplateMobileWildcard(), UID_VPN, 0L, 0L, 0L, 0L, 0); + } + @Test public void vpnWithIncorrectUnderlyingIface() throws Exception { // WiFi and Cell networks are connected and VPN is using Cell (which has TEST_IFACE2), @@ -1001,7 +1129,7 @@ public class NetworkStatsServiceTest { new NetworkState[] { buildWifiState(), buildMobile4gState(TEST_IFACE2), buildVpnState() }; - VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(TEST_IFACE)}; + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; expectNetworkStatsUidDetail(buildEmptyStats()); expectBandwidthControlCheck(); @@ -1382,11 +1510,11 @@ public class NetworkStatsServiceTest { return new NetworkState(info, prop, new NetworkCapabilities(), VPN_NETWORK, null, null); } - private static VpnInfo createVpnInfo(String underlyingIface) { + private static VpnInfo createVpnInfo(String[] underlyingIfaces) { VpnInfo info = new VpnInfo(); info.ownerUid = UID_VPN; info.vpnIface = TUN_IFACE; - info.primaryUnderlyingIface = underlyingIface; + info.underlyingIfaces = underlyingIfaces; return info; } From 2af0b66aba89a4d1eeed69823967b659628f271c Mon Sep 17 00:00:00 2001 From: Varun Anand Date: Fri, 18 Jan 2019 19:22:48 -0800 Subject: [PATCH 11/20] NetworkStatsService: Fix getDetailedUidStats to take VPNs into account. This API is similar to one provided by NetworkStatsFactory with the difference that NSS also migrates traffic from VPN UID to other apps. Since traffic can only be migrated over NetworkStats delta, NSS therefore maintains NetworkStats snapshot across all UIDs/ifaces/tags. This snapshot gets updated whenever NSS records a new snapshot (based on various hooks such as VPN updating its underlying networks, network getting lost, etc.), or getDetailedUidStats API is invoked by one of its callers. Bug: 113122541 Bug: 120145746 Test: atest FrameworksNetTests Test: manually verified that battery stats are migrating traffic off of TUN (after patching above CL where we point BatteryStats to use this API). Change-Id: Ib0f0c2d4d41ee1d7a027ea9da457baaf198d649e --- .../java/android/net/NetworkStatsTest.java | 31 ++++ .../server/net/NetworkStatsServiceTest.java | 148 +++++++++++++++++- 2 files changed, 173 insertions(+), 6 deletions(-) diff --git a/tests/net/java/android/net/NetworkStatsTest.java b/tests/net/java/android/net/NetworkStatsTest.java index 9ed6cb9b45..c16a0f4466 100644 --- a/tests/net/java/android/net/NetworkStatsTest.java +++ b/tests/net/java/android/net/NetworkStatsTest.java @@ -812,6 +812,37 @@ public class NetworkStatsTest { assertEquals(entry2, stats.getValues(1, null)); } + @Test + public void testFilterDebugEntries() { + NetworkStats.Entry entry1 = new NetworkStats.Entry( + "test1", 10100, SET_DEFAULT, TAG_NONE, METERED_NO, ROAMING_NO, + DEFAULT_NETWORK_NO, 50000L, 25L, 100000L, 50L, 0L); + + NetworkStats.Entry entry2 = new NetworkStats.Entry( + "test2", 10101, SET_DBG_VPN_IN, TAG_NONE, METERED_NO, ROAMING_NO, + DEFAULT_NETWORK_NO, 50000L, 25L, 100000L, 50L, 0L); + + NetworkStats.Entry entry3 = new NetworkStats.Entry( + "test2", 10101, SET_DEFAULT, TAG_NONE, METERED_NO, ROAMING_NO, + DEFAULT_NETWORK_NO, 50000L, 25L, 100000L, 50L, 0L); + + NetworkStats.Entry entry4 = new NetworkStats.Entry( + "test2", 10101, SET_DBG_VPN_OUT, TAG_NONE, METERED_NO, ROAMING_NO, + DEFAULT_NETWORK_NO, 50000L, 25L, 100000L, 50L, 0L); + + NetworkStats stats = new NetworkStats(TEST_START, 4) + .addValues(entry1) + .addValues(entry2) + .addValues(entry3) + .addValues(entry4); + + stats.filterDebugEntries(); + + assertEquals(2, stats.size()); + assertEquals(entry1, stats.getValues(0, null)); + assertEquals(entry3, stats.getValues(1, null)); + } + @Test public void testApply464xlatAdjustments() { final String v4Iface = "v4-wlan0"; diff --git a/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java b/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java index c46534c8d0..e35c34affd 100644 --- a/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java +++ b/tests/net/java/com/android/server/net/NetworkStatsServiceTest.java @@ -57,6 +57,7 @@ import static com.android.server.net.NetworkStatsService.ACTION_NETWORK_STATS_PO import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; @@ -216,11 +217,16 @@ public class NetworkStatsServiceTest { expectNetworkStatsUidDetail(buildEmptyStats()); expectSystemReady(); + assertNull(mService.getTunAdjustedStats()); mService.systemReady(); + // Verify that system ready fetches realtime stats and initializes tun adjusted stats. + verify(mNetManager).getNetworkStatsUidDetail(UID_ALL, INTERFACES_ALL); + assertNotNull("failed to initialize TUN adjusted stats", mService.getTunAdjustedStats()); + assertEquals(0, mService.getTunAdjustedStats().size()); + mSession = mService.openSession(); assertNotNull("openSession() failed", mSession); - // catch INetworkManagementEventObserver during systemReady() ArgumentCaptor networkObserver = ArgumentCaptor.forClass(INetworkManagementEventObserver.class); @@ -733,11 +739,13 @@ public class NetworkStatsServiceTest { NetworkStats stats = mService.getDetailedUidStats(ifaceFilter); - verify(mNetManager, times(1)).getNetworkStatsUidDetail(eq(UID_ALL), argThat(ifaces -> - ifaces != null && ifaces.length == 2 - && ArrayUtils.contains(ifaces, TEST_IFACE) - && ArrayUtils.contains(ifaces, stackedIface))); - + // mNetManager#getNetworkStatsUidDetail(UID_ALL, INTERFACES_ALL) has following invocations: + // 1) NetworkStatsService#systemReady from #setUp. + // 2) mService#forceUpdateIfaces in the test above. + // 3) Finally, mService#getDetailedUidStats. + verify(mNetManager, times(3)).getNetworkStatsUidDetail(UID_ALL, INTERFACES_ALL); + assertTrue(ArrayUtils.contains(stats.getUniqueIfaces(), TEST_IFACE)); + assertTrue(ArrayUtils.contains(stats.getUniqueIfaces(), stackedIface)); assertEquals(2, stats.size()); assertEquals(uidStats, stats.getValues(0, null)); assertEquals(tetheredStats1, stats.getValues(1, null)); @@ -1157,6 +1165,134 @@ public class NetworkStatsServiceTest { assertUidTotal(buildTemplateMobileWildcard(), UID_VPN, 1100L, 100L, 1100L, 100L, 1); } + @Test + public void recordSnapshot_migratesTunTrafficAndUpdatesTunAdjustedStats() throws Exception { + assertEquals(0, mService.getTunAdjustedStats().size()); + // VPN using WiFi (TEST_IFACE). + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; + expectBandwidthControlCheck(); + // create some traffic (assume 10 bytes of MTU for VPN interface and 1 byte encryption + // overhead per packet): + // 1000 bytes (100 packets) were downloaded by UID_RED over VPN. + // VPN received 1100 bytes (100 packets) over WiFi. + incrementCurrentTime(HOUR_IN_MILLIS); + expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 2) + .addValues(TUN_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1000L, 100L, 0L, 0L, 0L) + .addValues(TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 1100L, 100L, 0L, 0L, 0L)); + + // this should lead to NSS#recordSnapshotLocked + mService.forceUpdateIfaces( + new Network[0], vpnInfos, new NetworkState[0], null /* activeIface */); + + // Verify TUN adjusted stats have traffic migrated correctly. + // Of 1100 bytes VPN received over WiFi, expect 1000 bytes attributed to UID_RED and 100 + // bytes attributed to UID_VPN. + NetworkStats tunAdjStats = mService.getTunAdjustedStats(); + assertValues( + tunAdjStats, TEST_IFACE, UID_RED, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 1000L, 100L, 0L, 0L, 0); + assertValues( + tunAdjStats, TEST_IFACE, UID_VPN, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 100L, 0L, 0L, 0L, 0); + } + + @Test + public void getDetailedUidStats_migratesTunTrafficAndUpdatesTunAdjustedStats() + throws Exception { + assertEquals(0, mService.getTunAdjustedStats().size()); + // VPN using WiFi (TEST_IFACE). + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; + expectBandwidthControlCheck(); + mService.forceUpdateIfaces( + new Network[0], vpnInfos, new NetworkState[0], null /* activeIface */); + // create some traffic (assume 10 bytes of MTU for VPN interface and 1 byte encryption + // overhead per packet): + // 1000 bytes (100 packets) were downloaded by UID_RED over VPN. + // VPN received 1100 bytes (100 packets) over WiFi. + incrementCurrentTime(HOUR_IN_MILLIS); + expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 2) + .addValues(TUN_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1000L, 100L, 0L, 0L, 0L) + .addValues(TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 1100L, 100L, 0L, 0L, 0L)); + + mService.getDetailedUidStats(INTERFACES_ALL); + + // Verify internally maintained TUN adjusted stats + NetworkStats tunAdjStats = mService.getTunAdjustedStats(); + // Verify stats for TEST_IFACE (WiFi): + // Of 1100 bytes VPN received over WiFi, expect 1000 bytes attributed to UID_RED and 100 + // bytes attributed to UID_VPN. + assertValues( + tunAdjStats, TEST_IFACE, UID_RED, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 1000L, 100L, 0L, 0L, 0); + assertValues( + tunAdjStats, TEST_IFACE, UID_VPN, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 100L, 0L, 0L, 0L, 0); + // Verify stats for TUN_IFACE; only UID_RED should have usage on it. + assertValues( + tunAdjStats, TUN_IFACE, UID_RED, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 1000L, 100L, 0L, 0L, 0); + assertValues( + tunAdjStats, TUN_IFACE, UID_VPN, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 0L, 0L, 0L, 0L, 0); + + // lets assume that since last time, VPN received another 1100 bytes (same assumptions as + // before i.e. UID_RED downloaded another 1000 bytes). + incrementCurrentTime(HOUR_IN_MILLIS); + // Note - NetworkStatsFactory returns counters that are monotonically increasing. + expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 2) + .addValues(TUN_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 2000L, 200L, 0L, 0L, 0L) + .addValues(TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 2200L, 200L, 0L, 0L, 0L)); + + mService.getDetailedUidStats(INTERFACES_ALL); + + tunAdjStats = mService.getTunAdjustedStats(); + // verify TEST_IFACE stats: + assertValues( + tunAdjStats, TEST_IFACE, UID_RED, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 2000L, 200L, 0L, 0L, 0); + assertValues( + tunAdjStats, TEST_IFACE, UID_VPN, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 200L, 0L, 0L, 0L, 0); + // verify TUN_IFACE stats: + assertValues( + tunAdjStats, TUN_IFACE, UID_RED, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 2000L, 200L, 0L, 0L, 0); + assertValues( + tunAdjStats, TUN_IFACE, UID_VPN, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 0L, 0L, 0L, 0L, 0); + } + + @Test + public void getDetailedUidStats_returnsCorrectStatsWithVpnRunning() throws Exception { + // VPN using WiFi (TEST_IFACE). + VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; + expectBandwidthControlCheck(); + mService.forceUpdateIfaces( + new Network[0], vpnInfos, new NetworkState[0], null /* activeIface */); + // create some traffic (assume 10 bytes of MTU for VPN interface and 1 byte encryption + // overhead per packet): + // 1000 bytes (100 packets) were downloaded by UID_RED over VPN. + // VPN received 1100 bytes (100 packets) over WiFi. + incrementCurrentTime(HOUR_IN_MILLIS); + expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 2) + .addValues(TUN_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1000L, 100L, 0L, 0L, 0L) + .addValues(TEST_IFACE, UID_VPN, SET_DEFAULT, TAG_NONE, 1100L, 100L, 0L, 0L, 0L)); + + // Query realtime stats for TEST_IFACE. + NetworkStats queriedStats = + mService.getDetailedUidStats(new String[] {TEST_IFACE}); + + assertEquals(HOUR_IN_MILLIS, queriedStats.getElapsedRealtime()); + // verify that returned stats are only for TEST_IFACE and VPN traffic is migrated correctly. + assertEquals(new String[] {TEST_IFACE}, queriedStats.getUniqueIfaces()); + assertValues( + queriedStats, TEST_IFACE, UID_RED, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 1000L, 100L, 0L, 0L, 0); + assertValues( + queriedStats, TEST_IFACE, UID_VPN, SET_ALL, TAG_NONE, METERED_ALL, ROAMING_ALL, + DEFAULT_NETWORK_ALL, 100L, 0L, 0L, 0L, 0); + } + @Test public void testRegisterUsageCallback() throws Exception { // pretend that wifi network comes online; service should ask about full From 3c33ee5d8103a66f68796ffed18f68e45e1061fb Mon Sep 17 00:00:00 2001 From: Xiao Ma Date: Fri, 1 Mar 2019 12:25:36 +0900 Subject: [PATCH 12/20] Add DHCP address lease expiry in IpMemoryStore. Bug:122710829 Test: atest FrameworksNetTests Change-Id: I643fe1231edcd18923514ab66c64a6cf83e69443 --- .../net/java/android/net/ipmemorystore/ParcelableTests.java | 5 ++++- .../server/net/ipmemorystore/NetworkAttributesTest.java | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/net/java/android/net/ipmemorystore/ParcelableTests.java b/tests/net/java/android/net/ipmemorystore/ParcelableTests.java index 76cccc9574..1a3ea6096c 100644 --- a/tests/net/java/android/net/ipmemorystore/ParcelableTests.java +++ b/tests/net/java/android/net/ipmemorystore/ParcelableTests.java @@ -44,6 +44,8 @@ public class ParcelableTests { assertEquals(in, new NetworkAttributes(parcelingRoundTrip(in.toParcelable()))); builder.setAssignedV4Address((Inet4Address) Inet4Address.getByName("1.2.3.4")); + // lease will expire in two hours + builder.setAssignedV4AddressExpiry(System.currentTimeMillis() + 7_200_000); // groupHint stays null this time around builder.setDnsAddresses(Collections.emptyList()); builder.setMtu(18); @@ -51,6 +53,7 @@ public class ParcelableTests { assertEquals(in, new NetworkAttributes(parcelingRoundTrip(in.toParcelable()))); builder.setAssignedV4Address((Inet4Address) Inet4Address.getByName("6.7.8.9")); + builder.setAssignedV4AddressExpiry(System.currentTimeMillis() + 3_600_000); builder.setGroupHint("groupHint"); builder.setDnsAddresses(Arrays.asList( InetAddress.getByName("ACA1:652B:0911:DE8F:1200:115E:913B:AA2A"), @@ -66,7 +69,7 @@ public class ParcelableTests { // Verify that this test does not miss any new field added later. // If any field is added to NetworkAttributes it must be tested here for parceling // roundtrip. - assertEquals(4, Arrays.stream(NetworkAttributes.class.getDeclaredFields()) + assertEquals(5, Arrays.stream(NetworkAttributes.class.getDeclaredFields()) .filter(f -> !Modifier.isStatic(f.getModifiers())).count()); } diff --git a/tests/net/java/com/android/server/net/ipmemorystore/NetworkAttributesTest.java b/tests/net/java/com/android/server/net/ipmemorystore/NetworkAttributesTest.java index dc20185430..fb84611cb6 100644 --- a/tests/net/java/com/android/server/net/ipmemorystore/NetworkAttributesTest.java +++ b/tests/net/java/com/android/server/net/ipmemorystore/NetworkAttributesTest.java @@ -57,6 +57,7 @@ public class NetworkAttributesTest { final NetworkAttributes na = new NetworkAttributes( (Inet4Address) Inet4Address.getByAddress(new byte[] {1, 2, 3, 4}), + System.currentTimeMillis() + 7_200_000, "some hint", Arrays.asList(Inet4Address.getByAddress(new byte[] {5, 6, 7, 8}), Inet4Address.getByAddress(new byte[] {9, 0, 1, 2})), From fb5afabcb8d3a28a24ffcce88af17bb9586d7c4c Mon Sep 17 00:00:00 2001 From: Luke Huang Date: Thu, 28 Mar 2019 13:56:31 +0800 Subject: [PATCH 13/20] Unremove DnsResolver#query method used by external developers This method was removed as part of addressing API council feedback in b/129261432 Add back previous DnsResolver#query which is already being used by developers. Bug: 129395490 Test: atest DnsResolverTest Change-Id: Ic956db204f3940d39d42e1b11dda39e57d356fad --- core/java/android/net/DnsResolver.java | 207 +++++++++++++++++++++++-- 1 file changed, 194 insertions(+), 13 deletions(-) diff --git a/core/java/android/net/DnsResolver.java b/core/java/android/net/DnsResolver.java index f9e0af227a..687b721834 100644 --- a/core/java/android/net/DnsResolver.java +++ b/core/java/android/net/DnsResolver.java @@ -22,6 +22,10 @@ import static android.net.NetworkUtils.resNetworkResult; import static android.net.NetworkUtils.resNetworkSend; import static android.os.MessageQueue.OnFileDescriptorEventListener.EVENT_ERROR; import static android.os.MessageQueue.OnFileDescriptorEventListener.EVENT_INPUT; +import static android.system.OsConstants.AF_INET; +import static android.system.OsConstants.AF_INET6; +import static android.system.OsConstants.IPPROTO_UDP; +import static android.system.OsConstants.SOCK_DGRAM; import android.annotation.CallbackExecutor; import android.annotation.IntDef; @@ -30,12 +34,18 @@ import android.annotation.Nullable; import android.os.CancellationSignal; import android.os.Looper; import android.system.ErrnoException; +import android.system.Os; import android.util.Log; +import libcore.io.IoUtils; + import java.io.FileDescriptor; +import java.io.IOException; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; import java.net.UnknownHostException; import java.util.ArrayList; import java.util.List; @@ -52,6 +62,7 @@ public final class DnsResolver { private static final String TAG = "DnsResolver"; private static final int FD_EVENTS = EVENT_INPUT | EVENT_ERROR; private static final int MAXPACKET = 8 * 1024; + private static final int SLEEP_TIME = 2; @IntDef(prefix = { "CLASS_" }, value = { CLASS_IN @@ -188,9 +199,9 @@ public final class DnsResolver { * Send a raw DNS query. * The answer will be provided asynchronously through the provided {@link AnswerCallback}. * - * @param network {@link Network} specifying which network for querying. + * @param network {@link Network} specifying which network to query on. * {@code null} for query on default network. - * @param query blob message + * @param query blob message to query * @param flags flags as a combination of the FLAGS_* constants * @param executor The {@link Executor} that the callback should be executed on. * @param cancellationSignal used by the caller to signal if the query should be @@ -211,21 +222,23 @@ public final class DnsResolver { queryfd = resNetworkSend((network != null ? network.netId : NETID_UNSET), query, query.length, flags); } catch (ErrnoException e) { - callback.onQueryException(e); + executor.execute(() -> { + callback.onQueryException(e); + }); return; } - maybeAddCancellationSignal(cancellationSignal, queryfd, lock); registerFDListener(executor, queryfd, callback, cancellationSignal, lock); + maybeAddCancellationSignal(cancellationSignal, queryfd, lock); } /** * Send a DNS query with the specified name, class and query type. * The answer will be provided asynchronously through the provided {@link AnswerCallback}. * - * @param network {@link Network} specifying which network for querying. + * @param network {@link Network} specifying which network to query on. * {@code null} for query on default network. - * @param domain domain name for querying + * @param domain domain name to query * @param nsClass dns class as one of the CLASS_* constants * @param nsType dns resource record (RR) type as one of the TYPE_* constants * @param flags flags as a combination of the FLAGS_* constants @@ -249,12 +262,145 @@ public final class DnsResolver { queryfd = resNetworkQuery((network != null ? network.netId : NETID_UNSET), domain, nsClass, nsType, flags); } catch (ErrnoException e) { - callback.onQueryException(e); + executor.execute(() -> { + callback.onQueryException(e); + }); return; } - maybeAddCancellationSignal(cancellationSignal, queryfd, lock); registerFDListener(executor, queryfd, callback, cancellationSignal, lock); + maybeAddCancellationSignal(cancellationSignal, queryfd, lock); + } + + private class InetAddressAnswerAccumulator extends InetAddressAnswerCallback { + private final List mAllAnswers; + private ParseException mParseException; + private ErrnoException mErrnoException; + private final InetAddressAnswerCallback mUserCallback; + private final int mTargetAnswerCount; + private int mReceivedAnswerCount = 0; + + InetAddressAnswerAccumulator(int size, @NonNull InetAddressAnswerCallback callback) { + mTargetAnswerCount = size; + mAllAnswers = new ArrayList<>(); + mUserCallback = callback; + } + + private boolean maybeReportException() { + if (mErrnoException != null) { + mUserCallback.onQueryException(mErrnoException); + return true; + } + if (mParseException != null) { + mUserCallback.onParseException(mParseException); + return true; + } + return false; + } + + private void maybeReportAnswer() { + if (++mReceivedAnswerCount != mTargetAnswerCount) return; + if (mAllAnswers.isEmpty() && maybeReportException()) return; + // TODO: Do RFC6724 sort. + mUserCallback.onAnswer(mAllAnswers); + } + + @Override + public void onAnswer(@NonNull List answer) { + mAllAnswers.addAll(answer); + maybeReportAnswer(); + } + + @Override + public void onParseException(@NonNull ParseException e) { + mParseException = e; + maybeReportAnswer(); + } + + @Override + public void onQueryException(@NonNull ErrnoException e) { + mErrnoException = e; + maybeReportAnswer(); + } + } + + /** + * Send a DNS query with the specified name, get back a set of InetAddresses asynchronously. + * The answer will be provided asynchronously through the provided + * {@link InetAddressAnswerCallback}. + * + * @param network {@link Network} specifying which network to query on. + * {@code null} for query on default network. + * @param domain domain name to query + * @param flags flags as a combination of the FLAGS_* constants + * @param executor The {@link Executor} that the callback should be executed on. + * @param cancellationSignal used by the caller to signal if the query should be + * cancelled. May be {@code null}. + * @param callback an {@link InetAddressAnswerCallback} which will be called to notify the + * caller of the result of dns query. + */ + public void query(@Nullable Network network, @NonNull String domain, @QueryFlag int flags, + @NonNull @CallbackExecutor Executor executor, + @Nullable CancellationSignal cancellationSignal, + @NonNull InetAddressAnswerCallback callback) { + if (cancellationSignal != null && cancellationSignal.isCanceled()) { + return; + } + final Object lock = new Object(); + final boolean queryIpv6 = haveIpv6(network); + final boolean queryIpv4 = haveIpv4(network); + + final FileDescriptor v4fd; + final FileDescriptor v6fd; + + int queryCount = 0; + + if (queryIpv6) { + try { + v6fd = resNetworkQuery((network != null + ? network.netId : NETID_UNSET), domain, CLASS_IN, TYPE_AAAA, flags); + } catch (ErrnoException e) { + executor.execute(() -> { + callback.onQueryException(e); + }); + return; + } + queryCount++; + } else v6fd = null; + + // TODO: Use device flag to controll the sleep time. + // Avoiding gateways drop packets if queries are sent too close together + try { + Thread.sleep(SLEEP_TIME); + } catch (InterruptedException ex) { } + + if (queryIpv4) { + try { + v4fd = resNetworkQuery((network != null + ? network.netId : NETID_UNSET), domain, CLASS_IN, TYPE_A, flags); + } catch (ErrnoException e) { + if (queryIpv6) resNetworkCancel(v6fd); // Closes fd, marks it invalid. + executor.execute(() -> { + callback.onQueryException(e); + }); + return; + } + queryCount++; + } else v4fd = null; + + final InetAddressAnswerAccumulator accumulator = + new InetAddressAnswerAccumulator(queryCount, callback); + + if (queryIpv6) registerFDListener(executor, v6fd, accumulator, cancellationSignal, lock); + if (queryIpv4) registerFDListener(executor, v4fd, accumulator, cancellationSignal, lock); + + if (cancellationSignal == null) return; + cancellationSignal.setOnCancelListener(() -> { + synchronized (lock) { + if (queryIpv4) cancelQuery(v4fd); + if (queryIpv6) cancelQuery(v6fd); + } + }); } private void registerFDListener(@NonNull Executor executor, @@ -271,7 +417,7 @@ public final class DnsResolver { } byte[] answerbuf = null; try { - answerbuf = resNetworkResult(fd); + answerbuf = resNetworkResult(fd); // Closes fd, marks it invalid. } catch (ErrnoException e) { Log.e(TAG, "resNetworkResult:" + e.toString()); answerCallback.onQueryException(e); @@ -291,19 +437,54 @@ public final class DnsResolver { }); } + private void cancelQuery(@NonNull FileDescriptor queryfd) { + if (!queryfd.valid()) return; + Looper.getMainLooper().getQueue().removeOnFileDescriptorEventListener(queryfd); + resNetworkCancel(queryfd); // Closes fd, marks it invalid. + } + private void maybeAddCancellationSignal(@Nullable CancellationSignal cancellationSignal, @NonNull FileDescriptor queryfd, @NonNull Object lock) { if (cancellationSignal == null) return; cancellationSignal.setOnCancelListener(() -> { synchronized (lock) { - if (!queryfd.valid()) return; - Looper.getMainLooper().getQueue() - .removeOnFileDescriptorEventListener(queryfd); - resNetworkCancel(queryfd); + cancelQuery(queryfd); } }); } + // These two functions match the behaviour of have_ipv4 and have_ipv6 in the native resolver. + private boolean haveIpv4(@Nullable Network network) { + final SocketAddress addrIpv4 = + new InetSocketAddress(InetAddresses.parseNumericAddress("8.8.8.8"), 0); + return checkConnectivity(network, AF_INET, addrIpv4); + } + + private boolean haveIpv6(@Nullable Network network) { + final SocketAddress addrIpv6 = + new InetSocketAddress(InetAddresses.parseNumericAddress("2000::"), 0); + return checkConnectivity(network, AF_INET6, addrIpv6); + } + + private boolean checkConnectivity(@Nullable Network network, + int domain, @NonNull SocketAddress addr) { + final FileDescriptor socket; + try { + socket = Os.socket(domain, SOCK_DGRAM, IPPROTO_UDP); + } catch (ErrnoException e) { + return false; + } + try { + if (network != null) network.bindSocket(socket); + Os.connect(socket, addr); + } catch (IOException | ErrnoException e) { + return false; + } finally { + IoUtils.closeQuietly(socket); + } + return true; + } + private static class DnsAddressAnswer extends DnsPacket { private static final String TAG = "DnsResolver.DnsAddressAnswer"; private static final boolean DBG = false; From 293ffaa0812b73ee7f4d5cf2cc1c2ed19bd1c717 Mon Sep 17 00:00:00 2001 From: Luke Huang Date: Fri, 29 Mar 2019 18:01:11 +0800 Subject: [PATCH 14/20] Minor change for async DNS API do the minor changes to address comments before Bug: 129395490 Test: atest DnsResolverTest Change-Id: I56e2e5bc4352ff5c979579247a333a41950079da --- core/java/android/net/DnsResolver.java | 47 +++++++++++++++----------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/core/java/android/net/DnsResolver.java b/core/java/android/net/DnsResolver.java index 687b721834..06c32c675a 100644 --- a/core/java/android/net/DnsResolver.java +++ b/core/java/android/net/DnsResolver.java @@ -62,7 +62,7 @@ public final class DnsResolver { private static final String TAG = "DnsResolver"; private static final int FD_EVENTS = EVENT_INPUT | EVENT_ERROR; private static final int MAXPACKET = 8 * 1024; - private static final int SLEEP_TIME = 2; + private static final int SLEEP_TIME_MS = 2; @IntDef(prefix = { "CLASS_" }, value = { CLASS_IN @@ -228,8 +228,11 @@ public final class DnsResolver { return; } - registerFDListener(executor, queryfd, callback, cancellationSignal, lock); - maybeAddCancellationSignal(cancellationSignal, queryfd, lock); + synchronized (lock) { + registerFDListener(executor, queryfd, callback, cancellationSignal, lock); + if (cancellationSignal == null) return; + addCancellationSignal(cancellationSignal, queryfd, lock); + } } /** @@ -267,9 +270,11 @@ public final class DnsResolver { }); return; } - - registerFDListener(executor, queryfd, callback, cancellationSignal, lock); - maybeAddCancellationSignal(cancellationSignal, queryfd, lock); + synchronized (lock) { + registerFDListener(executor, queryfd, callback, cancellationSignal, lock); + if (cancellationSignal == null) return; + addCancellationSignal(cancellationSignal, queryfd, lock); + } } private class InetAddressAnswerAccumulator extends InetAddressAnswerCallback { @@ -368,10 +373,10 @@ public final class DnsResolver { queryCount++; } else v6fd = null; - // TODO: Use device flag to controll the sleep time. + // TODO: Use device flag to control the sleep time. // Avoiding gateways drop packets if queries are sent too close together try { - Thread.sleep(SLEEP_TIME); + Thread.sleep(SLEEP_TIME_MS); } catch (InterruptedException ex) { } if (queryIpv4) { @@ -391,16 +396,21 @@ public final class DnsResolver { final InetAddressAnswerAccumulator accumulator = new InetAddressAnswerAccumulator(queryCount, callback); - if (queryIpv6) registerFDListener(executor, v6fd, accumulator, cancellationSignal, lock); - if (queryIpv4) registerFDListener(executor, v4fd, accumulator, cancellationSignal, lock); - - if (cancellationSignal == null) return; - cancellationSignal.setOnCancelListener(() -> { - synchronized (lock) { - if (queryIpv4) cancelQuery(v4fd); - if (queryIpv6) cancelQuery(v6fd); + synchronized (lock) { + if (queryIpv6) { + registerFDListener(executor, v6fd, accumulator, cancellationSignal, lock); } - }); + if (queryIpv4) { + registerFDListener(executor, v4fd, accumulator, cancellationSignal, lock); + } + if (cancellationSignal == null) return; + cancellationSignal.setOnCancelListener(() -> { + synchronized (lock) { + if (queryIpv4) cancelQuery(v4fd); + if (queryIpv6) cancelQuery(v6fd); + } + }); + } } private void registerFDListener(@NonNull Executor executor, @@ -443,9 +453,8 @@ public final class DnsResolver { resNetworkCancel(queryfd); // Closes fd, marks it invalid. } - private void maybeAddCancellationSignal(@Nullable CancellationSignal cancellationSignal, + private void addCancellationSignal(@NonNull CancellationSignal cancellationSignal, @NonNull FileDescriptor queryfd, @NonNull Object lock) { - if (cancellationSignal == null) return; cancellationSignal.setOnCancelListener(() -> { synchronized (lock) { cancelQuery(queryfd); From 895a741bf150519dab03e4e11ebb4fb5926e17bc Mon Sep 17 00:00:00 2001 From: paulhu Date: Wed, 27 Mar 2019 22:26:37 +0800 Subject: [PATCH 15/20] Fix ApfCapabilities, LinkAddress, RouteInfo, IpPrefix API issues. Fix: 129362082 Fix: 129361362 Fix: 129360330 Fix: 129362379 Test: atest FrameworksNetTests NetworkStackTests Change-Id: I05fbc6f98207d5cf002e3cbc5829040af7d6be52 --- core/java/android/net/IpPrefix.java | 20 +++--- core/java/android/net/LinkAddress.java | 15 +++-- core/java/android/net/RouteInfo.java | 63 +++++++++++++++---- .../java/android/net/apf/ApfCapabilities.java | 22 ++++--- 4 files changed, 85 insertions(+), 35 deletions(-) diff --git a/core/java/android/net/IpPrefix.java b/core/java/android/net/IpPrefix.java index b4f3a288d2..416157ca65 100644 --- a/core/java/android/net/IpPrefix.java +++ b/core/java/android/net/IpPrefix.java @@ -16,6 +16,7 @@ package android.net; +import android.annotation.IntRange; import android.annotation.NonNull; import android.annotation.SystemApi; import android.annotation.TestApi; @@ -71,7 +72,7 @@ public final class IpPrefix implements Parcelable { * * @hide */ - public IpPrefix(@NonNull byte[] address, int prefixLength) { + public IpPrefix(@NonNull byte[] address, @IntRange(from = 0, to = 128) int prefixLength) { this.address = address.clone(); this.prefixLength = prefixLength; checkAndMaskAddressAndPrefixLength(); @@ -88,7 +89,7 @@ public final class IpPrefix implements Parcelable { */ @SystemApi @TestApi - public IpPrefix(@NonNull InetAddress address, int prefixLength) { + public IpPrefix(@NonNull InetAddress address, @IntRange(from = 0, to = 128) int prefixLength) { // We don't reuse the (byte[], int) constructor because it calls clone() on the byte array, // which is unnecessary because getAddress() already returns a clone. this.address = address.getAddress(); @@ -150,13 +151,13 @@ public final class IpPrefix implements Parcelable { * * @return the address in the form of a byte array. */ - public InetAddress getAddress() { + public @NonNull InetAddress getAddress() { try { return InetAddress.getByAddress(address); } catch (UnknownHostException e) { // Cannot happen. InetAddress.getByAddress can only throw an exception if the byte // array is the wrong length, but we check that in the constructor. - return null; + throw new IllegalArgumentException("Address is invalid"); } } @@ -166,7 +167,7 @@ public final class IpPrefix implements Parcelable { * * @return the address in the form of a byte array. */ - public byte[] getRawAddress() { + public @NonNull byte[] getRawAddress() { return address.clone(); } @@ -175,6 +176,7 @@ public final class IpPrefix implements Parcelable { * * @return the prefix length. */ + @IntRange(from = 0, to = 128) public int getPrefixLength() { return prefixLength; } @@ -183,10 +185,10 @@ public final class IpPrefix implements Parcelable { * Determines whether the prefix contains the specified address. * * @param address An {@link InetAddress} to test. - * @return {@code true} if the prefix covers the given address. + * @return {@code true} if the prefix covers the given address. {@code false} otherwise. */ - public boolean contains(InetAddress address) { - byte[] addrBytes = (address == null) ? null : address.getAddress(); + public boolean contains(@NonNull InetAddress address) { + byte[] addrBytes = address.getAddress(); if (addrBytes == null || addrBytes.length != this.address.length) { return false; } @@ -201,7 +203,7 @@ public final class IpPrefix implements Parcelable { * @param otherPrefix the prefix to test * @hide */ - public boolean containsPrefix(IpPrefix otherPrefix) { + public boolean containsPrefix(@NonNull IpPrefix otherPrefix) { if (otherPrefix.getPrefixLength() < prefixLength) return false; final byte[] otherAddress = otherPrefix.getRawAddress(); NetworkUtils.maskRawAddress(otherAddress, prefixLength); diff --git a/core/java/android/net/LinkAddress.java b/core/java/android/net/LinkAddress.java index 78b4665d45..f17adea538 100644 --- a/core/java/android/net/LinkAddress.java +++ b/core/java/android/net/LinkAddress.java @@ -25,6 +25,7 @@ import static android.system.OsConstants.RT_SCOPE_LINK; import static android.system.OsConstants.RT_SCOPE_SITE; import static android.system.OsConstants.RT_SCOPE_UNIVERSE; +import android.annotation.IntRange; import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.SystemApi; @@ -170,7 +171,7 @@ public class LinkAddress implements Parcelable { * Constructs a new {@code LinkAddress} from an {@code InetAddress} and prefix length, with * the specified flags and scope. Flags and scope are not checked for validity. * @param address The IP address. - * @param prefixLength The prefix length. + * @param prefixLength The prefix length. Must be >= 0 and <= (32 or 128) (IPv4 or IPv6). * @param flags A bitmask of {@code IFA_F_*} values representing properties of the address. * @param scope An integer defining the scope in which the address is unique (e.g., * {@link OsConstants#RT_SCOPE_LINK} or {@link OsConstants#RT_SCOPE_SITE}). @@ -178,7 +179,8 @@ public class LinkAddress implements Parcelable { */ @SystemApi @TestApi - public LinkAddress(InetAddress address, int prefixLength, int flags, int scope) { + public LinkAddress(@NonNull InetAddress address, @IntRange(from = 0, to = 128) int prefixLength, + int flags, int scope) { init(address, prefixLength, flags, scope); } @@ -186,12 +188,13 @@ public class LinkAddress implements Parcelable { * Constructs a new {@code LinkAddress} from an {@code InetAddress} and a prefix length. * The flags are set to zero and the scope is determined from the address. * @param address The IP address. - * @param prefixLength The prefix length. + * @param prefixLength The prefix length. Must be >= 0 and <= (32 or 128) (IPv4 or IPv6). * @hide */ @SystemApi @TestApi - public LinkAddress(@NonNull InetAddress address, int prefixLength) { + public LinkAddress(@NonNull InetAddress address, + @IntRange(from = 0, to = 128) int prefixLength) { this(address, prefixLength, 0, 0); this.scope = scopeForUnicastAddress(address); } @@ -202,7 +205,7 @@ public class LinkAddress implements Parcelable { * @param interfaceAddress The interface address. * @hide */ - public LinkAddress(InterfaceAddress interfaceAddress) { + public LinkAddress(@NonNull InterfaceAddress interfaceAddress) { this(interfaceAddress.getAddress(), interfaceAddress.getNetworkPrefixLength()); } @@ -306,6 +309,7 @@ public class LinkAddress implements Parcelable { /** * Returns the prefix length of this {@code LinkAddress}. */ + @IntRange(from = 0, to = 128) public int getPrefixLength() { return prefixLength; } @@ -316,6 +320,7 @@ public class LinkAddress implements Parcelable { * @hide */ @UnsupportedAppUsage + @IntRange(from = 0, to = 128) public int getNetworkPrefixLength() { return getPrefixLength(); } diff --git a/core/java/android/net/RouteInfo.java b/core/java/android/net/RouteInfo.java index 24d9b8e2c1..fdd904a041 100644 --- a/core/java/android/net/RouteInfo.java +++ b/core/java/android/net/RouteInfo.java @@ -16,6 +16,8 @@ package android.net; +import android.annotation.IntDef; +import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.SystemApi; import android.annotation.TestApi; @@ -24,6 +26,8 @@ import android.os.Build; import android.os.Parcel; import android.os.Parcelable; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; import java.net.Inet4Address; import java.net.Inet6Address; import java.net.InetAddress; @@ -51,20 +55,32 @@ import java.util.Objects; * (IPv4 or IPv6). */ public final class RouteInfo implements Parcelable { + /** @hide */ + @IntDef(value = { + RTN_UNICAST, + RTN_UNREACHABLE, + RTN_THROW, + }) + @Retention(RetentionPolicy.SOURCE) + public @interface RouteType {} + /** * The IP destination address for this route. */ + @NonNull private final IpPrefix mDestination; /** * The gateway address for this route. */ @UnsupportedAppUsage + @Nullable private final InetAddress mGateway; /** * The interface for this route. */ + @Nullable private final String mInterface; @@ -108,13 +124,14 @@ public final class RouteInfo implements Parcelable { * @param destination the destination prefix * @param gateway the IP address to route packets through * @param iface the interface name to send packets on + * @param type the type of this route * * @hide */ @SystemApi @TestApi public RouteInfo(@Nullable IpPrefix destination, @Nullable InetAddress gateway, - @Nullable String iface, int type) { + @Nullable String iface, @RouteType int type) { switch (type) { case RTN_UNICAST: case RTN_UNREACHABLE: @@ -173,10 +190,24 @@ public final class RouteInfo implements Parcelable { } /** - * @hide + * Constructs a {@code RouteInfo} object. + * + * If destination is null, then gateway must be specified and the + * constructed route is either the IPv4 default route 0.0.0.0 + * if the gateway is an instance of {@link Inet4Address}, or the IPv6 default + * route ::/0 if gateway is an instance of {@link Inet6Address}. + *

+ * Destination and gateway may not both be null. + * + * @param destination the destination address and prefix in an {@link IpPrefix} + * @param gateway the {@link InetAddress} to route packets through + * @param iface the interface name to send packets on + * + * @hide */ @UnsupportedAppUsage - public RouteInfo(IpPrefix destination, InetAddress gateway, String iface) { + public RouteInfo(@Nullable IpPrefix destination, @Nullable InetAddress gateway, + @Nullable String iface) { this(destination, gateway, iface, RTN_UNICAST); } @@ -184,7 +215,8 @@ public final class RouteInfo implements Parcelable { * @hide */ @UnsupportedAppUsage - public RouteInfo(LinkAddress destination, InetAddress gateway, String iface) { + public RouteInfo(@Nullable LinkAddress destination, @Nullable InetAddress gateway, + @Nullable String iface) { this(destination == null ? null : new IpPrefix(destination.getAddress(), destination.getPrefixLength()), gateway, iface); @@ -205,7 +237,7 @@ public final class RouteInfo implements Parcelable { * * @hide */ - public RouteInfo(IpPrefix destination, InetAddress gateway) { + public RouteInfo(@Nullable IpPrefix destination, @Nullable InetAddress gateway) { this(destination, gateway, null); } @@ -215,7 +247,7 @@ public final class RouteInfo implements Parcelable { * TODO: Remove this. */ @UnsupportedAppUsage - public RouteInfo(LinkAddress destination, InetAddress gateway) { + public RouteInfo(@Nullable LinkAddress destination, @Nullable InetAddress gateway) { this(destination, gateway, null); } @@ -227,7 +259,7 @@ public final class RouteInfo implements Parcelable { * @hide */ @UnsupportedAppUsage - public RouteInfo(InetAddress gateway) { + public RouteInfo(@NonNull InetAddress gateway) { this((IpPrefix) null, gateway, null); } @@ -239,35 +271,36 @@ public final class RouteInfo implements Parcelable { * * @hide */ - public RouteInfo(IpPrefix destination) { + public RouteInfo(@NonNull IpPrefix destination) { this(destination, null, null); } /** * @hide */ - public RouteInfo(LinkAddress destination) { + public RouteInfo(@NonNull LinkAddress destination) { this(destination, null, null); } /** * @hide */ - public RouteInfo(IpPrefix destination, int type) { + public RouteInfo(@NonNull IpPrefix destination, @RouteType int type) { this(destination, null, null, type); } /** * @hide */ - public static RouteInfo makeHostRoute(InetAddress host, String iface) { + public static RouteInfo makeHostRoute(@NonNull InetAddress host, @Nullable String iface) { return makeHostRoute(host, null, iface); } /** * @hide */ - public static RouteInfo makeHostRoute(InetAddress host, InetAddress gateway, String iface) { + public static RouteInfo makeHostRoute(@Nullable InetAddress host, @Nullable InetAddress gateway, + @Nullable String iface) { if (host == null) return null; if (host instanceof Inet4Address) { @@ -290,6 +323,7 @@ public final class RouteInfo implements Parcelable { * * @return {@link IpPrefix} specifying the destination. This is never {@code null}. */ + @NonNull public IpPrefix getDestination() { return mDestination; } @@ -298,6 +332,7 @@ public final class RouteInfo implements Parcelable { * TODO: Convert callers to use IpPrefix and then remove. * @hide */ + @NonNull public LinkAddress getDestinationLinkAddress() { return new LinkAddress(mDestination.getAddress(), mDestination.getPrefixLength()); } @@ -308,6 +343,7 @@ public final class RouteInfo implements Parcelable { * @return {@link InetAddress} specifying the gateway or next hop. This may be * {@code null} for a directly-connected route." */ + @Nullable public InetAddress getGateway() { return mGateway; } @@ -317,6 +353,7 @@ public final class RouteInfo implements Parcelable { * * @return The name of the interface used for this route. */ + @Nullable public String getInterface() { return mInterface; } @@ -330,6 +367,7 @@ public final class RouteInfo implements Parcelable { */ @TestApi @SystemApi + @RouteType public int getType() { return mType; } @@ -401,6 +439,7 @@ public final class RouteInfo implements Parcelable { * @hide */ @UnsupportedAppUsage + @Nullable public static RouteInfo selectBestRoute(Collection routes, InetAddress dest) { if ((routes == null) || (dest == null)) return null; diff --git a/core/java/android/net/apf/ApfCapabilities.java b/core/java/android/net/apf/ApfCapabilities.java index 17a03c7c89..4dd2ace59c 100644 --- a/core/java/android/net/apf/ApfCapabilities.java +++ b/core/java/android/net/apf/ApfCapabilities.java @@ -19,14 +19,17 @@ package android.net.apf; import android.annotation.NonNull; import android.annotation.SystemApi; import android.annotation.TestApi; -import android.content.Context; +import android.content.res.Resources; import android.os.Parcel; import android.os.Parcelable; import com.android.internal.R; /** - * APF program support capabilities. + * APF program support capabilities. APF stands for Android Packet Filtering and it is a flexible + * way to drop unwanted network packets to save power. + * + * See documentation at hardware/google/apf/apf.h * * This class is immutable. * @hide @@ -104,10 +107,11 @@ public final class ApfCapabilities implements Parcelable { } /** - * Returns true if the APF interpreter advertises support for the data buffer access opcodes - * LDDW and STDW. + * Determines whether the APF interpreter advertises support for the data buffer access opcodes + * LDDW (LoaD Data Word) and STDW (STore Data Word). Full LDDW (LoaD Data Word) and + * STDW (STore Data Word) support is present from APFv4 on. * - * Full LDDW and STDW support is present from APFv4 on. + * @return {@code true} if the IWifiStaIface#readApfPacketFilterData is supported. */ public boolean hasDataAccess() { return apfVersionSupported >= 4; @@ -116,14 +120,14 @@ public final class ApfCapabilities implements Parcelable { /** * @return Whether the APF Filter in the device should filter out IEEE 802.3 Frames. */ - public static boolean getApfDrop8023Frames(@NonNull Context context) { - return context.getResources().getBoolean(R.bool.config_apfDrop802_3Frames); + public static boolean getApfDrop8023Frames() { + return Resources.getSystem().getBoolean(R.bool.config_apfDrop802_3Frames); } /** * @return An array of blacklisted EtherType, packets with EtherTypes within it will be dropped. */ - public static @NonNull int[] getApfEthTypeBlackList(@NonNull Context context) { - return context.getResources().getIntArray(R.array.config_apfEthTypeBlackList); + public static @NonNull int[] getApfEtherTypeBlackList() { + return Resources.getSystem().getIntArray(R.array.config_apfEthTypeBlackList); } } From ca33d197d9ca3a025b27266f0ce4846abc7379aa Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Wed, 27 Mar 2019 15:42:53 +0900 Subject: [PATCH 16/20] Fix race when starting NetworkMonitor NetworkMonitor obtained LinkProperties and NetworkCapabilities via synchronous calls to ConnectivityManager after receiving an asynchronous notification, which is prone to races: the network could be gone before the LinkProperties/NetworkCapabilities can be fetched. Fix the race by passing LinkProperties/NetworkCapabilities directly to NetworkMonitor in the asynchronous notifications. Test: atest FrameworksNetTests NetworkStackTests Test: booted, WiFi works Bug: 129375892 Change-Id: I200ac7ca6ff79590b11c9be705f650c92fd3cb63 --- .../android/server/ConnectivityService.java | 27 +++++++------------ .../server/connectivity/NetworkAgentInfo.java | 23 +++++++++++++++- .../server/ConnectivityServiceTest.java | 2 +- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 58807de063..873bd498d2 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5382,7 +5382,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mContext, mTrackerHandler, new NetworkMisc(networkMisc), this, mNetd, mNMS, factorySerialNumber); // Make sure the network capabilities reflect what the agent info says. - nai.networkCapabilities = mixInCapabilities(nai, nc); + nai.setNetworkCapabilities(mixInCapabilities(nai, nc)); final String extraInfo = networkInfo.getExtraInfo(); final String name = TextUtils.isEmpty(extraInfo) ? nai.networkCapabilities.getSSID() : extraInfo; @@ -5475,12 +5475,12 @@ public class ConnectivityService extends IConnectivityManager.Stub // Start or stop DNS64 detection and 464xlat according to network state. networkAgent.clatd.update(); notifyIfacesChangedForNetworkStats(); + try { + networkAgent.networkMonitor().notifyLinkPropertiesChanged(newLp); + } catch (RemoteException e) { + e.rethrowFromSystemServer(); + } if (networkAgent.everConnected) { - try { - networkAgent.networkMonitor().notifyLinkPropertiesChanged(); - } catch (RemoteException e) { - e.rethrowFromSystemServer(); - } notifyNetworkCallbacks(networkAgent, ConnectivityManager.CALLBACK_IP_CHANGED); } } @@ -5708,7 +5708,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkCapabilities prevNc; synchronized (nai) { prevNc = nai.networkCapabilities; - nai.networkCapabilities = newNc; + nai.setNetworkCapabilities(newNc); } updateUids(nai, prevNc, newNc); @@ -5723,11 +5723,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // If the requestable capabilities have changed or the score changed, we can't have been // called by rematchNetworkAndRequests, so it's safe to start a rematch. rematchAllNetworksAndRequests(nai, oldScore); - try { - nai.networkMonitor().notifyNetworkCapabilitiesChanged(); - } catch (RemoteException e) { - e.rethrowFromSystemServer(); - } notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED); } @@ -5986,11 +5981,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } if (capabilitiesChanged) { - try { - nai.networkMonitor().notifyNetworkCapabilitiesChanged(); - } catch (RemoteException e) { - e.rethrowFromSystemServer(); - } notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED); } @@ -6399,7 +6389,8 @@ public class ConnectivityService extends IConnectivityManager.Stub if (networkAgent.networkMisc.acceptPartialConnectivity) { networkAgent.networkMonitor().setAcceptPartialConnectivity(); } - networkAgent.networkMonitor().notifyNetworkConnected(); + networkAgent.networkMonitor().notifyNetworkConnected( + networkAgent.linkProperties, networkAgent.networkCapabilities); } catch (RemoteException e) { e.rethrowFromSystemServer(); } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 8f2825ca72..aa05d9a2fa 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -29,6 +29,7 @@ import android.net.NetworkState; import android.os.Handler; import android.os.INetworkManagementService; import android.os.Messenger; +import android.os.RemoteException; import android.os.SystemClock; import android.util.Log; import android.util.SparseArray; @@ -120,7 +121,8 @@ public class NetworkAgentInfo implements Comparable { // This Network object is always valid. public final Network network; public LinkProperties linkProperties; - // This should only be modified via ConnectivityService.updateCapabilities(). + // This should only be modified by ConnectivityService, via setNetworkCapabilities(). + // TODO: make this private with a getter. public NetworkCapabilities networkCapabilities; public final NetworkMisc networkMisc; // Indicates if netd has been told to create this Network. From this point on the appropriate @@ -278,6 +280,25 @@ public class NetworkAgentInfo implements Comparable { mNetworkMonitor = networkMonitor; } + /** + * Set the NetworkCapabilities on this NetworkAgentInfo. Also attempts to notify NetworkMonitor + * of the new capabilities, if NetworkMonitor has been created. + * + *

If {@link NetworkMonitor#notifyNetworkCapabilitiesChanged(NetworkCapabilities)} fails, + * the exception is logged but not reported to callers. + */ + public void setNetworkCapabilities(NetworkCapabilities nc) { + networkCapabilities = nc; + final INetworkMonitor nm = mNetworkMonitor; + if (nm != null) { + try { + nm.notifyNetworkCapabilitiesChanged(nc); + } catch (RemoteException e) { + Log.e(TAG, "Error notifying NetworkMonitor of updated NetworkCapabilities", e); + } + } + } + public ConnectivityService connService() { return mConnService; } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 6f48da3c34..c7d46e6832 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -496,7 +496,7 @@ public class ConnectivityServiceTest { }; try { - doAnswer(validateAnswer).when(mNetworkMonitor).notifyNetworkConnected(); + doAnswer(validateAnswer).when(mNetworkMonitor).notifyNetworkConnected(any(), any()); doAnswer(validateAnswer).when(mNetworkMonitor).forceReevaluation(anyInt()); } catch (RemoteException e) { fail(e.getMessage()); From 3ee61008815baeaec759e5d548ba590361613111 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 1 Apr 2019 13:04:07 +0900 Subject: [PATCH 17/20] Cleanup SystemReady in the network stack The system server (in SystemServer.java) defines the boot sequence, during which an event called SystemReady happens. This corresponds to a time when critical system services that are depended upon by other components in the system server are ready to handle requests from their dependencies. Some system services are listening to this event to defer initializations that depend on the critical services. Because the network stack is only started after SystemReady, there is no way any NetworkMonitor may be started before SystemReady. Remove the associated mechanism. Fix: 129376083 Test: FrameworksNetTests Change-Id: I071eeb10d0b7c4f71af6653d322c7b442b2cc7ee --- .../android/server/ConnectivityService.java | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 971dd23e98..317eb93d7c 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3738,16 +3738,6 @@ public class ConnectivityService extends IConnectivityManager.Stub break; } case EVENT_SYSTEM_READY: { - for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) { - // Might have been called already in handleRegisterNetworkAgent since - // mSystemReady is set before sending EVENT_SYSTEM_READY, but calling - // this several times is fine. - try { - nai.networkMonitor().notifySystemReady(); - } catch (RemoteException e) { - e.rethrowFromSystemServer(); - } - } mMultipathPolicyTracker.start(); break; } @@ -5423,15 +5413,6 @@ public class ConnectivityService extends IConnectivityManager.Stub synchronized (mNetworkForNetId) { mNetworkForNetId.put(nai.network.netId, nai); } - synchronized (this) { - if (mSystemReady) { - try { - networkMonitor.notifySystemReady(); - } catch (RemoteException e) { - e.rethrowFromSystemServer(); - } - } - } try { networkMonitor.start(); From 646ee4621cd5fc7ed6e7a678a8d7da5a78889bcc Mon Sep 17 00:00:00 2001 From: junyulai Date: Mon, 1 Apr 2019 11:33:49 +0800 Subject: [PATCH 18/20] Ignore the asynchronous result while stopping keepalive Currently, onStopped callback are synchronizely triggered when stop() was called, since the framework don't really care about the result of stopping keepalive. However, if keepalive failed to stop for some reason, the handleStopKeepalive was called mistakenly and trigger additional callback that fail the test case. This commit is the behavior change prior to state machine refactoring, and introduce a stopping state for ignoring the result in the stopping state. Bug: 129512753 Bug: 123988249 Test: 1. atest com.android.server.ConnectivityServiceTest \ #testNattSocketKeepalives --generate-new-metrics 100 2. atest FrameworksNetTests Change-Id: I4fa94e0740ba488fb5fe7ac7c3812c195dd0ec4c --- .../server/connectivity/KeepaliveTracker.java | 43 +++++++++++-------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java index d7a57b992e..35f7ea3ae0 100644 --- a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java +++ b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java @@ -132,6 +132,7 @@ public class KeepaliveTracker { private static final int NOT_STARTED = 1; private static final int STARTING = 2; private static final int STARTED = 3; + private static final int STOPPING = 4; private int mStartedState = NOT_STARTED; KeepaliveInfo(@NonNull ISocketKeepaliveCallback callback, @@ -314,6 +315,7 @@ public class KeepaliveTracker { } } if (NOT_STARTED != mStartedState) { + mStartedState = STOPPING; Log.d(TAG, "Stopping keepalive " + mSlot + " on " + mNai.name()); if (mType == TYPE_NATT) { mNai.asyncChannel.sendMessage(CMD_STOP_SOCKET_KEEPALIVE, mSlot); @@ -456,8 +458,8 @@ public class KeepaliveTracker { ki = mKeepalives.get(nai).get(slot); } catch(NullPointerException e) {} if (ki == null) { - Log.e(TAG, "Event " + message.what + " for unknown keepalive " + slot + " on " - + nai.name()); + Log.e(TAG, "Event " + message.what + "," + slot + "," + reason + + " for unknown keepalive " + slot + " on " + nai.name()); return; } @@ -476,27 +478,30 @@ public class KeepaliveTracker { // messages in order. // TODO : clarify this code and get rid of mStartedState. Using a StateMachine is an // option. - if (reason == SUCCESS && KeepaliveInfo.STARTING == ki.mStartedState) { - // Keepalive successfully started. - if (DBG) Log.d(TAG, "Started keepalive " + slot + " on " + nai.name()); - ki.mStartedState = KeepaliveInfo.STARTED; - try { - ki.mCallback.onStarted(slot); - } catch (RemoteException e) { - Log.w(TAG, "Discarded onStarted(" + slot + ") callback"); - } - } else { - // Keepalive successfully stopped, or error. - if (reason == SUCCESS) { - // The message indicated success stopping : don't call handleStopKeepalive. - if (DBG) Log.d(TAG, "Successfully stopped keepalive " + slot + " on " + nai.name()); + if (KeepaliveInfo.STARTING == ki.mStartedState) { + if (SUCCESS == reason) { + // Keepalive successfully started. + if (DBG) Log.d(TAG, "Started keepalive " + slot + " on " + nai.name()); + ki.mStartedState = KeepaliveInfo.STARTED; + try { + ki.mCallback.onStarted(slot); + } catch (RemoteException e) { + Log.w(TAG, "Discarded onStarted(" + slot + ") callback"); + } } else { - // The message indicated some error trying to start or during the course of - // keepalive : do call handleStopKeepalive. + Log.d(TAG, "Failed to start keepalive " + slot + " on " + nai.name() + + ": " + reason); + // The message indicated some error trying to start: do call handleStopKeepalive. handleStopKeepalive(nai, slot, reason); - if (DBG) Log.d(TAG, "Keepalive " + slot + " on " + nai.name() + " error " + reason); } + } else if (KeepaliveInfo.STOPPING == ki.mStartedState) { + // The message indicated result of stopping : don't call handleStopKeepalive. + Log.d(TAG, "Stopped keepalive " + slot + " on " + nai.name() + + " stopped: " + reason); ki.mStartedState = KeepaliveInfo.NOT_STARTED; + } else { + Log.wtf(TAG, "Event " + message.what + "," + slot + "," + reason + + " for keepalive in wrong state: " + ki.toString()); } } From 1a407651019718d0c3db426b9ef96c49632453ba Mon Sep 17 00:00:00 2001 From: paulhu Date: Fri, 22 Mar 2019 16:35:06 +0800 Subject: [PATCH 19/20] Address leftover comments on 923337 and 930217. - Restrict unprivileged apps to use NetworkRequest.Builder#setSignalStrength. - Remove the "throws NullPointerException" in CaptivePortalProbeSpec constructor. - Remove the null check in LinkProperties. - Add annotataion into all ConnectivityManager.NetworkCallback methods. Change-Id: Id275cac1d6a30d7515cd7b113394f5e8a0179314 Fix: 129097486 Test: atest FrameworksNetTests --- .../java/android/net/ConnectivityManager.java | 24 ++++++----- core/java/android/net/LinkProperties.java | 8 +--- core/java/android/net/NetworkRequest.java | 5 +++ .../android/server/ConnectivityService.java | 25 ++++++++++- .../server/ConnectivityServiceTest.java | 41 +++++++++++++++++++ 5 files changed, 83 insertions(+), 20 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index ae93cf0197..79560f50b4 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -3231,7 +3231,7 @@ public class ConnectivityManager { * * @hide */ - public void onPreCheck(Network network) {} + public void onPreCheck(@NonNull Network network) {} /** * Called when the framework connects and has declared a new network ready for use. @@ -3244,8 +3244,9 @@ public class ConnectivityManager { * @param blocked Whether access to the {@link Network} is blocked due to system policy. * @hide */ - public void onAvailable(Network network, NetworkCapabilities networkCapabilities, - LinkProperties linkProperties, boolean blocked) { + public void onAvailable(@NonNull Network network, + @NonNull NetworkCapabilities networkCapabilities, + @NonNull LinkProperties linkProperties, boolean blocked) { // Internally only this method is called when a new network is available, and // it calls the callback in the same way and order that older versions used // to call so as not to change the behavior. @@ -3269,7 +3270,7 @@ public class ConnectivityManager { * * @param network The {@link Network} of the satisfying network. */ - public void onAvailable(Network network) {} + public void onAvailable(@NonNull Network network) {} /** * Called when the network is about to be disconnected. Often paired with an @@ -3285,7 +3286,7 @@ public class ConnectivityManager { * network connected. Note that the network may suffer a * hard loss at any time. */ - public void onLosing(Network network, int maxMsToLive) {} + public void onLosing(@NonNull Network network, int maxMsToLive) {} /** * Called when the framework has a hard loss of the network or when the @@ -3293,7 +3294,7 @@ public class ConnectivityManager { * * @param network The {@link Network} lost. */ - public void onLost(Network network) {} + public void onLost(@NonNull Network network) {} /** * Called if no network is found in the timeout time specified in @@ -3313,8 +3314,8 @@ public class ConnectivityManager { * @param networkCapabilities The new {@link android.net.NetworkCapabilities} for this * network. */ - public void onCapabilitiesChanged(Network network, - NetworkCapabilities networkCapabilities) {} + public void onCapabilitiesChanged(@NonNull Network network, + @NonNull NetworkCapabilities networkCapabilities) {} /** * Called when the network the framework connected to for this request @@ -3323,7 +3324,8 @@ public class ConnectivityManager { * @param network The {@link Network} whose link properties have changed. * @param linkProperties The new {@link LinkProperties} for this network. */ - public void onLinkPropertiesChanged(Network network, LinkProperties linkProperties) {} + public void onLinkPropertiesChanged(@NonNull Network network, + @NonNull LinkProperties linkProperties) {} /** * Called when the network the framework connected to for this request @@ -3334,7 +3336,7 @@ public class ConnectivityManager { * a tunnel, etc. * @hide */ - public void onNetworkSuspended(Network network) {} + public void onNetworkSuspended(@NonNull Network network) {} /** * Called when the network the framework connected to for this request @@ -3342,7 +3344,7 @@ public class ConnectivityManager { * preceded by a matching {@link NetworkCallback#onNetworkSuspended} call. * @hide */ - public void onNetworkResumed(Network network) {} + public void onNetworkResumed(@NonNull Network network) {} /** * Called when access to the specified network is blocked or unblocked. diff --git a/core/java/android/net/LinkProperties.java b/core/java/android/net/LinkProperties.java index 03d6d48bc8..ad67763ac3 100644 --- a/core/java/android/net/LinkProperties.java +++ b/core/java/android/net/LinkProperties.java @@ -316,9 +316,6 @@ public final class LinkProperties implements Parcelable { @SystemApi @TestApi public boolean removeLinkAddress(@NonNull LinkAddress toRemove) { - if (toRemove == null) { - return false; - } int i = findLinkAddressIndex(toRemove); if (i >= 0) { mLinkAddresses.remove(i); @@ -391,10 +388,7 @@ public final class LinkProperties implements Parcelable { @TestApi @SystemApi public boolean removeDnsServer(@NonNull InetAddress dnsServer) { - if (dnsServer != null) { - return mDnses.remove(dnsServer); - } - return false; + return mDnses.remove(dnsServer); } /** diff --git a/core/java/android/net/NetworkRequest.java b/core/java/android/net/NetworkRequest.java index 3a41a079da..acafa1354a 100644 --- a/core/java/android/net/NetworkRequest.java +++ b/core/java/android/net/NetworkRequest.java @@ -17,6 +17,7 @@ package android.net; import android.annotation.NonNull; +import android.annotation.RequiresPermission; import android.annotation.SystemApi; import android.annotation.UnsupportedAppUsage; import android.net.NetworkCapabilities.NetCapability; @@ -343,10 +344,14 @@ public class NetworkRequest implements Parcelable { * current value. A value of {@code SIGNAL_STRENGTH_UNSPECIFIED} means no value when * received and has no effect when requesting a callback. * + *

This method requires the caller to hold the + * {@link android.Manifest.permission#NETWORK_SIGNAL_STRENGTH_WAKEUP} permission + * * @param signalStrength the bearer-specific signal strength. * @hide */ @SystemApi + @RequiresPermission(android.Manifest.permission.NETWORK_SIGNAL_STRENGTH_WAKEUP) public @NonNull Builder setSignalStrength(int signalStrength) { mNetworkCapabilities.setSignalStrength(signalStrength); return this; diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index bca2df47cd..5f3c67fb35 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1882,6 +1882,15 @@ public class ConnectivityService extends IConnectivityManager.Stub return false; } + private boolean checkAnyPermissionOf(int pid, int uid, String... permissions) { + for (String permission : permissions) { + if (mContext.checkPermission(permission, pid, uid) == PERMISSION_GRANTED) { + return true; + } + } + return false; + } + private void enforceAnyPermissionOf(String... permissions) { if (!checkAnyPermissionOf(permissions)) { throw new SecurityException("Requires one of the following permissions: " @@ -1956,6 +1965,12 @@ public class ConnectivityService extends IConnectivityManager.Stub NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK); } + private boolean checkNetworkSignalStrengthWakeupPermission(int pid, int uid) { + return checkAnyPermissionOf(pid, uid, + android.Manifest.permission.NETWORK_SIGNAL_STRENGTH_WAKEUP, + NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK); + } + private void enforceConnectivityRestrictedNetworksPermission() { try { mContext.enforceCallingOrSelfPermission( @@ -4952,13 +4967,19 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - // This checks that the passed capabilities either do not request a specific SSID, or the - // calling app has permission to do so. + // This checks that the passed capabilities either do not request a specific SSID/SignalStrength + // , or the calling app has permission to do so. private void ensureSufficientPermissionsForRequest(NetworkCapabilities nc, int callerPid, int callerUid) { if (null != nc.getSSID() && !checkSettingsPermission(callerPid, callerUid)) { throw new SecurityException("Insufficient permissions to request a specific SSID"); } + + if (nc.hasSignalStrength() + && !checkNetworkSignalStrengthWakeupPermission(callerPid, callerUid)) { + throw new SecurityException( + "Insufficient permissions to request a specific signal strength"); + } } private ArrayList getSignalStrengthThresholds(NetworkAgentInfo nai) { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 92a865a3dc..34dafa7723 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -3022,6 +3022,47 @@ public class ConnectivityServiceTest { } } + @Test + public void testInvalidSignalStrength() { + NetworkRequest r = new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_INTERNET) + .addTransportType(TRANSPORT_WIFI) + .setSignalStrength(-75) + .build(); + // Registering a NetworkCallback with signal strength but w/o NETWORK_SIGNAL_STRENGTH_WAKEUP + // permission should get SecurityException. + try { + mCm.registerNetworkCallback(r, new NetworkCallback()); + fail("Expected SecurityException filing a callback with signal strength"); + } catch (SecurityException expected) { + // expected + } + + try { + mCm.registerNetworkCallback(r, PendingIntent.getService( + mServiceContext, 0, new Intent(), 0)); + fail("Expected SecurityException filing a callback with signal strength"); + } catch (SecurityException expected) { + // expected + } + + // Requesting a Network with signal strength should get IllegalArgumentException. + try { + mCm.requestNetwork(r, new NetworkCallback()); + fail("Expected IllegalArgumentException filing a request with signal strength"); + } catch (IllegalArgumentException expected) { + // expected + } + + try { + mCm.requestNetwork(r, PendingIntent.getService( + mServiceContext, 0, new Intent(), 0)); + fail("Expected IllegalArgumentException filing a request with signal strength"); + } catch (IllegalArgumentException expected) { + // expected + } + } + @Test public void testRegisterDefaultNetworkCallback() throws Exception { final TestNetworkCallback defaultNetworkCallback = new TestNetworkCallback(); From 207037a2438bd2f06df1417abebefa2f4b62ed05 Mon Sep 17 00:00:00 2001 From: paulhu Date: Tue, 2 Apr 2019 00:49:00 +0800 Subject: [PATCH 20/20] Fix IpPrefixTest#testContainsInetAddress fail. The argument of IpPreFix#contains() has been marked as @NonNull. So the IpPrefixTest#testContainsInetAddress should not test contains() method wiht null object. Bug: None Test: atest FrameworksNetTests atest IpPrefixTest#testContainsInetAddress Change-Id: I2f6bee19514dc47702f64d2a2bbf02d8b7b1b407 --- tests/net/java/android/net/IpPrefixTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/net/java/android/net/IpPrefixTest.java b/tests/net/java/android/net/IpPrefixTest.java index 3cc0e368d3..abf019afed 100644 --- a/tests/net/java/android/net/IpPrefixTest.java +++ b/tests/net/java/android/net/IpPrefixTest.java @@ -231,7 +231,6 @@ public class IpPrefixTest { assertFalse(p.contains(Address("2001:db8:f00::ace:d00e"))); assertFalse(p.contains(Address("2001:db8:f00::bad:d00d"))); assertFalse(p.contains(Address("2001:4868:4860::8888"))); - assertFalse(p.contains((InetAddress)null)); assertFalse(p.contains(Address("8.8.8.8"))); p = new IpPrefix("192.0.2.0/23");