Merge "Fix a crash when changing preferences"

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);
         }
@@ -5845,7 +5858,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;
@@ -10248,7 +10261,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,

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_ORDER_MOBILE_DATA_PREFERERRED;
 import static com.android.server.ConnectivityService.PREFERENCE_ORDER_OEM;
 import static com.android.server.ConnectivityService.PREFERENCE_ORDER_PROFILE;
@@ -540,6 +541,9 @@ public class ConnectivityServiceTest {
         private final LinkedBlockingQueue<Intent> 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 "<permission name>,<pid>,<uid>". PID+UID permissons have priority over generic ones.
         private final HashMap<String, Integer> mMockedPermissions = new HashMap<>();
 
         MockContext(Context base, ContentProvider settingsProvider) {
@@ -641,30 +645,40 @@ public class ConnectivityServiceTest {
             return mPackageManager;
         }
 
-        private int checkMockedPermission(String permission, Supplier<Integer> ifAbsent) {
+        private int checkMockedPermission(String permission, int pid, int uid,
-            final Integer granted = mMockedPermissions.get(permission);
+                Supplier<Integer> ifAbsent) {
-            return granted != null ? granted : ifAbsent.get();
+            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(
+            return checkMockedPermission(permission, pid, uid,
-                    permission, () -> super.checkPermission(permission, pid, uid));
+                    () -> super.checkPermission(permission, pid, uid));
         }
 
         @Override
         public int checkCallingOrSelfPermission(String permission) {
-            return checkMockedPermission(
+            return checkMockedPermission(permission, Process.myPid(), Process.myUid(),
-                    permission, () -> super.checkCallingOrSelfPermission(permission));
+                    () -> super.checkCallingOrSelfPermission(permission));
         }
 
         @Override
         public void enforceCallingOrSelfPermission(String permission, String message) {
-            final Integer granted = mMockedPermissions.get(permission);
+            final Integer granted = checkMockedPermission(permission,
-            if (granted == null) {
+                    Process.myPid(), Process.myUid(),
+                    () -> {
                         super.enforceCallingOrSelfPermission(permission, message);
-                return;
+                        // enforce will crash if the permission is not granted
-            }
+                        return PERMISSION_GRANTED;
+                    });
 
             if (!granted.equals(PERMISSION_GRANTED)) {
                 throw new SecurityException("[Test] permission denied: " + permission);
@@ -674,6 +688,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.
+         *
          * <p>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
@@ -683,6 +699,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.
+         *
+         * <p>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,
@@ -1564,15 +1595,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);
         }
     }
 
@@ -13283,17 +13320,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,
+        // 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();
 
+            // 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,
+                        mCm.setProfileNetworkPreference(testHandle,
-                    Runnable::run, listener);
+                                PROFILE_NETWORK_PREFERENCE_ENTERPRISE, Runnable::run, listener);
+                    });
             listener.expectOnComplete();
+
+            doAsUid(otherAppUid, () -> {
+                for (final NetworkCallback callback : callbacks) {
+                    mCm.unregisterNetworkCallback(callback);
+                }
+            });
         });
     }
 
@@ -13305,7 +13370,9 @@ public class ConnectivityServiceTest {
         mockHasSystemFeature(PackageManager.FEATURE_AUTOMOTIVE, true);
         @OemNetworkPreferences.OemNetworkPreference final int networkPref =
                 OEM_NETWORK_PREFERENCE_OEM_PRIVATE_ONLY;
-        testRequestCountLimits(() -> {
+        // 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(
@@ -13316,28 +13383,32 @@ public class ConnectivityServiceTest {
                     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<TestNetworkCallback> callbacks = new ArraySet<>();
         try {
             final int requestCount = mService.mSystemNetworkRequestCounter
                     .mUidToNetworkRequestCount.get(Process.myUid());
-            // The limit is hit when total requests <= limit.
+            // The limit is hit when total requests = limit - 1, and exceeded with a crash when
-            final int maxCount =
+            // total requests >= limit.
-                    ConnectivityService.MAX_NETWORK_REQUESTS_PER_SYSTEM_UID - requestCount;
+            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);
                 }
-
+                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);
@@ -13582,15 +13653,18 @@ public class ConnectivityServiceTest {
     public void testMobileDataPreferredUidsChangedCountsRequestsCorrectlyOnSet() throws Exception {
         ConnectivitySettingsManager.setMobileDataPreferredUids(mServiceContext,
                 Set.of(PRIMARY_USER_HANDLE.getUid(TEST_PACKAGE_UID)));
-        testRequestCountLimits(() -> {
+        // 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.
+                            // re-set so as to test the limit as part of replacing existing requests
                             mService.updateMobileDataPreferredUids();
                             waitForIdle();
-        });
+                        }));
     }
 
     @Test