Implement ConnectivityService TODO and fix many race conditions
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
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 31fc874..06b8e1c 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -2217,7 +2217,7 @@
// A network factory has connected. Send it all current NetworkRequests.
for (NetworkRequestInfo nri : mNetworkRequests.values()) {
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,
(nai != null ? nai.getCurrentScore() : 0), 0, nri.request);
}
@@ -2294,9 +2294,9 @@
// Remove all previously satisfied requests.
for (int i = 0; i < nai.numNetworkRequests(); 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) {
- mNetworkForRequestId.remove(request.requestId);
+ clearNetworkForRequest(request.requestId);
sendUpdatedScoreToFactories(request, 0);
}
}
@@ -2372,7 +2372,7 @@
}
}
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);
}
}
@@ -2427,7 +2427,7 @@
// 2. Unvalidated WiFi will not be reaped when validated cellular
// is currently satisfying the request. This is desirable when
// WiFi ends up validating and out scoring cellular.
- mNetworkForRequestId.get(nri.request.requestId).getCurrentScore() <
+ getNetworkForRequest(nri.request.requestId).getCurrentScore() <
nai.getCurrentScoreAsValidated())) {
return false;
}
@@ -2454,7 +2454,7 @@
if (mNetworkRequests.get(nri.request) == null) {
return;
}
- if (mNetworkForRequestId.get(nri.request.requestId) != null) {
+ if (getNetworkForRequest(nri.request.requestId) != null) {
return;
}
if (VDBG || (DBG && nri.request.isRequest())) {
@@ -2494,7 +2494,7 @@
mNetworkRequestInfoLogs.log("RELEASE " + nri);
if (nri.request.isRequest()) {
boolean wasKept = false;
- NetworkAgentInfo nai = mNetworkForRequestId.get(nri.request.requestId);
+ NetworkAgentInfo nai = getNetworkForRequest(nri.request.requestId);
if (nai != null) {
boolean wasBackgroundNetwork = nai.isBackgroundNetwork();
nai.removeRequest(nri.request.requestId);
@@ -2511,7 +2511,7 @@
} else {
wasKept = true;
}
- mNetworkForRequestId.remove(nri.request.requestId);
+ clearNetworkForRequest(nri.request.requestId);
if (!wasBackgroundNetwork && nai.isBackgroundNetwork()) {
// Went from foreground to background.
updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities);
@@ -4288,7 +4288,8 @@
* and the are the highest scored network available.
* 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 =
new SparseArray<NetworkAgentInfo>();
@@ -4318,8 +4319,26 @@
// priority networks like Wi-Fi are active.
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() {
- return mNetworkForRequestId.get(mDefaultRequest.requestId);
+ return getNetworkForRequest(mDefaultRequest.requestId);
}
private boolean isDefaultNetwork(NetworkAgentInfo nai) {
@@ -4873,7 +4892,7 @@
// requests or not, and doesn't affect the network's score.
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);
if (newNetwork == currentNetwork && satisfies) {
if (VDBG) {
@@ -4905,7 +4924,7 @@
if (VDBG) log(" accepting network in place of null");
}
newNetwork.unlingerRequest(nri.request);
- mNetworkForRequestId.put(nri.request.requestId, newNetwork);
+ setNetworkForRequest(nri.request.requestId, newNetwork);
if (!newNetwork.addRequest(nri.request)) {
Slog.wtf(TAG, "BUG: " + newNetwork.name() + " already has " + nri.request);
}
@@ -4939,7 +4958,7 @@
}
newNetwork.removeRequest(nri.request.requestId);
if (currentNetwork == newNetwork) {
- mNetworkForRequestId.remove(nri.request.requestId);
+ clearNetworkForRequest(nri.request.requestId);
sendUpdatedScoreToFactories(nri.request, 0);
} else {
Slog.wtf(TAG, "BUG: Removing request " + nri.request.requestId + " from " +