From 3c4ebbbbc117f3a567d162768a70331dc0278511 Mon Sep 17 00:00:00 2001 From: Ilya Matyukhin Date: Thu, 23 Jun 2022 07:23:25 +0000 Subject: Don't crash on illegal biometric states Bug: 233448368 Test: atest BiometricSchedulerOperationTest Change-Id: I81d648e584b90ad00f8200fe981a113dd44bd7c2 Merged-In: I81d648e584b90ad00f8200fe981a113dd44bd7c2 --- .../sensors/BiometricSchedulerOperation.java | 84 +++++++++++++---- .../sensors/BiometricSchedulerOperationTest.java | 101 ++++++++++++++++++++- 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 @@ -125,6 +128,34 @@ public class BiometricSchedulerOperationTest { verify(mClientMonitor, never()).start(any()); } + @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); @@ -169,8 +200,35 @@ public class BiometricSchedulerOperationTest { verify(mOnStartCallback).onClientFinished(eq(mClientMonitor), eq(false)); } + @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); @@ -208,6 +271,16 @@ public class BiometricSchedulerOperationTest { assertThrows(IllegalStateException.class, () -> mOperation.abort()); } + @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); @@ -253,6 +326,30 @@ public class BiometricSchedulerOperationTest { verify(mClientMonitor).destroy(); } + @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); -- cgit v1.2.3-59-g8ed1b From 8f803aea46db62c94269e779a57b573545802090 Mon Sep 17 00:00:00 2001 From: Ilya Matyukhin Date: Thu, 23 Jun 2022 07:40:59 +0000 Subject: Do nothing in duplicate onDialogAnimatedIn calls Bug: 233448368 Test: atest AuthSessionTest Change-Id: I171604ee6dc40f1d44ac65525c32ff97f1fa1d76 Merged-In: I171604ee6dc40f1d44ac65525c32ff97f1fa1d76 --- .../com/android/server/biometrics/AuthSession.java | 5 +-- .../android/server/biometrics/AuthSessionTest.java | 43 +++++++++++++++++++++- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/biometrics/AuthSession.java b/services/core/java/com/android/server/biometrics/AuthSession.java index cc49f07dd0e5..41ca13f5d5f5 100644 --- a/services/core/java/com/android/server/biometrics/AuthSession.java +++ b/services/core/java/com/android/server/biometrics/AuthSession.java @@ -538,13 +538,12 @@ public final class AuthSession implements IBinder.DeathRecipient { void onDialogAnimatedIn() { if (mState != STATE_AUTH_STARTED) { - Slog.w(TAG, "onDialogAnimatedIn, unexpected state: " + mState); + Slog.e(TAG, "onDialogAnimatedIn, unexpected state: " + mState); + return; } mState = STATE_AUTH_STARTED_UI_SHOWING; - startAllPreparedFingerprintSensors(); - mState = STATE_AUTH_STARTED_UI_SHOWING; } void onTryAgainPressed() { diff --git a/services/tests/servicestests/src/com/android/server/biometrics/AuthSessionTest.java b/services/tests/servicestests/src/com/android/server/biometrics/AuthSessionTest.java index 25cf8a86baad..e95924ad7109 100644 --- a/services/tests/servicestests/src/com/android/server/biometrics/AuthSessionTest.java +++ b/services/tests/servicestests/src/com/android/server/biometrics/AuthSessionTest.java @@ -20,7 +20,9 @@ import static android.hardware.biometrics.BiometricAuthenticator.TYPE_FACE; import static android.hardware.biometrics.BiometricAuthenticator.TYPE_FINGERPRINT; import static android.hardware.biometrics.BiometricPrompt.DISMISSED_REASON_NEGATIVE; -import static com.android.server.biometrics.BiometricServiceStateProto.*; +import static com.android.server.biometrics.BiometricServiceStateProto.STATE_AUTH_CALLED; +import static com.android.server.biometrics.BiometricServiceStateProto.STATE_AUTH_STARTED; +import static com.android.server.biometrics.BiometricServiceStateProto.STATE_AUTH_STARTED_UI_SHOWING; import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertFalse; @@ -32,6 +34,8 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -279,6 +283,43 @@ public class AuthSessionTest { session.mPreAuthInfo.eligibleSensors.get(fingerprintSensorId).getSensorState()); } + @Test + public void testOnDialogAnimatedInDoesNothingDuringInvalidState() throws Exception { + setupFingerprint(0 /* id */, FingerprintSensorProperties.TYPE_UDFPS_OPTICAL); + final long operationId = 123; + final int userId = 10; + + final AuthSession session = createAuthSession(mSensors, + false /* checkDevicePolicyManager */, + Authenticators.BIOMETRIC_STRONG, + TEST_REQUEST_ID, + operationId, + userId); + final IBiometricAuthenticator impl = session.mPreAuthInfo.eligibleSensors.get(0).impl; + + session.goToInitialState(); + for (BiometricSensor sensor : session.mPreAuthInfo.eligibleSensors) { + assertEquals(BiometricSensor.STATE_WAITING_FOR_COOKIE, sensor.getSensorState()); + session.onCookieReceived( + session.mPreAuthInfo.eligibleSensors.get(sensor.id).getCookie()); + } + assertTrue(session.allCookiesReceived()); + assertEquals(STATE_AUTH_STARTED, session.getState()); + verify(impl, never()).startPreparedClient(anyInt()); + + // First invocation should start the client monitor. + session.onDialogAnimatedIn(); + assertEquals(STATE_AUTH_STARTED_UI_SHOWING, session.getState()); + verify(impl).startPreparedClient(anyInt()); + + // Subsequent invocations should not start the client monitor again. + session.onDialogAnimatedIn(); + session.onDialogAnimatedIn(); + session.onDialogAnimatedIn(); + assertEquals(STATE_AUTH_STARTED_UI_SHOWING, session.getState()); + verify(impl, times(1)).startPreparedClient(anyInt()); + } + @Test public void testCancelAuthentication_whenStateAuthCalled_invokesCancel() throws RemoteException { -- cgit v1.2.3-59-g8ed1b