From 5234f3acc64056ec290e0c61ae954e0bf5455322 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 29 Jul 2021 20:03:04 +0900 Subject: [PATCH] Fix a crash when changing preferences The crash occurs when some app has more than half its limit in requests that will need to be moved to some other default network upon changing the preferences. This will send the requests for this app over the limit temporarily when creating new requests for the reevaluated ones. While ConnectivityService has a provision for making a transaction-like addition/removal of requests that is meant to avoid exactly this kind of crash with the transact() method on PerUidCounter, the code only transacts on mSystemNetworkRequestCounter. But these requests are counted in the mNetworkRequestCounters, which is not part of the transaction, causing the crash anyway. To avoid the problem, this patch allows the request counters to go over the max if and only if the system server is updating the request counts for a UID other than its own. This should allow only the case where ConnectivityService is moving the requests over to the new per-uid default, while keeping the exception when registering from an app (then the calling UID is not the system server), or when the system server registers its own requests (then the UID inside the request is that of the system server). A much better solution than this patch would be to completely eliminate the transact() method by somehow unregistering the old ones before creating the new ones. However this would be a much bigger and difficult patch than this, and much more dangerous, because callers depend on the list of requests to find out the old requests to remove, so they have to be created first. Another possible clean solution would be to count the requests not in the NRI constructor, but later. This would be more error-prone though because it would be very easy to create an NRI without counting it. Bug: 192470012 Test: ConnectivityServiceTest. Improve tests so they catch this case. Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/1781202 Merged-In: Ia482e6fbf2bf300ce6cbaca72810d394ed201b98 Change-Id: I6744d2f60d6bd664f048b532a58461c110a5b7fe (cherry picked from commit 916aeb7b0dab0198858fe265c5675140276700bd) --- .../android/server/ConnectivityService.java | 25 ++- .../server/ConnectivityServiceTest.java | 178 +++++++++++++----- 2 files changed, 145 insertions(+), 58 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index c8a0b7f0d9..e34c0640c2 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -324,7 +324,8 @@ public class ConnectivityService extends IConnectivityManager.Stub private static final int DEFAULT_NASCENT_DELAY_MS = 5_000; // The maximum number of network request allowed per uid before an exception is thrown. - private static final int MAX_NETWORK_REQUESTS_PER_UID = 100; + @VisibleForTesting + static final int MAX_NETWORK_REQUESTS_PER_UID = 100; // The maximum number of network request allowed for system UIDs before an exception is thrown. @VisibleForTesting @@ -344,7 +345,8 @@ public class ConnectivityService extends IConnectivityManager.Stub @VisibleForTesting protected final PermissionMonitor mPermissionMonitor; - private final PerUidCounter mNetworkRequestCounter; + @VisibleForTesting + final PerUidCounter mNetworkRequestCounter; @VisibleForTesting final PerUidCounter mSystemNetworkRequestCounter; @@ -1154,9 +1156,20 @@ public class ConnectivityService extends IConnectivityManager.Stub private void incrementCountOrThrow(final int uid, final int numToIncrement) { final int newRequestCount = mUidToNetworkRequestCount.get(uid, 0) + numToIncrement; - if (newRequestCount >= mMaxCountPerUid) { + if (newRequestCount >= mMaxCountPerUid + // HACK : the system server is allowed to go over the request count limit + // when it is creating requests on behalf of another app (but not itself, + // so it can still detect its own request leaks). This only happens in the + // per-app API flows in which case the old requests for that particular + // UID will be removed soon. + // TODO : instead of this hack, addPerAppDefaultNetworkRequests and other + // users of transact() should unregister the requests to decrease the count + // before they increase it again by creating a new NRI. Then remove the + // transact() method. + && (Process.myUid() == uid || Process.myUid() != Binder.getCallingUid())) { throw new ServiceSpecificException( - ConnectivityManager.Errors.TOO_MANY_REQUESTS); + ConnectivityManager.Errors.TOO_MANY_REQUESTS, + "Uid " + uid + " exceeded its allotted requests limit"); } mUidToNetworkRequestCount.put(uid, newRequestCount); } @@ -5817,7 +5830,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mUid = nri.mUid; mAsUid = nri.mAsUid; mPendingIntent = nri.mPendingIntent; - mPerUidCounter = getRequestCounter(this); + mPerUidCounter = nri.mPerUidCounter; mPerUidCounter.incrementCountOrThrow(mUid); mCallbackFlags = nri.mCallbackFlags; mCallingAttributionTag = nri.mCallingAttributionTag; @@ -10147,7 +10160,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkRequestInfo trackingNri = getDefaultRequestTrackingUid(callbackRequest.mAsUid); - // If this nri is not being tracked, the change it back to an untracked nri. + // If this nri is not being tracked, then change it back to an untracked nri. if (trackingNri == mDefaultRequest) { callbackRequestsToRegister.add(new NetworkRequestInfo( callbackRequest, diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 0f0ac1223c..9dde31ab91 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -125,6 +125,7 @@ import static android.net.resolv.aidl.IDnsResolverUnsolicitedEventListener.VALID import static android.os.Process.INVALID_UID; import static android.system.OsConstants.IPPROTO_TCP; +import static com.android.server.ConnectivityService.MAX_NETWORK_REQUESTS_PER_SYSTEM_UID; import static com.android.server.ConnectivityService.PREFERENCE_PRIORITY_MOBILE_DATA_PREFERERRED; import static com.android.server.ConnectivityService.PREFERENCE_PRIORITY_OEM; import static com.android.server.ConnectivityService.PREFERENCE_PRIORITY_PROFILE; @@ -539,6 +540,9 @@ public class ConnectivityServiceTest { private final LinkedBlockingQueue mStartedActivities = new LinkedBlockingQueue<>(); // Map of permission name -> PermissionManager.Permission_{GRANTED|DENIED} constant + // For permissions granted across the board, the key is only the permission name. + // For permissions only granted to a combination of uid/pid, the key + // is ",,". PID+UID permissons have priority over generic ones. private final HashMap mMockedPermissions = new HashMap<>(); MockContext(Context base, ContentProvider settingsProvider) { @@ -640,30 +644,40 @@ public class ConnectivityServiceTest { return mPackageManager; } - private int checkMockedPermission(String permission, Supplier ifAbsent) { - final Integer granted = mMockedPermissions.get(permission); - return granted != null ? granted : ifAbsent.get(); + private int checkMockedPermission(String permission, int pid, int uid, + Supplier ifAbsent) { + final Integer granted = mMockedPermissions.get(permission + "," + pid + "," + uid); + if (null != granted) { + return granted; + } + final Integer allGranted = mMockedPermissions.get(permission); + if (null != allGranted) { + return allGranted; + } + return ifAbsent.get(); } @Override public int checkPermission(String permission, int pid, int uid) { - return checkMockedPermission( - permission, () -> super.checkPermission(permission, pid, uid)); + return checkMockedPermission(permission, pid, uid, + () -> super.checkPermission(permission, pid, uid)); } @Override public int checkCallingOrSelfPermission(String permission) { - return checkMockedPermission( - permission, () -> super.checkCallingOrSelfPermission(permission)); + return checkMockedPermission(permission, Process.myPid(), Process.myUid(), + () -> super.checkCallingOrSelfPermission(permission)); } @Override public void enforceCallingOrSelfPermission(String permission, String message) { - final Integer granted = mMockedPermissions.get(permission); - if (granted == null) { - super.enforceCallingOrSelfPermission(permission, message); - return; - } + final Integer granted = checkMockedPermission(permission, + Process.myPid(), Process.myUid(), + () -> { + super.enforceCallingOrSelfPermission(permission, message); + // enforce will crash if the permission is not granted + return PERMISSION_GRANTED; + }); if (!granted.equals(PERMISSION_GRANTED)) { throw new SecurityException("[Test] permission denied: " + permission); @@ -673,6 +687,8 @@ public class ConnectivityServiceTest { /** * Mock checks for the specified permission, and have them behave as per {@code granted}. * + * This will apply across the board no matter what the checked UID and PID are. + * *

Passing null reverts to default behavior, which does a real permission check on the * test package. * @param granted One of {@link PackageManager#PERMISSION_GRANTED} or @@ -682,6 +698,21 @@ public class ConnectivityServiceTest { mMockedPermissions.put(permission, granted); } + /** + * Mock checks for the specified permission, and have them behave as per {@code granted}. + * + * This will only apply to the passed UID and PID. + * + *

Passing null reverts to default behavior, which does a real permission check on the + * test package. + * @param granted One of {@link PackageManager#PERMISSION_GRANTED} or + * {@link PackageManager#PERMISSION_DENIED}. + */ + public void setPermission(String permission, int pid, int uid, Integer granted) { + final String key = permission + "," + pid + "," + uid; + mMockedPermissions.put(key, granted); + } + @Override public Intent registerReceiverForAllUsers(@Nullable BroadcastReceiver receiver, @NonNull IntentFilter filter, @Nullable String broadcastPermission, @@ -1569,15 +1600,21 @@ public class ConnectivityServiceTest { } private void withPermission(String permission, ExceptionalRunnable r) throws Exception { - if (mServiceContext.checkCallingOrSelfPermission(permission) == PERMISSION_GRANTED) { - r.run(); - return; - } try { mServiceContext.setPermission(permission, PERMISSION_GRANTED); r.run(); } finally { - mServiceContext.setPermission(permission, PERMISSION_DENIED); + mServiceContext.setPermission(permission, null); + } + } + + private void withPermission(String permission, int pid, int uid, ExceptionalRunnable r) + throws Exception { + try { + mServiceContext.setPermission(permission, pid, uid, PERMISSION_GRANTED); + r.run(); + } finally { + mServiceContext.setPermission(permission, pid, uid, null); } } @@ -13368,17 +13405,45 @@ public class ConnectivityServiceTest { @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); + final TestOnCompleteListener listener = new TestOnCompleteListener(); + // Leave one request available so the profile preference can be set. + testRequestCountLimits(1 /* countToLeaveAvailable */, () -> { + withPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, + Process.myPid(), Process.myUid(), () -> { + // Set initially to test the limit prior to having existing requests. + 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); + // Simulate filing requests as some app on the work profile + final int otherAppUid = UserHandle.getUid(TEST_WORK_PROFILE_USER_ID, + UserHandle.getAppId(Process.myUid() + 1)); + final int remainingCount = ConnectivityService.MAX_NETWORK_REQUESTS_PER_UID + - mService.mNetworkRequestCounter.mUidToNetworkRequestCount.get(otherAppUid) + - 1; + final NetworkCallback[] callbacks = new NetworkCallback[remainingCount]; + doAsUid(otherAppUid, () -> { + for (int i = 0; i < remainingCount; ++i) { + callbacks[i] = new TestableNetworkCallback(); + mCm.registerDefaultNetworkCallback(callbacks[i]); + } + }); + + withPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, + Process.myPid(), Process.myUid(), () -> { + // 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(); + + doAsUid(otherAppUid, () -> { + for (final NetworkCallback callback : callbacks) { + mCm.unregisterNetworkCallback(callback); + } + }); }); } @@ -13390,39 +13455,45 @@ public class ConnectivityServiceTest { 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(); + // Leave one request available so the OEM preference can be set. + testRequestCountLimits(1 /* countToLeaveAvailable */, () -> + withPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, () -> { + // 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(); - }); + // 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 { + private void testRequestCountLimits(final int countToLeaveAvailable, + @NonNull final ExceptionalRunnable 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; + // The limit is hit when total requests = limit - 1, and exceeded with a crash when + // total requests >= limit. + final int countToFile = + MAX_NETWORK_REQUESTS_PER_SYSTEM_UID - requestCount - countToLeaveAvailable; // Need permission so registerDefaultNetworkCallback uses mSystemNetworkRequestCounter withPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, () -> { - for (int i = 1; i < maxCount - 1; i++) { + for (int i = 1; i < countToFile; 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(); + assertEquals(MAX_NETWORK_REQUESTS_PER_SYSTEM_UID - 1 - countToLeaveAvailable, + mService.mSystemNetworkRequestCounter + .mUidToNetworkRequestCount.get(Process.myUid())); }); + // 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); @@ -13667,15 +13738,18 @@ public class ConnectivityServiceTest { public void testMobileDataPreferredUidsChangedCountsRequestsCorrectlyOnSet() throws Exception { ConnectivitySettingsManager.setMobileDataPreferredUids(mServiceContext, Set.of(PRIMARY_USER_HANDLE.getUid(TEST_PACKAGE_UID))); - testRequestCountLimits(() -> { - // Set initially to test the limit prior to having existing requests. - mService.updateMobileDataPreferredUids(); - waitForIdle(); + // Leave one request available so MDO preference set up above can be set. + testRequestCountLimits(1 /* countToLeaveAvailable */, () -> + withPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, + Process.myPid(), Process.myUid(), () -> { + // Set initially to test the limit prior to having existing requests. + mService.updateMobileDataPreferredUids(); + waitForIdle(); - // re-set so as to test the limit as part of replacing existing requests. - mService.updateMobileDataPreferredUids(); - waitForIdle(); - }); + // re-set so as to test the limit as part of replacing existing requests + mService.updateMobileDataPreferredUids(); + waitForIdle(); + })); } @Test