From f15fc752c5f397c6a46d63fea3b42bf58cd9eb5a Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Wed, 20 Sep 2017 11:20:14 +0900 Subject: [PATCH] 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 Merged-In: I86c830ebd559e31d4576a7606705a056afb064ac Merged-In: I2011e23c9f894c079ab66cd7cc5c14af572a956d Merged-In: Ic70901a6aa22a03e97f494e793920ab07a0fd612 Merged-In: I4a7658e1fa6946063ab86a251269413903841ee8 Merged-In: Ia59d45f4e95a536d7982f61ac9c9a1bfc5e8ebb8 (cherry picked from commit cd95278e55bd0e4935f42214d964f8c6aa52b4ea) Change-Id: I2e26bef9eddd342f51c02b991632c7ea04fe7e66 --- .../android/server/ConnectivityService.java | 45 +++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index adf536bbf4..12cdc74b37 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2201,7 +2201,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // 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); } @@ -2278,9 +2278,9 @@ public class ConnectivityService extends IConnectivityManager.Stub // 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); } } @@ -2356,7 +2356,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } 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); } } @@ -2411,7 +2411,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // 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; } @@ -2438,7 +2438,7 @@ public class ConnectivityService extends IConnectivityManager.Stub 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())) { @@ -2478,7 +2478,7 @@ public class ConnectivityService extends IConnectivityManager.Stub 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); @@ -2495,7 +2495,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } 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); @@ -4286,7 +4286,8 @@ public class ConnectivityService extends IConnectivityManager.Stub * 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 mNetworkForRequestId = new SparseArray(); @@ -4316,8 +4317,26 @@ public class ConnectivityService extends IConnectivityManager.Stub // 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) { @@ -4879,7 +4898,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // 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) { @@ -4911,7 +4930,7 @@ public class ConnectivityService extends IConnectivityManager.Stub 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); } @@ -4945,7 +4964,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } 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 " +