From 7207a83ca8796ced7e7fc4f7eba2433ecaa2ff0f Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Fri, 25 Jun 2021 12:03:10 -0700 Subject: Fix dangling NetworkAgent This change fixes the potential for a networkAgent to be left dangling, due to a situation where a VcnGatewayConnection shuts down, but fails to unregister it's NetworkAgent. The root cause was that the NetworkAgent was not unregistered when moving to the DisconnectedState from the RetryTimeoutState, and the new state assumed that there was no NetworkAgent, and thus failed to close it when disconnecting. Thus, this change ensures that the NetworkAgent is closed before moving to the DisconnectedState. Additionally, it adds safety-checks to onQuitting(), ensuring that if all else fails, these fields are cleaned up. Lastly, this change adds the specific gateway reference to facilitate future debugging of issues such as this where there is potential for duplicate networks, or gateway connections. Bug: 191707296 Test: atest FrameworksVcnTests Change-Id: I84cd43a0c136662f5c2d229650f1f5f889e6f144 --- services/core/java/com/android/server/vcn/Vcn.java | 6 ++++- .../android/server/vcn/VcnGatewayConnection.java | 16 +++++++++++++ .../VcnGatewayConnectionRetryTimeoutStateTest.java | 28 ++++++++++++++++++++++ .../server/vcn/VcnGatewayConnectionTest.java | 23 ++++++++++++++++++ 4 files changed, 72 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/vcn/Vcn.java b/services/core/java/com/android/server/vcn/Vcn.java index f7d61367c81e..0d91e4762504 100644 --- a/services/core/java/com/android/server/vcn/Vcn.java +++ b/services/core/java/com/android/server/vcn/Vcn.java @@ -514,7 +514,11 @@ public class Vcn extends Handler { } private String getLogPrefix() { - return "[" + LogUtils.getHashedSubscriptionGroup(mSubscriptionGroup) + "] "; + return "[" + + LogUtils.getHashedSubscriptionGroup(mSubscriptionGroup) + + "-" + + System.identityHashCode(this) + + "] "; } private void logVdbg(String msg) { diff --git a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java index 493697049f34..c6f3f73d0c61 100644 --- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java +++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java @@ -714,6 +714,18 @@ public class VcnGatewayConnection extends StateMachine { protected void onQuitting() { logDbg("Quitting VcnGatewayConnection"); + if (mNetworkAgent != null) { + logWtf("NetworkAgent was non-null in onQuitting"); + mNetworkAgent.unregister(); + mNetworkAgent = null; + } + + if (mIkeSession != null) { + logWtf("IkeSession was non-null in onQuitting"); + mIkeSession.kill(); + mIkeSession = null; + } + // No need to call setInterfaceDown(); the IpSecInterface is being fully torn down. if (mTunnelIface != null) { mTunnelIface.close(); @@ -1863,6 +1875,7 @@ public class VcnGatewayConnection extends StateMachine { if (mUnderlying == null) { logWtf("Underlying network was null in retry state"); + teardownNetwork(); transitionTo(mDisconnectedState); } else { // Safe to blindly set up, as it is cancelled and cleared on exiting this state @@ -1879,6 +1892,7 @@ public class VcnGatewayConnection extends StateMachine { // If new underlying is null, all networks were lost; go back to disconnected. if (mUnderlying == null) { + teardownNetwork(); transitionTo(mDisconnectedState); return; } else if (oldUnderlying != null @@ -2134,6 +2148,8 @@ public class VcnGatewayConnection extends StateMachine { + LogUtils.getHashedSubscriptionGroup(mSubscriptionGroup) + "-" + mConnectionConfig.getGatewayConnectionName() + + "-" + + System.identityHashCode(this) + "] "; } diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java index a88f112f4502..69407657b3c4 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java @@ -16,10 +16,16 @@ package com.android.server.vcn; +import static com.android.server.vcn.VcnGatewayConnection.VcnNetworkAgent; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; @@ -33,16 +39,20 @@ import org.junit.runner.RunWith; @SmallTest public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnectionTestBase { private long mFirstRetryInterval; + private VcnNetworkAgent mNetworkAgent; @Before public void setUp() throws Exception { super.setUp(); mFirstRetryInterval = mConfig.getRetryIntervalsMillis()[0]; + mNetworkAgent = mock(VcnNetworkAgent.class); mGatewayConnection.setUnderlyingNetwork(TEST_UNDERLYING_NETWORK_RECORD_1); mGatewayConnection.transitionTo(mGatewayConnection.mRetryTimeoutState); mTestLooper.dispatchAll(); + + mGatewayConnection.setNetworkAgent(mNetworkAgent); } @Test @@ -54,6 +64,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect assertEquals(mGatewayConnection.mConnectingState, mGatewayConnection.getCurrentState()); verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */); + + assertNotNull(mGatewayConnection.getNetworkAgent()); + verify(mNetworkAgent, never()).unregister(); } @Test @@ -65,6 +78,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect assertEquals(mGatewayConnection.mRetryTimeoutState, mGatewayConnection.getCurrentState()); verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, false /* expectCanceled */); + + assertNotNull(mGatewayConnection.getNetworkAgent()); + verify(mNetworkAgent, never()).unregister(); } @Test @@ -76,6 +92,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect assertEquals(mGatewayConnection.mDisconnectedState, mGatewayConnection.getCurrentState()); verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */); + + assertNull(mGatewayConnection.getNetworkAgent()); + verify(mNetworkAgent).unregister(); } @Test @@ -93,6 +112,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect assertEquals(mGatewayConnection.mConnectingState, mGatewayConnection.getCurrentState()); verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */); + + assertNotNull(mGatewayConnection.getNetworkAgent()); + verify(mNetworkAgent, never()).unregister(); } @Test @@ -108,6 +130,9 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect assertNull(mGatewayConnection.getCurrentState()); assertTrue(mGatewayConnection.isQuitting()); + + assertNull(mGatewayConnection.getNetworkAgent()); + verify(mNetworkAgent).unregister(); } @Test @@ -117,5 +142,8 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect assertEquals(mGatewayConnection.mDisconnectedState, mGatewayConnection.getCurrentState()); assertFalse(mGatewayConnection.isQuitting()); + + assertNull(mGatewayConnection.getNetworkAgent()); + verify(mNetworkAgent).unregister(); } } diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java index a4f95e03e9bd..83610e0b7a67 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionTest.java @@ -24,8 +24,12 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VCN_MANAGED; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; +import static com.android.server.vcn.VcnGatewayConnection.VcnIkeSession; +import static com.android.server.vcn.VcnGatewayConnection.VcnNetworkAgent; + import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Matchers.eq; @@ -191,4 +195,23 @@ public class VcnGatewayConnectionTest extends VcnGatewayConnectionTestBase { verify(mDisconnectRequestAlarm).cancel(); } + + @Test + public void testQuittingCleansUpPersistentState() { + final VcnIkeSession vcnIkeSession = mock(VcnIkeSession.class); + final VcnNetworkAgent vcnNetworkAgent = mock(VcnNetworkAgent.class); + + mGatewayConnection.setIkeSession(vcnIkeSession); + mGatewayConnection.setNetworkAgent(vcnNetworkAgent); + + mGatewayConnection.quitNow(); + mTestLooper.dispatchAll(); + + assertNull(mGatewayConnection.getIkeSession()); + verify(vcnIkeSession).kill(); + assertNull(mGatewayConnection.getNetworkAgent()); + verify(vcnNetworkAgent).unregister(); + + verifyWakeLockReleased(); + } } -- cgit v1.2.3-59-g8ed1b