From ede14bde886d1673c21db0b981cd9838da9e0ef1 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 18 Mar 2025 19:53:35 +0900 Subject: [PATCH] Make iterating over mNetworkAgentInfos safer. This CL provides convenience methods to iterate over all networks. - forEachNetworkAgentInfo takes a consumer and runs it on all networks in mNetworkAgentInfos. The consumer is allowed to remove any number of networks. This is needed because some loops over mNetworkAgentInfos result in disconnecting networks, which removes them from the set. - anyNetworkAgentInfo takes a predicate and returns true iff it returns true on any element in mNetworkAgentInfos. The predicate is not allowed to modify the set in any way. Bug: 288149251 Test: simple refactor, covered by existing unit tests Change-Id: I5d8b38503895d33abc2d96daaa1c96372befce64 --- .../android/server/ConnectivityService.java | 168 ++++++++++-------- 1 file changed, 95 insertions(+), 73 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index dc4a35b553..9c70e7c4e6 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -429,6 +429,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; import java.util.function.Consumer; +import java.util.function.Predicate; /** * @hide @@ -5323,12 +5324,12 @@ public class ConnectivityService extends IConnectivityManager.Stub { private void handlePrivateDnsSettingsChanged() { final PrivateDnsConfig cfg = mDnsManager.getPrivateDnsConfig(); - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { handlePerNetworkPrivateDnsConfig(nai, cfg); if (networkRequiresPrivateDnsValidation(nai)) { handleUpdateLinkProperties(nai, new LinkProperties(nai.linkProperties)); } - } + }); } private void handlePerNetworkPrivateDnsConfig(NetworkAgentInfo nai, PrivateDnsConfig cfg) { @@ -5651,16 +5652,16 @@ public class ConnectivityService extends IConnectivityManager.Stub { private void maybeDisableForwardRulesForDisconnectingNai( @NonNull final NetworkAgentInfo disconnecting, final boolean sendCallbacks) { // Step 1 : maybe this network was the upstream for one or more local networks. - for (final NetworkAgentInfo local : mNetworkAgentInfos) { - if (!local.isLocalNetwork()) continue; + forEachNetworkAgentInfo(local -> { + if (!local.isLocalNetwork()) return; // return@forEach final NetworkRequest selector = local.localNetworkConfig.getUpstreamSelector(); - if (null == selector) continue; + if (null == selector) return; // return@forEach final NetworkRequestInfo nri = mNetworkRequests.get(selector); // null == nri can happen while disconnecting a network, because destroyNetwork() is // called after removing all associated NRIs from mNetworkRequests. - if (null == nri) continue; + if (null == nri) return; // return@forEach final NetworkAgentInfo satisfier = nri.getSatisfier(); - if (disconnecting != satisfier) continue; + if (disconnecting != satisfier) return; // return@forEach removeLocalNetworkUpstream(local, disconnecting); // Set the satisfier to null immediately so that the LOCAL_NETWORK_CHANGED callback // correctly contains null as an upstream. @@ -5668,7 +5669,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { nri.setSatisfier(null, null); notifyNetworkCallbacks(local, CALLBACK_LOCAL_NETWORK_INFO_CHANGED); } - } + }); // Step 2 : maybe this is a local network that had an upstream. if (!disconnecting.isLocalNetwork()) return; @@ -5841,12 +5842,12 @@ public class ConnectivityService extends IConnectivityManager.Stub { mNetworkRequests.put(req, nri); // TODO: Consider update signal strength for other types. if (req.isListen()) { - for (final NetworkAgentInfo network : mNetworkAgentInfos) { + forEachNetworkAgentInfo(network -> { if (req.networkCapabilities.hasSignalStrength() && network.satisfiesImmutableCapabilitiesOf(req)) { updateSignalStrengthThresholds(network, "REGISTER", req); } - } + }); } else if (req.isRequest() && mNetworkRequestStateStatsMetrics != null) { mNetworkRequestStateStatsMetrics.onNetworkRequestReceived(req); } @@ -6141,13 +6142,13 @@ public class ConnectivityService extends IConnectivityManager.Stub { private void removeListenRequestFromNetworks(@NonNull final NetworkRequest req) { // listens don't have a singular affected Network. Check all networks to see // if this listen request applies and remove it. - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { nai.removeRequest(req.requestId); if (req.networkCapabilities.hasSignalStrength() && nai.satisfiesImmutableCapabilitiesOf(req)) { updateSignalStrengthThresholds(nai, "RELEASE", req); } - } + }); } /** @@ -6210,6 +6211,43 @@ public class ConnectivityService extends IConnectivityManager.Stub { } } + /** + * Perform the specified operation on all networks. + * + * This method will run |op| exactly once for each network currently registered at the + * time it is called, even if |op| adds or removes networks. + * + * @param op the operation to perform. The operation is allowed to disconnect any number of + * networks. + */ + private void forEachNetworkAgentInfo(final Consumer op) { + // Create a copy instead of iterating over the set so |op| is allowed to disconnect any + // number of networks (which removes it from mNetworkAgentInfos). The copy is cheap + // because there are at most a handful of NetworkAgents connected at any given time. + final NetworkAgentInfo[] nais = new NetworkAgentInfo[mNetworkAgentInfos.size()]; + mNetworkAgentInfos.toArray(nais); + for (NetworkAgentInfo nai : nais) { + op.accept(nai); + } + } + + /** + * Check whether the specified condition is true for any network. + * + * This method will stop evaluating as soon as the condition returns true for any network. + * The order of iteration is not contractual. + * + * @param condition the condition to verify. This method must not modify the set of networks in + * any way. + * @return whether {@code condition} returned true for any network + */ + private boolean anyNetworkAgentInfo(final Predicate condition) { + for (int i = mNetworkAgentInfos.size() - 1; i >= 0; i--) { + if (condition.test(mNetworkAgentInfos.valueAt(i))) return true; + } + return false; + } + private RequestInfoPerUidCounter getRequestCounter(NetworkRequestInfo nri) { return hasAnyPermissionOf(mContext, nri.mPid, nri.mUid, NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) @@ -6551,14 +6589,14 @@ public class ConnectivityService extends IConnectivityManager.Stub { ensureRunningOnConnectivityServiceThread(); // Agent info scores and offer scores depend on whether cells yields to bad wifi. final boolean avoidBadWifi = avoidBadWifi(); - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { nai.updateScoreForNetworkAgentUpdate(); if (avoidBadWifi) { // If the device is now avoiding bad wifi, remove notifications that might have // been put up when the device didn't. mNotifier.clearNotification(nai.network.getNetId(), NotificationType.LOST_INTERNET); } - } + }); // UpdateOfferScore will update mNetworkOffers inline, so make a copy first. final ArrayList offersToUpdate = new ArrayList<>(mNetworkOffers); for (final NetworkOfferInfo noi : offersToUpdate) { @@ -6896,19 +6934,15 @@ public class ConnectivityService extends IConnectivityManager.Stub { final Network underpinnedNetwork = ki.getUnderpinnedNetwork(); final Network network = ki.getNetwork(); - boolean networkFound = false; - boolean underpinnedNetworkFound = false; - for (NetworkAgentInfo n : mNetworkAgentInfos) { - if (n.network.equals(network)) networkFound = true; - if (n.everConnected() && n.network.equals(underpinnedNetwork)) { - underpinnedNetworkFound = true; - } - } + final boolean networkFound = + anyNetworkAgentInfo(n -> n.network.equals(network)); // If the network no longer exists, then the keepalive should have been // cleaned up already. There is no point trying to resume keepalives. if (!networkFound) return; + final boolean underpinnedNetworkFound = anyNetworkAgentInfo( + n -> n.everConnected() && n.network.equals(underpinnedNetwork)); if (underpinnedNetworkFound) { mKeepaliveTracker.handleMonitorAutomaticKeepalive(ki, underpinnedNetwork.netId); @@ -7368,12 +7402,12 @@ public class ConnectivityService extends IConnectivityManager.Stub { return new UnderlyingNetworkInfo[0]; } List infoList = new ArrayList<>(); - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { UnderlyingNetworkInfo info = createVpnInfo(nai); if (info != null) { infoList.add(info); } - } + }); return infoList.toArray(new UnderlyingNetworkInfo[infoList.size()]); } @@ -7451,11 +7485,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { */ private void propagateUnderlyingNetworkCapabilities(Network updatedNetwork) { ensureRunningOnConnectivityServiceThread(); - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { if (updatedNetwork == null || hasUnderlyingNetwork(nai, updatedNetwork)) { updateCapabilitiesForNetwork(nai); } - } + }); } private boolean isUidBlockedByVpn(int uid, List blockedUidRanges) { @@ -7503,11 +7537,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { mPermissionMonitor.updateVpnLockdownUidRanges(requireVpn, ranges); } - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { final boolean curMetered = nai.networkCapabilities.isMetered(); maybeNotifyNetworkBlocked(nai, curMetered, curMetered, mVpnBlockedUidRanges, newVpnBlockedUidRanges); - } + }); mVpnBlockedUidRanges = newVpnBlockedUidRanges; } @@ -9071,6 +9105,9 @@ public class ConnectivityService extends IConnectivityManager.Stub { // Tracks all NetworkAgents that are currently registered. // NOTE: Only should be accessed on ConnectivityServiceThread, except dump(). + // Code iterating over this set is recommended to use forAllNetworkAgentInfos(), which allows + // code within the loop to disconnect networks during iteration without causing null pointer or + // OOB exceptions. private final ArraySet mNetworkAgentInfos = new ArraySet<>(); // UID ranges for users that are currently blocked by VPNs. @@ -10445,7 +10482,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { // A NetworkAgent's allowedUids may need to be updated if the app has lost // carrier config - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { if (nai.networkCapabilities.getAllowedUidsNoCopy().contains(uid) && getSubscriptionIdFromNetworkCaps(nai.networkCapabilities) == subId) { final NetworkCapabilities nc = new NetworkCapabilities(nai.networkCapabilities); @@ -10457,7 +10494,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { mCarrierPrivilegeAuthenticator); updateCapabilities(nai.getScore(), nai, nc); } - } + }); } /** @@ -11374,7 +11411,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { throw new IllegalStateException("No user is available"); } - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { ArraySet allowedUidRanges = new ArraySet<>(); for (final UserHandle user : users) { final ArraySet restrictedUidRanges = @@ -11386,7 +11423,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { final UidRangeParcel[] rangesParcel = toUidRangeStableParcels(allowedUidRanges); configs.add(new NativeUidRangeConfig( nai.network.netId, rangesParcel, 0 /* subPriority */)); - } + }); // The netd API replaces the previous configs with the current configs. // Thus, for network disconnection or preference removal, no need to @@ -11608,9 +11645,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { // Gather the list of all relevant agents. final ArrayList nais = new ArrayList<>(); - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { - nais.add(nai); - } + forEachNetworkAgentInfo(nai -> nais.add(nai)); for (final NetworkRequestInfo nri : networkRequests) { // Non-multilayer listen requests can be ignored. @@ -11716,14 +11751,14 @@ public class ConnectivityService extends IConnectivityManager.Stub { // Don't send CALLBACK_LOCAL_NETWORK_INFO_CHANGED yet though : they should be sent after // onAvailable so clients know what network the change is about. Store such changes in // an array that's only allocated if necessary (because it's almost never necessary). - ArrayList localInfoChangedAgents = null; - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { - if (!nai.isLocalNetwork()) continue; + final ArrayList localInfoChangedAgents = new ArrayList<>(); + forEachNetworkAgentInfo(nai -> { + if (!nai.isLocalNetwork()) return; // return@forEach final NetworkRequest nr = nai.localNetworkConfig.getUpstreamSelector(); - if (null == nr) continue; // No upstream for this local network + if (null == nr) return; // return@forEach, no upstream for this local network final NetworkRequestInfo nri = mNetworkRequests.get(nr); final NetworkReassignment.RequestReassignment change = changes.getReassignment(nri); - if (null == change) continue; // No change in upstreams for this network + if (null == change) return; // return@forEach, no change in upstreams for this network final String fromIface = nai.linkProperties.getInterfaceName(); if (!hasSameInterfaceName(change.mOldNetwork, change.mNewNetwork) || change.mOldNetwork.isDestroyed()) { @@ -11751,9 +11786,8 @@ public class ConnectivityService extends IConnectivityManager.Stub { loge("Can't update forwarding rules", e); } } - if (null == localInfoChangedAgents) localInfoChangedAgents = new ArrayList<>(); localInfoChangedAgents.add(nai); - } + }); // Notify requested networks are available after the default net is switched, but // before LegacyTypeTracker sends legacy broadcasts @@ -11804,17 +11838,14 @@ public class ConnectivityService extends IConnectivityManager.Stub { } // Send LOCAL_NETWORK_INFO_CHANGED callbacks now that onAvailable and onLost have been sent. - if (null != localInfoChangedAgents) { - for (final NetworkAgentInfo nai : localInfoChangedAgents) { - notifyNetworkCallbacks(nai, - CALLBACK_LOCAL_NETWORK_INFO_CHANGED); - } + for (final NetworkAgentInfo nai : localInfoChangedAgents) { + notifyNetworkCallbacks(nai, CALLBACK_LOCAL_NETWORK_INFO_CHANGED); } updateLegacyTypeTrackerAndVpnLockdownForRematch(changes, nais); // Tear down all unneeded networks. - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { if (unneeded(nai, UnneededFor.TEARDOWN)) { if (nai.getInactivityExpiry() > 0) { // This network has active linger timers and no requests, but is not @@ -11832,7 +11863,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { teardownUnneededNetwork(nai); } } - } + }); } /** @@ -12345,7 +12376,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { * @param blockedReasons The reasons for why an uid is blocked. */ private void maybeNotifyNetworkBlockedForNewState(int uid, @BlockedReason int blockedReasons) { - for (final NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { final boolean metered = nai.networkCapabilities.isMetered(); final boolean vpnBlocked = isUidBlockedByVpn(uid, mVpnBlockedUidRanges); @@ -12353,9 +12384,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { uid, mUidBlockedReasons.get(uid, BLOCKED_REASON_NONE), metered, vpnBlocked); final int newBlockedState = getBlockedState(uid, blockedReasons, metered, vpnBlocked); - if (oldBlockedState == newBlockedState) { - continue; - } + if (oldBlockedState == newBlockedState) return; // return@forEach for (int i = 0; i < nai.numNetworkRequests(); i++) { NetworkRequest nr = nai.requestAt(i); NetworkRequestInfo nri = mNetworkRequests.get(nr); @@ -12364,7 +12393,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { newBlockedState); } } - } + }); } @VisibleForTesting @@ -12453,11 +12482,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { activeNetIds.add(nri.getSatisfier().network().netId); } } - for (NetworkAgentInfo nai : mNetworkAgentInfos) { + forEachNetworkAgentInfo(nai -> { if (activeNetIds.contains(nai.network().netId) || nai.isVPN()) { defaultNetworks.add(nai.network); } - } + }); return defaultNetworks; } @@ -13348,15 +13377,10 @@ public class ConnectivityService extends IConnectivityManager.Stub { } private boolean ownsVpnRunningOverNetwork(int uid, Network network) { - for (NetworkAgentInfo virtual : mNetworkAgentInfos) { - if (virtual.propagateUnderlyingCapabilities() - && virtual.networkCapabilities.getOwnerUid() == uid - && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, network)) { - return true; - } - } - - return false; + return anyNetworkAgentInfo(virtual -> + virtual.propagateUnderlyingCapabilities() + && virtual.networkCapabilities.getOwnerUid() == uid + && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, network)); } @CheckResult @@ -13527,18 +13551,16 @@ public class ConnectivityService extends IConnectivityManager.Stub { @Override public void onInterfaceLinkStateChanged(@NonNull String iface, boolean up) { mHandler.post(() -> { - for (NetworkAgentInfo nai : mNetworkAgentInfos) { - nai.clatd.handleInterfaceLinkStateChanged(iface, up); - } + forEachNetworkAgentInfo(nai -> + nai.clatd.handleInterfaceLinkStateChanged(iface, up)); }); } @Override public void onInterfaceRemoved(@NonNull String iface) { mHandler.post(() -> { - for (NetworkAgentInfo nai : mNetworkAgentInfos) { - nai.clatd.handleInterfaceRemoved(iface); - } + forEachNetworkAgentInfo(nai -> + nai.clatd.handleInterfaceRemoved(iface)); }); } } @@ -14319,7 +14341,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { final long oldIngressRateLimit = mIngressRateLimit; mIngressRateLimit = ConnectivitySettingsManager.getIngressRateLimitInBytesPerSecond( mContext); - for (final NetworkAgentInfo networkAgent : mNetworkAgentInfos) { + forEachNetworkAgentInfo(networkAgent -> { if (canNetworkBeRateLimited(networkAgent)) { // If rate limit has previously been enabled, remove the old limit first. if (oldIngressRateLimit >= 0) { @@ -14330,7 +14352,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { mIngressRateLimit); } } - } + }); } private boolean canNetworkBeRateLimited(@NonNull final NetworkAgentInfo networkAgent) {