Don't crash on illegal biometric states

Bug: 233448368
Test: atest BiometricSchedulerOperationTest
Change-Id: I81d648e584b90ad00f8200fe981a113dd44bd7c2
Merged-In: I81d648e584b90ad00f8200fe981a113dd44bd7c2
(cherry picked from commit 3c4ebbbbc117f3a567d162768a70331dc0278511)
Merged-In: I81d648e584b90ad00f8200fe981a113dd44bd7c2

2 files changed, 163 insertions, 22 deletions

diff --git a/services/core/java/com/android/server/biometrics/sensors/BiometricSchedulerOperation.java b/services/core/java/com/android/server/biometrics/sensors/BiometricSchedulerOperation.java
index 968146a166ed..ef2931ff5850 100644
--- a/services/core/java/com/android/server/biometrics/sensors/BiometricSchedulerOperation.java
+++ b/services/core/java/com/android/server/biometrics/sensors/BiometricSchedulerOperation.java
@@ -20,14 +20,18 @@ import android.annotation.IntDef;
 import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.hardware.biometrics.BiometricConstants;
+import android.os.Build;
 import android.os.Handler;
 import android.os.IBinder;
 import android.util.Slog;
 
 import com.android.internal.annotations.VisibleForTesting;
+import com.android.internal.util.ArrayUtils;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
+import java.util.Arrays;
+import java.util.function.BooleanSupplier;
 
 /**
  * Contains all the necessary information for a HAL operation.
@@ -84,6 +88,8 @@ public class BiometricSchedulerOperation {
     private final BaseClientMonitor mClientMonitor;
     @Nullable
     private final ClientMonitorCallback mClientCallback;
+    @NonNull
+    private final BooleanSupplier mIsDebuggable;
     @Nullable
     private ClientMonitorCallback mOnStartCallback;
     @OperationState
@@ -99,14 +105,33 @@ public class BiometricSchedulerOperation {
         this(clientMonitor, callback, STATE_WAITING_IN_QUEUE);
     }
 
+    @VisibleForTesting
+    BiometricSchedulerOperation(
+            @NonNull BaseClientMonitor clientMonitor,
+            @Nullable ClientMonitorCallback callback,
+            @NonNull BooleanSupplier isDebuggable
+    ) {
+        this(clientMonitor, callback, STATE_WAITING_IN_QUEUE, isDebuggable);
+    }
+
     protected BiometricSchedulerOperation(
             @NonNull BaseClientMonitor clientMonitor,
             @Nullable ClientMonitorCallback callback,
             @OperationState int state
     ) {
+        this(clientMonitor, callback, state, Build::isDebuggable);
+    }
+
+    private BiometricSchedulerOperation(
+            @NonNull BaseClientMonitor clientMonitor,
+            @Nullable ClientMonitorCallback callback,
+            @OperationState int state,
+            @NonNull BooleanSupplier isDebuggable
+    ) {
         mClientMonitor = clientMonitor;
         mClientCallback = callback;
         mState = state;
+        mIsDebuggable = isDebuggable;
         mCancelWatchdog = () -> {
             if (!isFinished()) {
                 Slog.e(TAG, "[Watchdog Triggered]: " + this);
@@ -144,13 +169,19 @@ public class BiometricSchedulerOperation {
      * @return if this operation started
      */
     public boolean start(@NonNull ClientMonitorCallback callback) {
-        checkInState("start",
+        if (errorWhenNoneOf("start",
                 STATE_WAITING_IN_QUEUE,
                 STATE_WAITING_FOR_COOKIE,
-                STATE_WAITING_IN_QUEUE_CANCELING);
+                STATE_WAITING_IN_QUEUE_CANCELING)) {
+            return false;
+        }
 
         if (mClientMonitor.getCookie() != 0) {
-            throw new IllegalStateException("operation requires cookie");
+            String err = "operation requires cookie";
+            if (mIsDebuggable.getAsBoolean()) {
+                throw new IllegalStateException(err);
+            }
+            Slog.e(TAG, err);
         }
 
         return doStart(callback);
@@ -164,16 +195,18 @@ public class BiometricSchedulerOperation {
      * @return if this operation started
      */
     public boolean startWithCookie(@NonNull ClientMonitorCallback callback, int cookie) {
-        checkInState("start",
-                STATE_WAITING_IN_QUEUE,
-                STATE_WAITING_FOR_COOKIE,
-                STATE_WAITING_IN_QUEUE_CANCELING);
-
         if (mClientMonitor.getCookie() != cookie) {
             Slog.e(TAG, "Mismatched cookie for operation: " + this + ", received: " + cookie);
             return false;
         }
 
+        if (errorWhenNoneOf("start",
+                STATE_WAITING_IN_QUEUE,
+                STATE_WAITING_FOR_COOKIE,
+                STATE_WAITING_IN_QUEUE_CANCELING)) {
+            return false;
+        }
+
         return doStart(callback);
     }
 
@@ -217,10 +250,12 @@ public class BiometricSchedulerOperation {
      * immediately abort the operation and notify the client that it has finished unsuccessfully.
      */
     public void abort() {
-        checkInState("cannot abort a non-pending operation",
+        if (errorWhenNoneOf("abort",
                 STATE_WAITING_IN_QUEUE,
                 STATE_WAITING_FOR_COOKIE,
-                STATE_WAITING_IN_QUEUE_CANCELING);
+                STATE_WAITING_IN_QUEUE_CANCELING)) {
+            return;
+        }
 
         if (isHalOperation()) {
             ((HalClientMonitor<?>) mClientMonitor).unableToStart();
@@ -247,7 +282,9 @@ public class BiometricSchedulerOperation {
      *                 the callback used from {@link #start(ClientMonitorCallback)} is used)
      */
     public void cancel(@NonNull Handler handler, @NonNull ClientMonitorCallback callback) {
-        checkNotInState("cancel", STATE_FINISHED);
+        if (errorWhenOneOf("cancel", STATE_FINISHED)) {
+            return;
+        }
 
         final int currentState = mState;
         if (!isInterruptable()) {
@@ -402,21 +439,28 @@ public class BiometricSchedulerOperation {
         return mClientMonitor;
     }
 
-    private void checkNotInState(String message, @OperationState int... states) {
-        for (int state : states) {
-            if (mState == state) {
-                throw new IllegalStateException(message + ": illegal state= " + state);
+    private boolean errorWhenOneOf(String op, @OperationState int... states) {
+        final boolean isError = ArrayUtils.contains(states, mState);
+        if (isError) {
+            String err = op + ": mState must not be " + mState;
+            if (mIsDebuggable.getAsBoolean()) {
+                throw new IllegalStateException(err);
             }
+            Slog.e(TAG, err);
         }
+        return isError;
     }
 
-    private void checkInState(String message, @OperationState int... states) {
-        for (int state : states) {
-            if (mState == state) {
-                return;
+    private boolean errorWhenNoneOf(String op, @OperationState int... states) {
+        final boolean isError = !ArrayUtils.contains(states, mState);
+        if (isError) {
+            String err = op + ": mState=" + mState + " must be one of " + Arrays.toString(states);
+            if (mIsDebuggable.getAsBoolean()) {
+                throw new IllegalStateException(err);
             }
+            Slog.e(TAG, err);
         }
-        throw new IllegalStateException(message + ": illegal state= " + mState);
+        return isError;
     }
 
     @Override
diff --git a/services/tests/servicestests/src/com/android/server/biometrics/sensors/BiometricSchedulerOperationTest.java b/services/tests/servicestests/src/com/android/server/biometrics/sensors/BiometricSchedulerOperationTest.java
index c17347320f52..9e9d70332f00 100644
--- a/services/tests/servicestests/src/com/android/server/biometrics/sensors/BiometricSchedulerOperationTest.java
+++ b/services/tests/servicestests/src/com/android/server/biometrics/sensors/BiometricSchedulerOperationTest.java
@@ -80,11 +80,14 @@ public class BiometricSchedulerOperationTest {
 
     private Handler mHandler;
     private BiometricSchedulerOperation mOperation;
+    private boolean mIsDebuggable;
 
     @Before
     public void setUp() {
         mHandler = new Handler(TestableLooper.get(this).getLooper());
-        mOperation = new BiometricSchedulerOperation(mClientMonitor, mClientCallback);
+        mIsDebuggable = false;
+        mOperation = new BiometricSchedulerOperation(mClientMonitor, mClientCallback,
+                () -> mIsDebuggable);
     }
 
     @Test
@@ -126,6 +129,34 @@ public class BiometricSchedulerOperationTest {
     }
 
     @Test
+    public void testSecondStartWithCookieCrashesWhenDebuggable() {
+        final int cookie = 5;
+        mIsDebuggable = true;
+        when(mClientMonitor.getCookie()).thenReturn(cookie);
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        final boolean started = mOperation.startWithCookie(mOnStartCallback, cookie);
+        assertThat(started).isTrue();
+
+        assertThrows(IllegalStateException.class,
+                () -> mOperation.startWithCookie(mOnStartCallback, cookie));
+    }
+
+    @Test
+    public void testSecondStartWithCookieFailsNicelyWhenNotDebuggable() {
+        final int cookie = 5;
+        mIsDebuggable = false;
+        when(mClientMonitor.getCookie()).thenReturn(cookie);
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        final boolean started = mOperation.startWithCookie(mOnStartCallback, cookie);
+        assertThat(started).isTrue();
+
+        final boolean startedAgain = mOperation.startWithCookie(mOnStartCallback, cookie);
+        assertThat(startedAgain).isFalse();
+    }
+
+    @Test
     public void startsWhenReadyAndHalAvailable() {
         when(mClientMonitor.getCookie()).thenReturn(0);
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
@@ -170,7 +201,34 @@ public class BiometricSchedulerOperationTest {
     }
 
     @Test
+    public void secondStartCrashesWhenDebuggable() {
+        mIsDebuggable = true;
+        when(mClientMonitor.getCookie()).thenReturn(0);
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        final boolean started = mOperation.start(mOnStartCallback);
+        assertThat(started).isTrue();
+
+        assertThrows(IllegalStateException.class, () -> mOperation.start(mOnStartCallback));
+    }
+
+    @Test
+    public void secondStartFailsNicelyWhenNotDebuggable() {
+        mIsDebuggable = false;
+        when(mClientMonitor.getCookie()).thenReturn(0);
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        final boolean started = mOperation.start(mOnStartCallback);
+        assertThat(started).isTrue();
+
+        final boolean startedAgain = mOperation.start(mOnStartCallback);
+        assertThat(startedAgain).isFalse();
+    }
+
+    @Test
     public void doesNotStartWithCookie() {
+        // This class only throws exceptions when debuggable.
+        mIsDebuggable = true;
         when(mClientMonitor.getCookie()).thenReturn(9);
         assertThrows(IllegalStateException.class,
                 () -> mOperation.start(mock(ClientMonitorCallback.class)));
@@ -178,6 +236,8 @@ public class BiometricSchedulerOperationTest {
 
     @Test
     public void cannotRestart() {
+        // This class only throws exceptions when debuggable.
+        mIsDebuggable = true;
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
 
         mOperation.start(mOnStartCallback);
@@ -188,6 +248,8 @@ public class BiometricSchedulerOperationTest {
 
     @Test
     public void abortsNotRunning() {
+        // This class only throws exceptions when debuggable.
+        mIsDebuggable = true;
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
 
         mOperation.abort();
@@ -200,7 +262,8 @@ public class BiometricSchedulerOperationTest {
     }
 
     @Test
-    public void cannotAbortRunning() {
+    public void abortCrashesWhenDebuggableIfOperationIsRunning() {
+        mIsDebuggable = true;
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
 
         mOperation.start(mOnStartCallback);
@@ -209,6 +272,16 @@ public class BiometricSchedulerOperationTest {
     }
 
     @Test
+    public void abortFailsNicelyWhenNotDebuggableIfOperationIsRunning() {
+        mIsDebuggable = false;
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        mOperation.start(mOnStartCallback);
+
+        mOperation.abort();
+    }
+
+    @Test
     public void cancel() {
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
 
@@ -254,6 +327,30 @@ public class BiometricSchedulerOperationTest {
     }
 
     @Test
+    public void cancelCrashesWhenDebuggableIfOperationIsFinished() {
+        mIsDebuggable = true;
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        mOperation.abort();
+        assertThat(mOperation.isFinished()).isTrue();
+
+        final ClientMonitorCallback cancelCb = mock(ClientMonitorCallback.class);
+        assertThrows(IllegalStateException.class, () -> mOperation.cancel(mHandler, cancelCb));
+    }
+
+    @Test
+    public void cancelFailsNicelyWhenNotDebuggableIfOperationIsFinished() {
+        mIsDebuggable = false;
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        mOperation.abort();
+        assertThat(mOperation.isFinished()).isTrue();
+
+        final ClientMonitorCallback cancelCb = mock(ClientMonitorCallback.class);
+        mOperation.cancel(mHandler, cancelCb);
+    }
+
+    @Test
     public void markCanceling() {
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);