diff options
9 files changed, 86 insertions, 23 deletions
diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java index 207a5e3db06e..b068f86ff0ab 100644 --- a/services/core/java/com/android/server/VcnManagementService.java +++ b/services/core/java/com/android/server/VcnManagementService.java @@ -550,12 +550,14 @@ public class VcnManagementService extends IVcnManagementService.Stub { @GuardedBy("mLock") private void stopVcnLocked(@NonNull ParcelUuid uuidToTeardown) { - final Vcn vcnToTeardown = mVcns.remove(uuidToTeardown); + // Remove in 2 steps. Make sure teardownAsync is triggered before removing from the map. + final Vcn vcnToTeardown = mVcns.get(uuidToTeardown); if (vcnToTeardown == null) { return; } vcnToTeardown.teardownAsynchronously(); + mVcns.remove(uuidToTeardown); // Now that the VCN is removed, notify all registered listeners to refresh their // UnderlyingNetworkPolicy. @@ -1054,18 +1056,15 @@ public class VcnManagementService extends IVcnManagementService.Stub { private void logVdbg(String msg) { if (VDBG) { Slog.v(TAG, msg); - LOCAL_LOG.log(TAG + " VDBG: " + msg); } } private void logDbg(String msg) { Slog.d(TAG, msg); - LOCAL_LOG.log(TAG + " DBG: " + msg); } private void logDbg(String msg, Throwable tr) { Slog.d(TAG, msg, tr); - LOCAL_LOG.log(TAG + " DBG: " + msg + tr); } private void logErr(String msg) { diff --git a/services/core/java/com/android/server/vcn/Vcn.java b/services/core/java/com/android/server/vcn/Vcn.java index 44a6d13153fd..9c3721b15f32 100644 --- a/services/core/java/com/android/server/vcn/Vcn.java +++ b/services/core/java/com/android/server/vcn/Vcn.java @@ -528,18 +528,15 @@ public class Vcn extends Handler { private void logVdbg(String msg) { if (VDBG) { Slog.v(TAG, getLogPrefix() + msg); - LOCAL_LOG.log(getLogPrefix() + "VDBG: " + msg); } } private void logDbg(String msg) { Slog.d(TAG, getLogPrefix() + msg); - LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg); } private void logDbg(String msg, Throwable tr) { Slog.d(TAG, getLogPrefix() + msg, tr); - LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg + tr); } private void logErr(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 38427697a9cd..3c0a05b6edf7 100644 --- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java +++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java @@ -92,6 +92,7 @@ import com.android.server.vcn.UnderlyingNetworkTracker.UnderlyingNetworkTrackerC import com.android.server.vcn.Vcn.VcnGatewayStatusCallback; import com.android.server.vcn.util.LogUtils; import com.android.server.vcn.util.MtuUtils; +import com.android.server.vcn.util.OneWayBoolean; import java.io.IOException; import java.net.Inet4Address; @@ -551,8 +552,13 @@ public class VcnGatewayConnection extends StateMachine { * <p>This variable is false for the lifecycle of the VcnGatewayConnection, until a command to * teardown has been received. This may be flipped due to events such as the Network becoming * unwanted, the owning VCN entering safe mode, or an irrecoverable internal failure. + * + * <p>WARNING: Assignments to this MUST ALWAYS (except for testing) use the or operator ("|="), + * otherwise the flag may be flipped back to false after having been set to true. This could + * lead to a case where the Vcn parent instance has commanded a teardown, but a spurious + * non-quitting disconnect request could flip this back to true. */ - private boolean mIsQuitting = false; + private OneWayBoolean mIsQuitting = new OneWayBoolean(); /** * Whether the VcnGatewayConnection is in safe mode. @@ -794,7 +800,7 @@ public class VcnGatewayConnection extends StateMachine { private void acquireWakeLock() { mVcnContext.ensureRunningOnLooperThread(); - if (!mIsQuitting) { + if (!mIsQuitting.getValue()) { mWakeLock.acquire(); logVdbg("Wakelock acquired: " + mWakeLock); @@ -1297,7 +1303,9 @@ public class VcnGatewayConnection extends StateMachine { // TODO(b/180526152): notify VcnStatusCallback for Network loss logDbg("Tearing down. Cause: " + info.reason); - mIsQuitting = info.shouldQuit; + if (info.shouldQuit) { + mIsQuitting.setTrue(); + } teardownNetwork(); @@ -1341,7 +1349,7 @@ public class VcnGatewayConnection extends StateMachine { private class DisconnectedState extends BaseState { @Override protected void enterState() { - if (mIsQuitting) { + if (mIsQuitting.getValue()) { quitNow(); // Ignore all queued events; cleanup is complete. } @@ -1365,7 +1373,7 @@ public class VcnGatewayConnection extends StateMachine { break; case EVENT_DISCONNECT_REQUESTED: if (((EventDisconnectRequestedInfo) msg.obj).shouldQuit) { - mIsQuitting = true; + mIsQuitting.setTrue(); quitNow(); } @@ -1451,7 +1459,10 @@ public class VcnGatewayConnection extends StateMachine { break; case EVENT_DISCONNECT_REQUESTED: EventDisconnectRequestedInfo info = ((EventDisconnectRequestedInfo) msg.obj); - mIsQuitting = info.shouldQuit; + if (info.shouldQuit) { + mIsQuitting.setTrue(); + } + teardownNetwork(); if (info.reason.equals(DISCONNECT_REASON_UNDERLYING_NETWORK_LOST)) { @@ -1467,7 +1478,7 @@ public class VcnGatewayConnection extends StateMachine { case EVENT_SESSION_CLOSED: mIkeSession = null; - if (!mIsQuitting && mUnderlying != null) { + if (!mIsQuitting.getValue() && mUnderlying != null) { transitionTo(mSkipRetryTimeout ? mConnectingState : mRetryTimeoutState); } else { teardownNetwork(); @@ -1626,7 +1637,7 @@ public class VcnGatewayConnection extends StateMachine { teardownAsynchronously(); } /* networkUnwantedCallback */, (status) -> { - if (mIsQuitting) { + if (mIsQuitting.getValue()) { return; // Ignore; VcnGatewayConnection quitting or already quit } @@ -2180,18 +2191,15 @@ public class VcnGatewayConnection extends StateMachine { private void logVdbg(String msg) { if (VDBG) { Slog.v(TAG, getLogPrefix() + msg); - LOCAL_LOG.log(getLogPrefix() + "VDBG: " + msg); } } private void logDbg(String msg) { Slog.d(TAG, getLogPrefix() + msg); - LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg); } private void logDbg(String msg, Throwable tr) { Slog.d(TAG, getLogPrefix() + msg, tr); - LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg + tr); } private void logWarn(String msg) { @@ -2238,7 +2246,7 @@ public class VcnGatewayConnection extends StateMachine { + (getCurrentState() == null ? null : getCurrentState().getClass().getSimpleName())); - pw.println("mIsQuitting: " + mIsQuitting); + pw.println("mIsQuitting: " + mIsQuitting.getValue()); pw.println("mIsInSafeMode: " + mIsInSafeMode); pw.println("mCurrentToken: " + mCurrentToken); pw.println("mFailedAttempts: " + mFailedAttempts); @@ -2275,12 +2283,12 @@ public class VcnGatewayConnection extends StateMachine { @VisibleForTesting(visibility = Visibility.PRIVATE) boolean isQuitting() { - return mIsQuitting; + return mIsQuitting.getValue(); } @VisibleForTesting(visibility = Visibility.PRIVATE) - void setIsQuitting(boolean isQuitting) { - mIsQuitting = isQuitting; + void setQuitting() { + mIsQuitting.setTrue(); } @VisibleForTesting(visibility = Visibility.PRIVATE) diff --git a/services/core/java/com/android/server/vcn/util/OneWayBoolean.java b/services/core/java/com/android/server/vcn/util/OneWayBoolean.java new file mode 100644 index 000000000000..e79bb2d2547d --- /dev/null +++ b/services/core/java/com/android/server/vcn/util/OneWayBoolean.java @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2021 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.vcn.util; + +/** + * OneWayBoolean is an abstraction for a boolean that MUST only ever be flipped from false to true + * + * <p>This class allows the providing of a guarantee that a flag will never be flipped back after + * being set. + * + * @hide + */ +public class OneWayBoolean { + private boolean mValue = false; + + /** Get boolean value. */ + public boolean getValue() { + return mValue; + } + + /** Sets the value to true. */ + public void setTrue() { + mValue = true; + } +} diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java index 6bfbfb1c8496..0f84f6ebe522 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java @@ -578,6 +578,10 @@ public class VcnGatewayConnectionConnectedStateTest extends VcnGatewayConnection mGatewayConnection.teardownAsynchronously(); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState()); assertTrue(mGatewayConnection.isQuitting()); } diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java index acc8bf98e95b..d1f3a210d870 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java @@ -127,6 +127,10 @@ public class VcnGatewayConnectionConnectingStateTest extends VcnGatewayConnectio mGatewayConnection.teardownAsynchronously(); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState()); assertTrue(mGatewayConnection.isQuitting()); } diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java index ac0edaa3b579..2056eea42ce6 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java @@ -68,7 +68,7 @@ public class VcnGatewayConnectionDisconnectedStateTest extends VcnGatewayConnect true /* isMobileDataEnabled */, mDeps); - vgc.setIsQuitting(true); + vgc.setQuitting(); vgc.transitionTo(vgc.mDisconnectedState); mTestLooper.dispatchAll(); @@ -102,6 +102,10 @@ public class VcnGatewayConnectionDisconnectedStateTest extends VcnGatewayConnect mGatewayConnection.teardownAsynchronously(); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + assertNull(mGatewayConnection.getCurrentState()); verify(mIpSecSvc).deleteTunnelInterface(eq(TEST_IPSEC_TUNNEL_RESOURCE_ID), any()); verifySafeModeTimeoutAlarmAndGetCallback(true /* expectCanceled */); diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java index 9da8b451c9fc..78aefad9f8ff 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java @@ -79,6 +79,10 @@ public class VcnGatewayConnectionDisconnectingStateTest extends VcnGatewayConnec mGatewayConnection.teardownAsynchronously(); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + // Should do nothing; already tearing down. assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState()); verifyTeardownTimeoutAlarmAndGetCallback(false /* expectCanceled */); diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java index 69407657b3c4..1c859790a2fe 100644 --- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java +++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java @@ -90,6 +90,10 @@ public class VcnGatewayConnectionRetryTimeoutStateTest extends VcnGatewayConnect .onSelectedUnderlyingNetworkChanged(null); mTestLooper.dispatchAll(); + // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag + mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false); + mTestLooper.dispatchAll(); + assertEquals(mGatewayConnection.mDisconnectedState, mGatewayConnection.getCurrentState()); verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */); |