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)
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index c8a0b7f..e34c064 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -324,7 +324,8 @@
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 @@
@VisibleForTesting
protected final PermissionMonitor mPermissionMonitor;
- private final PerUidCounter mNetworkRequestCounter;
+ @VisibleForTesting
+ final PerUidCounter mNetworkRequestCounter;
@VisibleForTesting
final PerUidCounter mSystemNetworkRequestCounter;
@@ -1154,9 +1156,20 @@
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 @@
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 @@
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,