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

Always disconnect agents immediately.

Browse Source 
Currently, various codepaths in ConnectivityService disconnect
networks using NetworkAgentInfo#disconnect. This posts a message
to the NetworkAgent to disconnect and also posts a message to
ConnectivityService to call disconnectAndDestroyNetwork, which
performs all cleanup in ConnectivityService. These two messages
race and the order is non-deterministic.

Instead, always disconnect using disconnectAndDestroyNetwork,
and have disconnectAndDestroyNetwork post the message to the
agent to disconnect.

This fixes a bug where if wifi uses unregisterAfterReplacement
twice in quick succession, when the third agent connects it
doesn't work because the interface is still being used by the
second network.

Also remove the import of IllegalArgumentException to keep the
linter happy (java.lang.* never needs to be imported).

Bug: 286649301
Test: covered by existing tests
Change-Id: Ie01f5589d6839ac6db25f0ba98fc929fbb5b0a96
This commit is contained in:
Lorenzo Colitti
2023-06-13 17:47:59 +09:00
parent 977fa60f5d
commit 4bc9fa6b8c
3 changed files with 23 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
service/src/com/android/server/ConnectivityService.java
View File

@@ -318,7 +318,6 @@ import java.io.IOException;
import java.io.InterruptedIOException;
import java.io.PrintWriter;
import java.io.Writer;
import java.lang.IllegalArgumentException;
import java.net.Inet4Address;
import java.net.InetAddress;
import java.net.InetSocketAddress;
@@ -4010,7 +4009,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
// the destroyed flag is only just above the "current satisfier wins"
// tie-breaker. But technically anything that affects scoring should rematch.
rematchAllNetworksAndRequests();
mHandler.postDelayed(() -> nai.disconnect(), timeoutMs);
mHandler.postDelayed(() -> disconnectAndDestroyNetwork(nai), timeoutMs);
break;
}
}
@@ -4609,6 +4608,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
if (DBG) {
log(nai.toShortString() + " disconnected, was satisfying " + nai.numNetworkRequests());
}
nai.disconnect();
// Clear all notifications of this network.
mNotifier.clearNotification(nai.network.getNetId());
// A network agent has disconnected.
@@ -5893,7 +5895,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork((Network) msg.obj);
if (nai == null) break;
nai.onPreventAutomaticReconnect();
nai.disconnect();
disconnectAndDestroyNetwork(nai);
break;
case EVENT_SET_VPN_NETWORK_PREFERENCE:
handleSetVpnNetworkPreference((VpnNetworkPreferenceInfo) msg.obj);
@@ -9038,7 +9040,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
break;
}
}
nai.disconnect();
disconnectAndDestroyNetwork(nai);
}
private void handleLingerComplete(NetworkAgentInfo oldNetwork) {
@@ -9580,7 +9582,10 @@ public class ConnectivityService extends IConnectivityManager.Stub
updateLegacyTypeTrackerAndVpnLockdownForRematch(changes, nais);
// Tear down all unneeded networks.
for (NetworkAgentInfo nai : mNetworkAgentInfos) {
// Iterate in reverse order because teardownUnneededNetwork removes the nai from
// mNetworkAgentInfos.
for (int i = mNetworkAgentInfos.size() - 1; i >= 0; i--) {
final NetworkAgentInfo nai = mNetworkAgentInfos.valueAt(i);
if (unneeded(nai, UnneededFor.TEARDOWN)) {
if (nai.getInactivityExpiry() > 0) {
// This network has active linger timers and no requests, but is not
@@ -9963,7 +9968,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
// This has to happen after matching the requests, because callbacks are just requests.
notifyNetworkCallbacks(networkAgent, ConnectivityManager.CALLBACK_PRECHECK);
} else if (state == NetworkInfo.State.DISCONNECTED) {
networkAgent.disconnect();
if (networkAgent.isVPN()) {
updateVpnUids(networkAgent, networkAgent.networkCapabilities, null);
}

13
tests/cts/net/src/android/net/cts/NetworkAgentTest.kt
View File

@@ -1247,15 +1247,15 @@ class NetworkAgentTest {
// Connect a third network. Because network1 is awaiting replacement, network3 is preferred
// as soon as it validates (until then, it is outscored by network1).
// The fact that the first events seen by matchAllCallback is the connection of network3
// The fact that the first event seen by matchAllCallback is the connection of network3
// implicitly ensures that no callbacks are sent since network1 was lost.
val (agent3, network3) = connectNetwork()
matchAllCallback.expectAvailableThenValidatedCallbacks(network3)
testCallback.expectAvailableDoubleValidatedCallbacks(network3)
// As soon as the replacement arrives, network1 is disconnected.
// Check that this happens before the replacement timeout (5 seconds) fires.
matchAllCallback.expectAvailableCallbacks(network3, validated = false)
matchAllCallback.expect<Lost>(network1, 2_000 /* timeoutMs */)
matchAllCallback.expectCaps(network3) { it.hasCapability(NET_CAPABILITY_VALIDATED) }
testCallback.expectAvailableDoubleValidatedCallbacks(network3)
agent1.expectCallback<OnNetworkUnwanted>()
// Test lingering:
@@ -1301,7 +1301,7 @@ class NetworkAgentTest {
val callback = TestableNetworkCallback()
requestNetwork(makeTestNetworkRequest(specifier = specifier6), callback)
val agent6 = createNetworkAgent(specifier = specifier6)
val network6 = agent6.register()
agent6.register()
if (SdkLevel.isAtLeastU()) {
agent6.expectCallback<OnNetworkCreated>()
} else {
@@ -1368,8 +1368,9 @@ class NetworkAgentTest {
val (newWifiAgent, newWifiNetwork) = connectNetwork(TRANSPORT_WIFI)
testCallback.expectAvailableCallbacks(newWifiNetwork, validated = true)
matchAllCallback.expectAvailableThenValidatedCallbacks(newWifiNetwork)
matchAllCallback.expectAvailableCallbacks(newWifiNetwork, validated = false)
matchAllCallback.expect<Lost>(wifiNetwork)
matchAllCallback.expectCaps(newWifiNetwork) { it.hasCapability(NET_CAPABILITY_VALIDATED) }
wifiAgent.expectCallback<OnNetworkUnwanted>()
}

10
tests/unit/java/com/android/server/ConnectivityServiceTest.java
View File

@@ -2946,22 +2946,24 @@ public class ConnectivityServiceTest {
if (expectLingering) {
generalCb.expectLosing(net1);
}
generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED));
defaultCb.expectAvailableDoubleValidatedCallbacks(net2);
// Make sure cell 1 is unwanted immediately if the radio can't time share, but only
// after some delay if it can.
if (expectLingering) {
generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED));
defaultCb.expectAvailableDoubleValidatedCallbacks(net2);
net1.assertNotDisconnected(TEST_CALLBACK_TIMEOUT_MS); // always incurs the timeout
generalCb.assertNoCallback();
// assertNotDisconnected waited for TEST_CALLBACK_TIMEOUT_MS, so waiting for the
// linger period gives TEST_CALLBACK_TIMEOUT_MS time for the event to process.
net1.expectDisconnected(UNREASONABLY_LONG_ALARM_WAIT_MS);
generalCb.expect(LOST, net1);
} else {
net1.expectDisconnected(TEST_CALLBACK_TIMEOUT_MS);
generalCb.expect(LOST, net1);
generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED));
defaultCb.expectAvailableDoubleValidatedCallbacks(net2);
}
net1.disconnect();
generalCb.expect(LOST, net1);
// Remove primary from net 2
net2.setScore(new NetworkScore.Builder().build());