From 00d92df6457a245524722afc0c5f19cb91ad0f63 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Tue, 5 Jul 2022 11:01:52 +0800 Subject: [PATCH] Use PerUidCounter in the connectivity static library This is a no-op refactoring which uses common PerUidCounter from the static library and delete the private implementation inside ConnectivityService. This refactoring includes: 1. Make a private implementation that inherite PerUidCounter to convert and rethrow exception to maintain backward compatibility. 2. Adjust per-uid max limit argument in the counter constructor since the private implementation is already buggy. 3. Use the getter in PerUidCounter for the existing tests. Test: testNetworkCallbackMaximum testProfileNetworkPrefCountsRequestsCorrectlyOnSet testRequestCountLimits Bug: 235771502 Change-Id: I5c49edab18774acf819828201041c9931fabccc4 --- .../android/server/ConnectivityService.java | 125 +++++++----------- .../connectivity/QosCallbackTracker.java | 4 +- .../server/ConnectivityServiceTest.java | 8 +- 3 files changed, 52 insertions(+), 85 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 7342efa886..617894f46a 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -257,6 +257,7 @@ import com.android.net.module.util.LinkPropertiesUtils.CompareOrUpdateResult; import com.android.net.module.util.LinkPropertiesUtils.CompareResult; import com.android.net.module.util.LocationPermissionChecker; import com.android.net.module.util.NetworkCapabilitiesUtils; +import com.android.net.module.util.PerUidCounter; import com.android.net.module.util.PermissionUtils; import com.android.net.module.util.TcUtils; import com.android.net.module.util.netlink.InetDiagMessage; @@ -386,9 +387,9 @@ public class ConnectivityService extends IConnectivityManager.Stub protected final PermissionMonitor mPermissionMonitor; @VisibleForTesting - final PerUidCounter mNetworkRequestCounter; + final RequestInfoPerUidCounter mNetworkRequestCounter; @VisibleForTesting - final PerUidCounter mSystemNetworkRequestCounter; + final RequestInfoPerUidCounter mSystemNetworkRequestCounter; private volatile boolean mLockdownEnabled; @@ -1185,71 +1186,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - /** - * Keeps track of the number of requests made under different uids. - */ - // TODO: Remove the hack and use com.android.net.module.util.PerUidCounter instead. - public static class PerUidCounter { - private final int mMaxCountPerUid; - - // Map from UID to number of NetworkRequests that UID has filed. - @VisibleForTesting - @GuardedBy("mUidToNetworkRequestCount") - final SparseIntArray mUidToNetworkRequestCount = new SparseIntArray(); - - /** - * Constructor - * - * @param maxCountPerUid the maximum count per uid allowed - */ - public PerUidCounter(final int maxCountPerUid) { - mMaxCountPerUid = maxCountPerUid; - } - - /** - * Increments the request count of the given uid. Throws an exception if the number - * of open requests for the uid exceeds the value of maxCounterPerUid which is the value - * passed into the constructor. see: {@link #PerUidCounter(int)}. - * - * @throws ServiceSpecificException with - * {@link ConnectivityManager.Errors.TOO_MANY_REQUESTS} if the number of requests for - * the uid exceed the allowed number. - * - * @param uid the uid that the request was made under - */ - public void incrementCountOrThrow(final int uid) { - synchronized (mUidToNetworkRequestCount) { - final int newRequestCount = mUidToNetworkRequestCount.get(uid, 0) + 1; - if (newRequestCount >= mMaxCountPerUid) { - throw new ServiceSpecificException( - ConnectivityManager.Errors.TOO_MANY_REQUESTS, - "Uid " + uid + " exceeded its allotted requests limit"); - } - mUidToNetworkRequestCount.put(uid, newRequestCount); - } - } - - /** - * Decrements the request count of the given uid. - * - * @param uid the uid that the request was made under - */ - public void decrementCount(final int uid) { - synchronized (mUidToNetworkRequestCount) { - /* numToDecrement */ - final int newRequestCount = mUidToNetworkRequestCount.get(uid, 0) - 1; - 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); - } - } - } - - } - /** * Dependencies of ConnectivityService, for injection in tests. */ @@ -1464,8 +1400,13 @@ public class ConnectivityService extends IConnectivityManager.Stub mNetIdManager = mDeps.makeNetIdManager(); mContext = Objects.requireNonNull(context, "missing Context"); mResources = deps.getResources(mContext); - mNetworkRequestCounter = new PerUidCounter(MAX_NETWORK_REQUESTS_PER_UID); - mSystemNetworkRequestCounter = new PerUidCounter(MAX_NETWORK_REQUESTS_PER_SYSTEM_UID); + // The legacy PerUidCounter is buggy and throwing exception at count == limit. + // Pass limit - 1 to maintain backward compatibility. + // TODO: Remove the workaround. + mNetworkRequestCounter = + new RequestInfoPerUidCounter(MAX_NETWORK_REQUESTS_PER_UID - 1); + mSystemNetworkRequestCounter = + new RequestInfoPerUidCounter(MAX_NETWORK_REQUESTS_PER_SYSTEM_UID - 1); mMetricsLog = logger; mNetworkRanker = new NetworkRanker(); @@ -4763,7 +4704,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } } - nri.decrementRequestCount(); + nri.mPerUidCounter.decrementCount(nri.mUid); mNetworkRequestInfoLogs.log("RELEASE " + nri); checkNrisConsistency(nri); @@ -4866,7 +4807,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - private PerUidCounter getRequestCounter(NetworkRequestInfo nri) { + private RequestInfoPerUidCounter getRequestCounter(NetworkRequestInfo nri) { return checkAnyPermissionOf( nri.mPid, nri.mUid, NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) ? mSystemNetworkRequestCounter : mNetworkRequestCounter; @@ -6230,7 +6171,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final String mCallingAttributionTag; // Counter keeping track of this NRI. - final PerUidCounter mPerUidCounter; + final RequestInfoPerUidCounter mPerUidCounter; // Effective UID of this request. This is different from mUid when a privileged process // files a request on behalf of another UID. This UID is used to determine blocked status, @@ -6396,10 +6337,6 @@ public class ConnectivityService extends IConnectivityManager.Stub return Collections.unmodifiableList(tempRequests); } - void decrementRequestCount() { - mPerUidCounter.decrementCount(mUid); - } - void linkDeathRecipient() { if (null != mBinder) { try { @@ -6461,6 +6398,38 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + // Keep backward compatibility since the ServiceSpecificException is used by + // the API surface, see {@link ConnectivityManager#convertServiceException}. + public static class RequestInfoPerUidCounter extends PerUidCounter { + RequestInfoPerUidCounter(int maxCountPerUid) { + super(maxCountPerUid); + } + + @Override + public synchronized void incrementCountOrThrow(int uid) { + try { + super.incrementCountOrThrow(uid); + } catch (IllegalStateException e) { + throw new ServiceSpecificException( + ConnectivityManager.Errors.TOO_MANY_REQUESTS, + "Uid " + uid + " exceeded its allotted requests limit"); + } + } + + @Override + public synchronized void decrementCountOrThrow(int uid) { + throw new UnsupportedOperationException("Use decrementCount instead."); + } + + public synchronized void decrementCount(int uid) { + try { + super.decrementCountOrThrow(uid); + } catch (IllegalStateException e) { + logwtf("Exception when decrement per uid request count: ", e); + } + } + } + // 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, @@ -9949,7 +9918,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // Decrement the reference count for this NetworkRequestInfo. The reference count is // incremented when the NetworkRequestInfo is created as part of // enforceRequestCountLimit(). - nri.decrementRequestCount(); + nri.mPerUidCounter.decrementCount(nri.mUid); return; } @@ -10015,7 +9984,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // Decrement the reference count for this NetworkRequestInfo. The reference count is // incremented when the NetworkRequestInfo is created as part of // enforceRequestCountLimit(). - nri.decrementRequestCount(); + nri.mPerUidCounter.decrementCount(nri.mUid); iCb.unlinkToDeath(cbInfo, 0); } diff --git a/service/src/com/android/server/connectivity/QosCallbackTracker.java b/service/src/com/android/server/connectivity/QosCallbackTracker.java index b6ab47b276..336a399bb9 100644 --- a/service/src/com/android/server/connectivity/QosCallbackTracker.java +++ b/service/src/com/android/server/connectivity/QosCallbackTracker.java @@ -52,7 +52,7 @@ public class QosCallbackTracker { private final Handler mConnectivityServiceHandler; @NonNull - private final ConnectivityService.PerUidCounter mNetworkRequestCounter; + private final ConnectivityService.RequestInfoPerUidCounter mNetworkRequestCounter; /** * Each agent gets a unique callback id that is used to proxy messages back to the original @@ -78,7 +78,7 @@ public class QosCallbackTracker { * uid */ public QosCallbackTracker(@NonNull final Handler connectivityServiceHandler, - final ConnectivityService.PerUidCounter networkRequestCounter) { + final ConnectivityService.RequestInfoPerUidCounter networkRequestCounter) { mConnectivityServiceHandler = connectivityServiceHandler; mNetworkRequestCounter = networkRequestCounter; } diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 06f730054c..0f263b26da 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -15761,7 +15761,7 @@ public class ConnectivityServiceTest { 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) + - mService.mNetworkRequestCounter.get(otherAppUid) - 1; final NetworkCallback[] callbacks = new NetworkCallback[remainingCount]; doAsUid(otherAppUid, () -> { @@ -15815,8 +15815,7 @@ public class ConnectivityServiceTest { @NonNull final ExceptionalRunnable r) throws Exception { final ArraySet callbacks = new ArraySet<>(); try { - final int requestCount = mService.mSystemNetworkRequestCounter - .mUidToNetworkRequestCount.get(Process.myUid()); + final int requestCount = mService.mSystemNetworkRequestCounter.get(Process.myUid()); // The limit is hit when total requests = limit - 1, and exceeded with a crash when // total requests >= limit. final int countToFile = @@ -15829,8 +15828,7 @@ public class ConnectivityServiceTest { callbacks.add(cb); } assertEquals(MAX_NETWORK_REQUESTS_PER_SYSTEM_UID - 1 - countToLeaveAvailable, - mService.mSystemNetworkRequestCounter - .mUidToNetworkRequestCount.get(Process.myUid())); + mService.mSystemNetworkRequestCounter.get(Process.myUid())); }); // Code to run to check if it triggers a max request count limit error. r.run();