Only pass the NRI for removal in NRI#binderDied
When NetworkRequestInfo#binderDied is called in ConnectivityService, only pass the NRI to handleRemoveNetworkRequest. This is to prevent a potential crash when unlinkDeathRecipient is called twice for the same NRI. Also, as a cleanup, don't iterate mRequests in the log message on binderDied. As per the bug, the chain of events leading to a potential crash are: - `Connectivity.NetworkRequestInfo#binderDied()` is called for an NRI tracking multiple `NetworkRequest` items. This can happen for a TRACK_DEFAULT request filed by a UID on a different preference than the default, which copies the request list. - This in turn triggers multiple `EVENT_RELEASE_NETWORK_REQUEST` events for the same NRI, one for reach `NetworkRequest` tracked. - When handling `EVENT_RELEASE_NETWORK_REQUEST`, each `NetworkRequest` that is passed in will then be used to look up the parent NRI that originally sent it to be released. - Therefore if an NRI was tracking three requests, it would trigger three release network events, then each request would be used to look up the same NRI again when handling said release event. - Finally, `ConnectivityService.NetworkRequestInfo#unlinkDeathRecipient` is called for the NRI in question. Using the scenario above, that means we could call `unlinkDeathRecipient` multiple times for the same NRI if it was tracking multiple network requests causing the associated crash. - If `unlinkDeathRecipient` is called more than once for the same NRI, it will cause the crash listed in this bug. - The fix is to only call handleRemoveNetworkRequest for the NRI once. This works since when removing the NRI, we iterate over all of its requests to remove them. By only calling handleRemoveNetworkRequest once, it's ensured `unlinkDeathRecipient` for this NRI as part of `Connectivity.NetworkRequestInfo#binderDied()` is only called once and not potentially multiple times. Bug: 185541983 Test: atest FrameworksNetTests Change-Id: I2a2ad4ec6d415423182a1856a898779203658f8b
This commit is contained in:
@@ -4206,13 +4206,16 @@ public class ConnectivityService extends IConnectivityManager.Stub
|
|||||||
|
|
||||||
private void handleRemoveNetworkRequest(@NonNull final NetworkRequestInfo nri) {
|
private void handleRemoveNetworkRequest(@NonNull final NetworkRequestInfo nri) {
|
||||||
ensureRunningOnConnectivityServiceThread();
|
ensureRunningOnConnectivityServiceThread();
|
||||||
nri.unlinkDeathRecipient();
|
|
||||||
for (final NetworkRequest req : nri.mRequests) {
|
for (final NetworkRequest req : nri.mRequests) {
|
||||||
mNetworkRequests.remove(req);
|
if (null == mNetworkRequests.remove(req)) {
|
||||||
|
logw("Attempted removal of untracked request " + req + " for nri " + nri);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
if (req.isListen()) {
|
if (req.isListen()) {
|
||||||
removeListenRequestFromNetworks(req);
|
removeListenRequestFromNetworks(req);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
nri.unlinkDeathRecipient();
|
||||||
if (mDefaultNetworkRequests.remove(nri)) {
|
if (mDefaultNetworkRequests.remove(nri)) {
|
||||||
// If this request was one of the defaults, then the UID rules need to be updated
|
// 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
|
// WARNING : if the app(s) for which this network request is the default are doing
|
||||||
@@ -5881,8 +5884,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
|
|||||||
@Override
|
@Override
|
||||||
public void binderDied() {
|
public void binderDied() {
|
||||||
log("ConnectivityService NetworkRequestInfo binderDied(" +
|
log("ConnectivityService NetworkRequestInfo binderDied(" +
|
||||||
mRequests + ", " + mBinder + ")");
|
"uid/pid:" + mUid + "/" + mPid + ", " + mBinder + ")");
|
||||||
releaseNetworkRequests(mRequests);
|
mHandler.post(() -> handleRemoveNetworkRequest(this));
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
@@ -6312,12 +6315,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
|
|||||||
return mNextNetworkProviderId.getAndIncrement();
|
return mNextNetworkProviderId.getAndIncrement();
|
||||||
}
|
}
|
||||||
|
|
||||||
private void releaseNetworkRequests(List<NetworkRequest> networkRequests) {
|
|
||||||
for (int i = 0; i < networkRequests.size(); i++) {
|
|
||||||
releaseNetworkRequest(networkRequests.get(i));
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public void releaseNetworkRequest(NetworkRequest networkRequest) {
|
public void releaseNetworkRequest(NetworkRequest networkRequest) {
|
||||||
ensureNetworkRequestHasType(networkRequest);
|
ensureNetworkRequestHasType(networkRequest);
|
||||||
|
|||||||
Reference in New Issue
Block a user