From fff0b7ce6df92ad612a02eb1a0a30d0f72d0b73d Mon Sep 17 00:00:00 2001 From: James Mattis Date: Sun, 28 Mar 2021 17:41:09 -0700 Subject: [PATCH] Correctly count nri uid request counts Correctly count nri uid request counts in the per-app functionality in connectivity currently used by set profile and set oem network preference APIs. Previously, upon creation, nris would be created prior to removing them. This would cause the uid request counts to artificially increase and incorrectly throw an error if the request count limit was hit even though in actuality an apps request count was valid. E.g., if there was an existing request for per-app functionality and its owning app made a change to the per-app requests, it would double count the existing requests. If the current count was say, one under the limit, an error would be thrown even though it was being replaced which should have resulted in no net change to the request count limit if working correctly. This patch will allow for the requests to be removed prior to creation so that request counts are tabulated correctly. Bug: 185849563 Bug: 183785319 Test: atest FrameworksNetTests Change-Id: I13da0c81256cc02bea6aff2fe1ef99d6f6b0e764 --- .../android/server/ConnectivityService.java | 111 +++++++++++++----- .../server/ConnectivityServiceTest.java | 86 ++++++++++++-- 2 files changed, 163 insertions(+), 34 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2216e89805..6460053e1b 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -319,7 +319,8 @@ public class ConnectivityService extends IConnectivityManager.Stub private static final int MAX_NETWORK_REQUESTS_PER_UID = 100; // The maximum number of network request allowed for system UIDs before an exception is thrown. - private static final int MAX_NETWORK_REQUESTS_PER_SYSTEM_UID = 250; + @VisibleForTesting + static final int MAX_NETWORK_REQUESTS_PER_SYSTEM_UID = 250; @VisibleForTesting protected int mLingerDelayMs; // Can't be final, or test subclass constructors can't change it. @@ -336,7 +337,8 @@ public class ConnectivityService extends IConnectivityManager.Stub protected final PermissionMonitor mPermissionMonitor; private final PerUidCounter mNetworkRequestCounter; - private final PerUidCounter mSystemNetworkRequestCounter; + @VisibleForTesting + final PerUidCounter mSystemNetworkRequestCounter; private volatile boolean mLockdownEnabled; @@ -1047,8 +1049,9 @@ public class ConnectivityService extends IConnectivityManager.Stub private final int mMaxCountPerUid; // Map from UID to number of NetworkRequests that UID has filed. + @VisibleForTesting @GuardedBy("mUidToNetworkRequestCount") - private final SparseIntArray mUidToNetworkRequestCount = new SparseIntArray(); + final SparseIntArray mUidToNetworkRequestCount = new SparseIntArray(); /** * Constructor @@ -1072,15 +1075,20 @@ public class ConnectivityService extends IConnectivityManager.Stub */ public void incrementCountOrThrow(final int uid) { synchronized (mUidToNetworkRequestCount) { - final int networkRequests = mUidToNetworkRequestCount.get(uid, 0) + 1; - if (networkRequests >= mMaxCountPerUid) { - throw new ServiceSpecificException( - ConnectivityManager.Errors.TOO_MANY_REQUESTS); - } - mUidToNetworkRequestCount.put(uid, networkRequests); + incrementCountOrThrow(uid, 1 /* numToIncrement */); } } + private void incrementCountOrThrow(final int uid, final int numToIncrement) { + final int newRequestCount = + mUidToNetworkRequestCount.get(uid, 0) + numToIncrement; + if (newRequestCount >= mMaxCountPerUid) { + throw new ServiceSpecificException( + ConnectivityManager.Errors.TOO_MANY_REQUESTS); + } + mUidToNetworkRequestCount.put(uid, newRequestCount); + } + /** * Decrements the request count of the given uid. * @@ -1088,16 +1096,50 @@ public class ConnectivityService extends IConnectivityManager.Stub */ public void decrementCount(final int uid) { synchronized (mUidToNetworkRequestCount) { - final int requests = mUidToNetworkRequestCount.get(uid, 0); - if (requests < 1) { - logwtf("BUG: too small request count " + requests + " for UID " + uid); - } else if (requests == 1) { - mUidToNetworkRequestCount.delete(uid); - } else { - mUidToNetworkRequestCount.put(uid, requests - 1); - } + decrementCount(uid, 1 /* numToDecrement */); } } + + private void decrementCount(final int uid, final int numToDecrement) { + final int newRequestCount = + mUidToNetworkRequestCount.get(uid, 0) - numToDecrement; + if (newRequestCount < 0) { + logwtf("BUG: too small request count " + newRequestCount + " for UID " + uid); + } else if (newRequestCount == 0) { + mUidToNetworkRequestCount.delete(uid); + } else { + mUidToNetworkRequestCount.put(uid, newRequestCount); + } + } + + /** + * Used to adjust the request counter for the per-app API flows. Directly adjusting the + * counter is not ideal however in the per-app flows, the nris can't be removed until they + * are used to create the new nris upon set. Therefore the request count limit can be + * artificially hit. This method is used as a workaround for this particular case so that + * the request counts are accounted for correctly. + * @param uid the uid to adjust counts for + * @param numOfNewRequests the new request count to account for + * @param r the runnable to execute + */ + public void transact(final int uid, final int numOfNewRequests, @NonNull final Runnable r) { + // This should only be used on the handler thread as per all current and foreseen + // use-cases. ensureRunningOnConnectivityServiceThread() can't be used because there is + // no ref to the outer ConnectivityService. + synchronized (mUidToNetworkRequestCount) { + final int reqCountOverage = getCallingUidRequestCountOverage(uid, numOfNewRequests); + decrementCount(uid, reqCountOverage); + r.run(); + incrementCountOrThrow(uid, reqCountOverage); + } + } + + private int getCallingUidRequestCountOverage(final int uid, final int numOfNewRequests) { + final int newUidRequestCount = mUidToNetworkRequestCount.get(uid, 0) + + numOfNewRequests; + return newUidRequestCount >= MAX_NETWORK_REQUESTS_PER_SYSTEM_UID + ? newUidRequestCount - (MAX_NETWORK_REQUESTS_PER_SYSTEM_UID - 1) : 0; + } } /** @@ -9569,9 +9611,13 @@ public class ConnectivityService extends IConnectivityManager.Stub validateNetworkCapabilitiesOfProfileNetworkPreference(preference.capabilities); mProfileNetworkPreferences = mProfileNetworkPreferences.plus(preference); - final ArraySet nris = - createNrisFromProfileNetworkPreferences(mProfileNetworkPreferences); - replaceDefaultNetworkRequestsForPreference(nris); + mSystemNetworkRequestCounter.transact( + mDeps.getCallingUid(), mProfileNetworkPreferences.preferences.size(), + () -> { + final ArraySet nris = + createNrisFromProfileNetworkPreferences(mProfileNetworkPreferences); + replaceDefaultNetworkRequestsForPreference(nris); + }); // Finally, rematch. rematchAllNetworksAndRequests(); @@ -9657,9 +9703,16 @@ public class ConnectivityService extends IConnectivityManager.Stub } mOemNetworkPreferencesLogs.log("UPDATE INITIATED: " + preference); - final ArraySet nris = - new OemNetworkRequestFactory().createNrisFromOemNetworkPreferences(preference); - replaceDefaultNetworkRequestsForPreference(nris); + final int uniquePreferenceCount = new ArraySet<>( + preference.getNetworkPreferences().values()).size(); + mSystemNetworkRequestCounter.transact( + mDeps.getCallingUid(), uniquePreferenceCount, + () -> { + final ArraySet nris = + new OemNetworkRequestFactory() + .createNrisFromOemNetworkPreferences(preference); + replaceDefaultNetworkRequestsForPreference(nris); + }); mOemNetworkPreferences = preference; if (null != listener) { @@ -9684,10 +9737,14 @@ public class ConnectivityService extends IConnectivityManager.Stub final ArraySet perAppCallbackRequestsToUpdate = getPerAppCallbackRequestsToUpdate(); final ArraySet nrisToRegister = new ArraySet<>(nris); - nrisToRegister.addAll( - createPerAppCallbackRequestsToRegister(perAppCallbackRequestsToUpdate)); - handleRemoveNetworkRequests(perAppCallbackRequestsToUpdate); - handleRegisterNetworkRequests(nrisToRegister); + mSystemNetworkRequestCounter.transact( + mDeps.getCallingUid(), perAppCallbackRequestsToUpdate.size(), + () -> { + nrisToRegister.addAll( + createPerAppCallbackRequestsToRegister(perAppCallbackRequestsToUpdate)); + handleRemoveNetworkRequests(perAppCallbackRequestsToUpdate); + handleRegisterNetworkRequests(nrisToRegister); + }); } /** diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 91811f66ea..c58e937006 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -10246,12 +10246,15 @@ public class ConnectivityServiceTest { return UidRange.createForUser(UserHandle.of(userId)); } - private void mockGetApplicationInfo(@NonNull final String packageName, @NonNull final int uid) - throws Exception { + private void mockGetApplicationInfo(@NonNull final String packageName, @NonNull final int uid) { final ApplicationInfo applicationInfo = new ApplicationInfo(); applicationInfo.uid = uid; - when(mPackageManager.getApplicationInfo(eq(packageName), anyInt())) - .thenReturn(applicationInfo); + try { + when(mPackageManager.getApplicationInfo(eq(packageName), anyInt())) + .thenReturn(applicationInfo); + } catch (Exception e) { + fail(e.getMessage()); + } } private void mockGetApplicationInfoThrowsNameNotFound(@NonNull final String packageName) @@ -10272,8 +10275,7 @@ public class ConnectivityServiceTest { } private OemNetworkPreferences createDefaultOemNetworkPreferences( - @OemNetworkPreferences.OemNetworkPreference final int preference) - throws Exception { + @OemNetworkPreferences.OemNetworkPreference final int preference) { // Arrange PackageManager mocks mockGetApplicationInfo(TEST_PACKAGE_NAME, TEST_PACKAGE_UID); @@ -10750,11 +10752,13 @@ public class ConnectivityServiceTest { mDone.complete(new Object()); } - void expectOnComplete() throws Exception { + void expectOnComplete() { try { mDone.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); } catch (TimeoutException e) { fail("Expected onComplete() not received after " + TIMEOUT_MS + " ms"); + } catch (Exception e) { + fail(e.getMessage()); } } @@ -12369,4 +12373,72 @@ public class ConnectivityServiceTest { expected, () -> mCm.registerNetworkCallback(getRequestWithSubIds(), new NetworkCallback())); } + + /** + * Validate request counts are counted accurately on setProfileNetworkPreference on set/replace. + */ + @Test + public void testProfileNetworkPrefCountsRequestsCorrectlyOnSet() throws Exception { + final UserHandle testHandle = setupEnterpriseNetwork(); + testRequestCountLimits(() -> { + // Set initially to test the limit prior to having existing requests. + final TestOnCompleteListener listener = new TestOnCompleteListener(); + mCm.setProfileNetworkPreference(testHandle, PROFILE_NETWORK_PREFERENCE_ENTERPRISE, + Runnable::run, listener); + listener.expectOnComplete(); + + // re-set so as to test the limit as part of replacing existing requests. + mCm.setProfileNetworkPreference(testHandle, PROFILE_NETWORK_PREFERENCE_ENTERPRISE, + Runnable::run, listener); + listener.expectOnComplete(); + }); + } + + /** + * Validate request counts are counted accurately on setOemNetworkPreference on set/replace. + */ + @Test + public void testSetOemNetworkPreferenceCountsRequestsCorrectlyOnSet() throws Exception { + mockHasSystemFeature(PackageManager.FEATURE_AUTOMOTIVE, true); + @OemNetworkPreferences.OemNetworkPreference final int networkPref = + OEM_NETWORK_PREFERENCE_OEM_PRIVATE_ONLY; + testRequestCountLimits(() -> { + // Set initially to test the limit prior to having existing requests. + final TestOemListenerCallback listener = new TestOemListenerCallback(); + mService.setOemNetworkPreference( + createDefaultOemNetworkPreferences(networkPref), listener); + listener.expectOnComplete(); + + // re-set so as to test the limit as part of replacing existing requests. + mService.setOemNetworkPreference( + createDefaultOemNetworkPreferences(networkPref), listener); + listener.expectOnComplete(); + }); + } + + private void testRequestCountLimits(@NonNull final Runnable r) throws Exception { + final ArraySet callbacks = new ArraySet<>(); + try { + final int requestCount = mService.mSystemNetworkRequestCounter + .mUidToNetworkRequestCount.get(Process.myUid()); + // The limit is hit when total requests <= limit. + final int maxCount = + ConnectivityService.MAX_NETWORK_REQUESTS_PER_SYSTEM_UID - requestCount; + // Need permission so registerDefaultNetworkCallback uses mSystemNetworkRequestCounter + withPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, () -> { + for (int i = 1; i < maxCount - 1; i++) { + final TestNetworkCallback cb = new TestNetworkCallback(); + mCm.registerDefaultNetworkCallback(cb); + callbacks.add(cb); + } + + // Code to run to check if it triggers a max request count limit error. + r.run(); + }); + } finally { + for (final TestNetworkCallback cb : callbacks) { + mCm.unregisterNetworkCallback(cb); + } + } + } }