LineageOS/android_packages_modules_Connectivity
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Implement ConnectivityService TODO and fix many race conditions

Browse Source 
This patch implements an outstanding TODO in ConnectivityService to add
synchronization over the map of network request ids to network agent
info objects.

This structure is accessed from multiple threads:
 - Binder thread on public aidl methods, most notably via
   getDefaultNetwork().
 - Internal handler.

This leads to many race conditions that can crash the system server and
reboot the phone if getDefaultNetwork() is called on a Binder thread to
service a public ConnectivityManager api while the default network state
is being updated on the internal handler after losing the default
network.

Bug: 65911184
Test: runtest frameworks-net
Change-Id: I86c830ebd559e31d4576a7606705a056afb064ac
This commit is contained in:
Hugo Benichi
2017-09-20 11:20:14 +09:00
parent e5090f2547
commit 53e6fd54e6
1 changed files with 32 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
services/core/java/com/android/server/ConnectivityService.java
View File

@@ -2217,7 +2217,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
// A network factory has connected. Send it all current NetworkRequests. // A network factory has connected. Send it all current NetworkRequests.
for (NetworkRequestInfo nri : mNetworkRequests.values()) { for (NetworkRequestInfo nri : mNetworkRequests.values()) {
if (nri.request.isListen()) continue; if (nri.request.isListen()) continue;
NetworkAgentInfo nai = mNetworkForRequestId.get(nri.request.requestId); NetworkAgentInfo nai = getNetworkForRequest(nri.request.requestId);
ac.sendMessage(android.net.NetworkFactory.CMD_REQUEST_NETWORK, ac.sendMessage(android.net.NetworkFactory.CMD_REQUEST_NETWORK,
(nai != null ? nai.getCurrentScore() : 0), 0, nri.request); (nai != null ? nai.getCurrentScore() : 0), 0, nri.request);
} }
@@ -2294,9 +2294,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
// Remove all previously satisfied requests. // Remove all previously satisfied requests.
for (int i = 0; i < nai.numNetworkRequests(); i++) { for (int i = 0; i < nai.numNetworkRequests(); i++) {
NetworkRequest request = nai.requestAt(i); NetworkRequest request = nai.requestAt(i);
NetworkAgentInfo currentNetwork = mNetworkForRequestId.get(request.requestId); NetworkAgentInfo currentNetwork = getNetworkForRequest(request.requestId);
if (currentNetwork != null && currentNetwork.network.netId == nai.network.netId) { if (currentNetwork != null && currentNetwork.network.netId == nai.network.netId) {
mNetworkForRequestId.remove(request.requestId); clearNetworkForRequest(request.requestId);
sendUpdatedScoreToFactories(request, 0); sendUpdatedScoreToFactories(request, 0);
} }
} }
@@ -2372,7 +2372,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
} }
} }
rematchAllNetworksAndRequests(null, 0); rematchAllNetworksAndRequests(null, 0);
if (nri.request.isRequest() && mNetworkForRequestId.get(nri.request.requestId) == null) { if (nri.request.isRequest() && getNetworkForRequest(nri.request.requestId) == null) {
sendUpdatedScoreToFactories(nri.request, 0); sendUpdatedScoreToFactories(nri.request, 0);
} }
} }
@@ -2427,7 +2427,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
// 2. Unvalidated WiFi will not be reaped when validated cellular // 2. Unvalidated WiFi will not be reaped when validated cellular
// is currently satisfying the request. This is desirable when // is currently satisfying the request. This is desirable when
// WiFi ends up validating and out scoring cellular. // WiFi ends up validating and out scoring cellular.
mNetworkForRequestId.get(nri.request.requestId).getCurrentScore() < getNetworkForRequest(nri.request.requestId).getCurrentScore() <
nai.getCurrentScoreAsValidated())) { nai.getCurrentScoreAsValidated())) {
return false; return false;
} }
@@ -2454,7 +2454,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
if (mNetworkRequests.get(nri.request) == null) { if (mNetworkRequests.get(nri.request) == null) {
return; return;
} }
if (mNetworkForRequestId.get(nri.request.requestId) != null) { if (getNetworkForRequest(nri.request.requestId) != null) {
return; return;
} }
if (VDBG || (DBG && nri.request.isRequest())) { if (VDBG || (DBG && nri.request.isRequest())) {
@@ -2494,7 +2494,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
mNetworkRequestInfoLogs.log("RELEASE " + nri); mNetworkRequestInfoLogs.log("RELEASE " + nri);
if (nri.request.isRequest()) { if (nri.request.isRequest()) {
boolean wasKept = false; boolean wasKept = false;
NetworkAgentInfo nai = mNetworkForRequestId.get(nri.request.requestId); NetworkAgentInfo nai = getNetworkForRequest(nri.request.requestId);
if (nai != null) { if (nai != null) {
boolean wasBackgroundNetwork = nai.isBackgroundNetwork(); boolean wasBackgroundNetwork = nai.isBackgroundNetwork();
nai.removeRequest(nri.request.requestId); nai.removeRequest(nri.request.requestId);
@@ -2511,7 +2511,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
} else { } else {
wasKept = true; wasKept = true;
} }
mNetworkForRequestId.remove(nri.request.requestId); clearNetworkForRequest(nri.request.requestId);
if (!wasBackgroundNetwork && nai.isBackgroundNetwork()) { if (!wasBackgroundNetwork && nai.isBackgroundNetwork()) {
// Went from foreground to background. // Went from foreground to background.
updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities);
@@ -4288,7 +4288,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
* and the are the highest scored network available. * and the are the highest scored network available.
* the are keyed off the Requests requestId. * the are keyed off the Requests requestId.
*/ */
// TODO: Yikes, this is accessed on multiple threads: add synchronization. // NOTE: Accessed on multiple threads, must be synchronized on itself.
@GuardedBy("mNetworkForRequestId")
private final SparseArray<NetworkAgentInfo> mNetworkForRequestId = private final SparseArray<NetworkAgentInfo> mNetworkForRequestId =
new SparseArray<NetworkAgentInfo>(); new SparseArray<NetworkAgentInfo>();
@@ -4318,8 +4319,26 @@ public class ConnectivityService extends IConnectivityManager.Stub
// priority networks like Wi-Fi are active. // priority networks like Wi-Fi are active.
private final NetworkRequest mDefaultMobileDataRequest; private final NetworkRequest mDefaultMobileDataRequest;
private NetworkAgentInfo getNetworkForRequest(int requestId) {
synchronized (mNetworkForRequestId) {
return mNetworkForRequestId.get(requestId);
}
}
private void clearNetworkForRequest(int requestId) {
synchronized (mNetworkForRequestId) {
mNetworkForRequestId.remove(requestId);
}
}
private void setNetworkForRequest(int requestId, NetworkAgentInfo nai) {
synchronized (mNetworkForRequestId) {
mNetworkForRequestId.put(requestId, nai);
}
}
private NetworkAgentInfo getDefaultNetwork() { private NetworkAgentInfo getDefaultNetwork() {
return mNetworkForRequestId.get(mDefaultRequest.requestId); return getNetworkForRequest(mDefaultRequest.requestId);
} }
private boolean isDefaultNetwork(NetworkAgentInfo nai) { private boolean isDefaultNetwork(NetworkAgentInfo nai) {
@@ -4873,7 +4892,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
// requests or not, and doesn't affect the network's score. // requests or not, and doesn't affect the network's score.
if (nri.request.isListen()) continue; if (nri.request.isListen()) continue;
final NetworkAgentInfo currentNetwork = mNetworkForRequestId.get(nri.request.requestId); final NetworkAgentInfo currentNetwork = getNetworkForRequest(nri.request.requestId);
final boolean satisfies = newNetwork.satisfies(nri.request); final boolean satisfies = newNetwork.satisfies(nri.request);
if (newNetwork == currentNetwork && satisfies) { if (newNetwork == currentNetwork && satisfies) {
if (VDBG) { if (VDBG) {
@@ -4905,7 +4924,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
if (VDBG) log(" accepting network in place of null"); if (VDBG) log(" accepting network in place of null");
} }
newNetwork.unlingerRequest(nri.request); newNetwork.unlingerRequest(nri.request);
mNetworkForRequestId.put(nri.request.requestId, newNetwork); setNetworkForRequest(nri.request.requestId, newNetwork);
if (!newNetwork.addRequest(nri.request)) { if (!newNetwork.addRequest(nri.request)) {
Slog.wtf(TAG, "BUG: " + newNetwork.name() + " already has " + nri.request); Slog.wtf(TAG, "BUG: " + newNetwork.name() + " already has " + nri.request);
} }
@@ -4939,7 +4958,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
} }
newNetwork.removeRequest(nri.request.requestId); newNetwork.removeRequest(nri.request.requestId);
if (currentNetwork == newNetwork) { if (currentNetwork == newNetwork) {
mNetworkForRequestId.remove(nri.request.requestId); clearNetworkForRequest(nri.request.requestId);
sendUpdatedScoreToFactories(nri.request, 0); sendUpdatedScoreToFactories(nri.request, 0);
} else { } else {
Slog.wtf(TAG, "BUG: Removing request " + nri.request.requestId + " from " + Slog.wtf(TAG, "BUG: Removing request " + nri.request.requestId + " from " +