Ensure VcnGatewayConnection#isQuitting never gets unset after being set
This change ensures that the VcnGatewayConnection can never abort a
quitting command; specifically, if a non-quitting disconnect request is
processed after a quitting disconnect request, the isQuitting value MUST
continue to stay set (as true).
Failure to OR the values results in orphaned VcnGatewayConnection(s),
and the VCN network thrashing.
Additionally reduce verbosity of local logs, to ensure it better
captures failures and logWtf(s)
Bug: 192776413
Test: atest FrameworksVcnTests
Change-Id: Iec8dae701838794261957609bae4e270eaa9cdc7
diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java
index 70b0fc1..8a1c684 100644
--- a/services/core/java/com/android/server/VcnManagementService.java
+++ b/services/core/java/com/android/server/VcnManagementService.java
@@ -520,12 +520,14 @@
@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.
@@ -1011,18 +1013,15 @@
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 44a6d13..9c3721b1 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 @@
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 3842769..3c0a05b 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.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 @@
* <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 @@
private void acquireWakeLock() {
mVcnContext.ensureRunningOnLooperThread();
- if (!mIsQuitting) {
+ if (!mIsQuitting.getValue()) {
mWakeLock.acquire();
logVdbg("Wakelock acquired: " + mWakeLock);
@@ -1297,7 +1303,9 @@
// 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 @@
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 @@
break;
case EVENT_DISCONNECT_REQUESTED:
if (((EventDisconnectRequestedInfo) msg.obj).shouldQuit) {
- mIsQuitting = true;
+ mIsQuitting.setTrue();
quitNow();
}
@@ -1451,7 +1459,10 @@
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 @@
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 @@
teardownAsynchronously();
} /* networkUnwantedCallback */,
(status) -> {
- if (mIsQuitting) {
+ if (mIsQuitting.getValue()) {
return; // Ignore; VcnGatewayConnection quitting or already quit
}
@@ -2180,18 +2191,15 @@
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 @@
+ (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 @@
@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 0000000..e79bb2d
--- /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 6bfbfb1..0f84f6e 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 @@
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 acc8bf9..d1f3a21 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 @@
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 ac0edaa..2056eea 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 @@
true /* isMobileDataEnabled */,
mDeps);
- vgc.setIsQuitting(true);
+ vgc.setQuitting();
vgc.transitionTo(vgc.mDisconnectedState);
mTestLooper.dispatchAll();
@@ -102,6 +102,10 @@
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 9da8b45..78aefad 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 @@
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 6940765..1c85979 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 @@
.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 */);
