From 80685f42c35cfce3692be6ec406d8e259b8afb9e Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 30 Oct 2023 18:31:27 +0900 Subject: [PATCH] Destroyed networks can't hope to beat champions In particular, this fixes a bug where a non-validated destroyed network can't get disconnected after it's replaced by another non-validated network because CS thinks it should keep it in case it validates and beats the replacement network. Test: new test for this : CSDestroyedNetworkTests Change-Id: I5d7e413624d6fca28b06484e5369cd17e4a599a4 --- .../android/server/ConnectivityService.java | 17 +++++- .../CSDestroyedNetworkTests.kt | 56 +++++++++++++++++++ .../base/CSAgentWrapper.kt | 2 +- 3 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 tests/unit/java/com/android/server/connectivityservice/CSDestroyedNetworkTests.kt diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index c4cb4c7bcc..517856e832 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -5273,7 +5273,14 @@ public class ConnectivityService extends IConnectivityManager.Stub private boolean isNetworkPotentialSatisfier( @NonNull final NetworkAgentInfo candidate, @NonNull final NetworkRequestInfo nri) { - // listen requests won't keep up a network satisfying it. If this is not a multilayer + // While destroyed network sometimes satisfy requests (including occasionally newly + // satisfying requests), *potential* satisfiers are networks that might beat a current + // champion if they validate. As such, a destroyed network is never a potential satisfier, + // because it's never a good idea to keep a destroyed network in case it validates. + // For example, declaring it a potential satisfier would keep an unvalidated destroyed + // candidate after it's been replaced by another unvalidated network. + if (candidate.isDestroyed()) return false; + // Listen requests won't keep up a network satisfying it. If this is not a multilayer // request, return immediately. For multilayer requests, check to see if any of the // multilayer requests may have a potential satisfier. if (!nri.isMultilayerRequest() && (nri.mRequests.get(0).isListen() @@ -5291,8 +5298,12 @@ public class ConnectivityService extends IConnectivityManager.Stub if (req.isListen() || req.isListenForBest()) { continue; } - // If this Network is already the best Network for a request, or if - // there is hope for it to become one if it validated, then it is needed. + // If there is hope for this network might validate and subsequently become the best + // network for that request, then it is needed. Note that this network can't already + // be the best for this request, or it would be the current satisfier, and therefore + // there would be no need to call this method to find out if it is a *potential* + // satisfier ("unneeded", the only caller, only calls this if this network currently + // satisfies no request). if (candidate.satisfies(req)) { // As soon as a network is found that satisfies a request, return. Specifically for // multilayer requests, returning as soon as a NetworkAgentInfo satisfies a request diff --git a/tests/unit/java/com/android/server/connectivityservice/CSDestroyedNetworkTests.kt b/tests/unit/java/com/android/server/connectivityservice/CSDestroyedNetworkTests.kt new file mode 100644 index 0000000000..d40c0353dd --- /dev/null +++ b/tests/unit/java/com/android/server/connectivityservice/CSDestroyedNetworkTests.kt @@ -0,0 +1,56 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server + +import android.net.NetworkCapabilities +import android.net.NetworkCapabilities.TRANSPORT_WIFI +import android.net.NetworkRequest +import com.android.testutils.RecorderCallback.CallbackEntry.Lost +import com.android.testutils.TestableNetworkCallback +import org.junit.Test + +private const val LONG_TIMEOUT_MS = 5_000 + +class CSDestroyedNetworkTests : CSTest() { + @Test + fun testDestroyNetworkNotKeptWhenUnvalidated() { + val nc = NetworkCapabilities.Builder() + .addTransportType(TRANSPORT_WIFI) + .build() + + val nr = NetworkRequest.Builder() + .clearCapabilities() + .addTransportType(TRANSPORT_WIFI) + .build() + val cbRequest = TestableNetworkCallback() + val cbCallback = TestableNetworkCallback() + cm.requestNetwork(nr, cbRequest) + cm.registerNetworkCallback(nr, cbCallback) + + val firstAgent = Agent(nc = nc) + firstAgent.connect() + cbCallback.expectAvailableCallbacks(firstAgent.network, validated = false) + + firstAgent.unregisterAfterReplacement(LONG_TIMEOUT_MS) + + val secondAgent = Agent(nc = nc) + secondAgent.connect() + cbCallback.expectAvailableCallbacks(secondAgent.network, validated = false) + + cbCallback.expect(timeoutMs = 500) { it.network == firstAgent.network } + } +} diff --git a/tests/unit/java/com/android/server/connectivityservice/base/CSAgentWrapper.kt b/tests/unit/java/com/android/server/connectivityservice/base/CSAgentWrapper.kt index 094ded3ae0..8860895609 100644 --- a/tests/unit/java/com/android/server/connectivityservice/base/CSAgentWrapper.kt +++ b/tests/unit/java/com/android/server/connectivityservice/base/CSAgentWrapper.kt @@ -34,7 +34,6 @@ import android.net.NetworkScore import android.net.NetworkTestResultParcelable import android.net.networkstack.NetworkStackClientBase import android.os.HandlerThread -import com.android.modules.utils.build.SdkLevel import com.android.testutils.RecorderCallback.CallbackEntry.Available import com.android.testutils.RecorderCallback.CallbackEntry.Lost import com.android.testutils.TestableNetworkCallback @@ -168,5 +167,6 @@ class CSAgentWrapper( cb.eventuallyExpect { it.network == agent.network } } + fun unregisterAfterReplacement(timeoutMs: Int) = agent.unregisterAfterReplacement(timeoutMs) fun sendLocalNetworkConfig(lnc: LocalNetworkConfig) = agent.sendLocalNetworkConfig(lnc) }