From 06985411b7015e76bc934b3bab6961448bd1cfdb Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 1 Mar 2021 22:08:57 +0900 Subject: Fix a bug where UID ranges would not be removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a new preferences object is sent that no longer contains a particular app, a new set of requests will be generated. All requests corresponding to that app will be unregistered, and no new ones will be filed since the preferences no longer contain that app. The place where the UID ranges are removed however is in makeDefaultForApps(), which takes a request. As there no longer is a default request for this app, makeDefaultForApps() will never be called with a request for it, and the UID ranges will never be removed. This change applies an emergency fix with some side effects when setting a new preference. This is acceptable, but should ideally be fixed ; see TODO in the code for details. Test: FrameworksNetTests Test: TODO : Need a unit test for this Change-Id: Iac3f55af5d00d174460e1d4cdd31f581835dbaa6 --- .../java/com/android/server/ConnectivityService.java | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index b825abb396a7..12b094eaee41 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3831,7 +3831,24 @@ public class ConnectivityService extends IConnectivityManager.Stub removeListenRequestFromNetworks(req); } } - mDefaultNetworkRequests.remove(nri); + if (mDefaultNetworkRequests.remove(nri)) { + // If this request was one of the defaults, then the UID rules need to be updated + // WARNING : if the app(s) for which this network request is the default are doing + // traffic, this will kill their connected sockets, even if an equivalent request + // is going to be reinstated right away ; unconnected traffic will go on the default + // until the new default is set, which will happen very soon. + // TODO : The only way out of this is to diff old defaults and new defaults, and only + // remove ranges for those requests that won't have a replacement + final NetworkAgentInfo satisfier = nri.getSatisfier(); + if (null != satisfier) { + try { + mNetd.networkRemoveUidRanges(satisfier.network.getNetId(), + toUidRangeStableParcels(nri.getUids())); + } catch (RemoteException e) { + loge("Exception setting network preference default network", e); + } + } + } mNetworkRequestCounter.decrementCount(nri.mUid); mNetworkRequestInfoLogs.log("RELEASE " + nri); -- cgit v1.2.3-59-g8ed1b From b80f9b0097e70438f05a111117dea5ebb06795cc Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 1 Mar 2021 22:12:56 +0900 Subject: Fix a bug where a request would not be refcounted This ends up crashing the system when the request is unregistered, because the ref counter finds the count associated with the relevant UID is zero. Test: FrameworksNetTests Test: TODO : this needs a new unit test Change-Id: I0ee0ce925a826d35d8fd58cefb8a870e98ce9add --- services/core/java/com/android/server/ConnectivityService.java | 1 + 1 file changed, 1 insertion(+) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 12b094eaee41..2b6523a72e21 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5259,6 +5259,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mPid = nri.mPid; mUid = nri.mUid; mPendingIntent = nri.mPendingIntent; + mNetworkRequestCounter.incrementCountOrThrow(mUid); mCallingAttributionTag = nri.mCallingAttributionTag; } -- cgit v1.2.3-59-g8ed1b From 775feeb477e75fbde2453c800183282da4ccc776 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 1 Mar 2021 22:16:08 +0900 Subject: Fix a bug where callbacks would see blips When a new OemPreference object is set, the requests tracking that default will be re-created to track the new default. This is true regardless of whether that particular default has changed. Because the new copied request doesn't know about the old satisfier, the rematch code will mistakenly think this callback didn't have a network and will send a spurious onAvailable to it. This patch fixes this by simply copying the satisfier together with the rest of the NRI. The matching code will then understand the correct previous status. As a drive-by fix, this also fixes the annotations on the reassignment contents that can be null. They have more complex interactions (not everything can be null at the same time), but the old annotations were just putting @NonNull on nullable stuff. In the same line, while there used to be a case with a satisfier but no request when the satisfier is the no-service network, there may now be a case where the old satisfier is known but the old request isn't. Test: FrameworksNetTests Test: TODO : need a unit test for this Change-Id: Ide2567b226722ea9d35ebd8205943363e27647a2 --- .../com/android/server/ConnectivityService.java | 24 ++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2b6523a72e21..aacf2770d1c2 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5254,6 +5254,14 @@ public class ConnectivityService extends IConnectivityManager.Stub ensureAllNetworkRequestsHaveType(r); mRequests = initializeRequests(r); mNetworkRequestForCallback = nri.getNetworkRequestForCallback(); + // Note here that the satisfier may have corresponded to an old request, that + // this code doesn't try to take over. While it is a small discrepancy in the + // structure of these requests, it will be fixed by the next rematch and it's + // not as bad as having an NRI not storing its real satisfier. + // Fixing this discrepancy would require figuring out in the copying code what + // is the new request satisfied by this, which is a bit complex and not very + // useful as no code is using it until rematch fixes it. + mSatisfier = nri.mSatisfier; mMessenger = nri.mMessenger; mBinder = nri.mBinder; mPid = nri.mPid; @@ -7219,13 +7227,13 @@ public class ConnectivityService extends IConnectivityManager.Stub private static class NetworkReassignment { static class RequestReassignment { @NonNull public final NetworkRequestInfo mNetworkRequestInfo; - @NonNull public final NetworkRequest mOldNetworkRequest; - @NonNull public final NetworkRequest mNewNetworkRequest; + @Nullable public final NetworkRequest mOldNetworkRequest; + @Nullable public final NetworkRequest mNewNetworkRequest; @Nullable public final NetworkAgentInfo mOldNetwork; @Nullable public final NetworkAgentInfo mNewNetwork; RequestReassignment(@NonNull final NetworkRequestInfo networkRequestInfo, - @NonNull final NetworkRequest oldNetworkRequest, - @NonNull final NetworkRequest newNetworkRequest, + @Nullable final NetworkRequest oldNetworkRequest, + @Nullable final NetworkRequest newNetworkRequest, @Nullable final NetworkAgentInfo oldNetwork, @Nullable final NetworkAgentInfo newNetwork) { mNetworkRequestInfo = networkRequestInfo; @@ -7298,14 +7306,14 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void updateSatisfiersForRematchRequest(@NonNull final NetworkRequestInfo nri, - @NonNull final NetworkRequest previousRequest, - @NonNull final NetworkRequest newRequest, + @Nullable final NetworkRequest previousRequest, + @Nullable final NetworkRequest newRequest, @Nullable final NetworkAgentInfo previousSatisfier, @Nullable final NetworkAgentInfo newSatisfier, final long now) { if (null != newSatisfier && mNoServiceNetwork != newSatisfier) { if (VDBG) log("rematch for " + newSatisfier.toShortString()); - if (null != previousSatisfier && mNoServiceNetwork != previousSatisfier) { + if (null != previousRequest && null != previousSatisfier) { if (VDBG || DDBG) { log(" accepting network in place of " + previousSatisfier.toShortString()); } @@ -7328,7 +7336,7 @@ public class ConnectivityService extends IConnectivityManager.Stub Log.wtf(TAG, "BUG: " + newSatisfier.toShortString() + " already has " + newRequest); } - } else if (null != previousSatisfier) { + } else if (null != previousRequest && null != previousSatisfier) { if (DBG) { log("Network " + previousSatisfier.toShortString() + " stopped satisfying" + " request " + previousRequest.requestId); -- cgit v1.2.3-59-g8ed1b