From 336ad77e71b9564c8ead9a10021876e9c71906fc Mon Sep 17 00:00:00 2001 From: Kevin Chyn Date: Wed, 30 Sep 2020 16:47:48 -0700 Subject: [PATCH] Handle case where ClientMonitors should not be started Currently, the only case is when the HAL is null. For this, there are two cases: 1) Non-BiometricPrompt operation For these, clean up the current client, and do the same for any operation that's currently pending. See code comments for more details 2) BiometricPrompt operation For these, just clean up the current BiometricPrompt operation Fixes: 165636620 Test: atest BiometricSchedulerTest Test: smoke test: enroll, auth on existing devices Change-Id: Iff64d5dba8dc6ebd73f0f9fba233390e76ca8a89 --- .../sensors/BiometricScheduler.java | 96 +++++++++--- .../sensors/BiometricSchedulerTest.java | 137 +++++++++++++++++- 2 files changed, 211 insertions(+), 22 deletions(-) diff --git a/services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java b/services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java index 588e865112c28..ce2d3405ddc68 100644 --- a/services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java +++ b/services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java @@ -28,6 +28,7 @@ import android.os.RemoteException; import android.os.ServiceManager; import android.util.Slog; +import com.android.internal.annotations.VisibleForTesting; import com.android.server.biometrics.sensors.fingerprint.GestureAvailabilityDispatcher; import java.io.PrintWriter; @@ -37,9 +38,9 @@ import java.text.SimpleDateFormat; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Date; +import java.util.Deque; import java.util.List; import java.util.Locale; -import java.util.Queue; /** * A scheduler for biometric HAL operations. Maintains a queue of {@link ClientMonitor} operations, @@ -53,7 +54,8 @@ public class BiometricScheduler { /** * Contains all the necessary information for a HAL operation. */ - private static final class Operation { + @VisibleForTesting + static final class Operation { /** * The operation is added to the list of pending operations and waiting for its turn. @@ -176,8 +178,8 @@ public class BiometricScheduler { @NonNull private final IBiometricService mBiometricService; @NonNull private final Handler mHandler = new Handler(Looper.getMainLooper()); @NonNull private final InternalCallback mInternalCallback; - @NonNull private final Queue mPendingOperations; - @Nullable private Operation mCurrentOperation; + @VisibleForTesting @NonNull final Deque mPendingOperations; + @VisibleForTesting @Nullable Operation mCurrentOperation; @NonNull private final ArrayDeque mCrashStates; // Internal callback, notified when an operation is complete. Notifies the requester @@ -226,6 +228,18 @@ public class BiometricScheduler { } } + @VisibleForTesting + BiometricScheduler(@NonNull String tag, + @Nullable GestureAvailabilityDispatcher gestureAvailabilityDispatcher, + @NonNull IBiometricService biometricService) { + mBiometricTag = tag; + mInternalCallback = new InternalCallback(); + mGestureAvailabilityDispatcher = gestureAvailabilityDispatcher; + mPendingOperations = new ArrayDeque<>(); + mBiometricService = biometricService; + mCrashStates = new ArrayDeque<>(); + } + /** * Creates a new scheduler. * @param tag for the specific instance of the scheduler. Should be unique. @@ -234,13 +248,8 @@ public class BiometricScheduler { */ public BiometricScheduler(@NonNull String tag, @Nullable GestureAvailabilityDispatcher gestureAvailabilityDispatcher) { - mBiometricTag = tag; - mInternalCallback = new InternalCallback(); - mGestureAvailabilityDispatcher = gestureAvailabilityDispatcher; - mPendingOperations = new ArrayDeque<>(); - mBiometricService = IBiometricService.Stub.asInterface( - ServiceManager.getService(Context.BIOMETRIC_SERVICE)); - mCrashStates = new ArrayDeque<>(); + this(tag, gestureAvailabilityDispatcher, IBiometricService.Stub.asInterface( + ServiceManager.getService(Context.BIOMETRIC_SERVICE))); } /** @@ -295,9 +304,50 @@ public class BiometricScheduler { // to arrive at the head of the queue, before pinging it to start. final boolean shouldStartNow = currentClient.getCookie() == 0; if (shouldStartNow) { - Slog.d(getTag(), "[Starting] " + mCurrentOperation); - currentClient.start(getInternalCallback()); - mCurrentOperation.state = Operation.STATE_STARTED; + if (mCurrentOperation.clientMonitor.getFreshDaemon() == null) { + // Note down current length of queue + final int pendingOperationsLength = mPendingOperations.size(); + final Operation lastOperation = mPendingOperations.peekLast(); + Slog.e(getTag(), "[Unable To Start] " + mCurrentOperation + + ". Last pending operation: " + lastOperation); + + // For current operations, 1) unableToStart, which notifies the caller-side, then + // 2) notify operation's callback, to notify applicable system service that the + // operation failed. + mCurrentOperation.clientMonitor.unableToStart(); + if (mCurrentOperation.mClientCallback != null) { + mCurrentOperation.mClientCallback + .onClientFinished(mCurrentOperation.clientMonitor, false /* success */); + } + + // Then for each operation currently in the pending queue at the time of this + // failure, do the same as above. Otherwise, it's possible that something like + // setActiveUser fails, but then authenticate (for the wrong user) is invoked. + for (int i = 0; i < pendingOperationsLength; i++) { + final Operation operation = mPendingOperations.pollFirst(); + if (operation == null) { + Slog.e(getTag(), "Null operation, index: " + i + + ", expected length: " + pendingOperationsLength); + break; + } + operation.clientMonitor.unableToStart(); + if (operation.mClientCallback != null) { + operation.mClientCallback.onClientFinished(operation.clientMonitor, + false /* success */); + } + Slog.w(getTag(), "[Aborted Operation] " + operation); + } + + // It's possible that during cleanup a new set of operations came in. We can try to + // run these. A single request from the manager layer to the service layer may + // actually be multiple operations (i.e. updateActiveUser + authenticate). + mCurrentOperation = null; + startNextOperationIfIdle(); + } else { + Slog.d(getTag(), "[Starting] " + mCurrentOperation); + currentClient.start(getInternalCallback()); + mCurrentOperation.state = Operation.STATE_STARTED; + } } else { try { mBiometricService.onReadyForAuthentication(currentClient.getCookie()); @@ -338,9 +388,21 @@ public class BiometricScheduler { return; } - Slog.d(getTag(), "[Starting] Prepared client: " + mCurrentOperation); - mCurrentOperation.state = Operation.STATE_STARTED; - mCurrentOperation.clientMonitor.start(getInternalCallback()); + if (mCurrentOperation.clientMonitor.getFreshDaemon() == null) { + Slog.e(getTag(), "[Unable To Start] Prepared client: " + mCurrentOperation); + // This is BiometricPrompt trying to auth but something's wrong with the HAL. + mCurrentOperation.clientMonitor.unableToStart(); + if (mCurrentOperation.mClientCallback != null) { + mCurrentOperation.mClientCallback.onClientFinished(mCurrentOperation.clientMonitor, + false /* success */); + } + mCurrentOperation = null; + startNextOperationIfIdle(); + } else { + Slog.d(getTag(), "[Starting] Prepared client: " + mCurrentOperation); + mCurrentOperation.state = Operation.STATE_STARTED; + mCurrentOperation.clientMonitor.start(getInternalCallback()); + } } /** diff --git a/services/tests/servicestests/src/com/android/server/biometrics/sensors/BiometricSchedulerTest.java b/services/tests/servicestests/src/com/android/server/biometrics/sensors/BiometricSchedulerTest.java index 36c55cce076b1..c890c52f2181b 100644 --- a/services/tests/servicestests/src/com/android/server/biometrics/sensors/BiometricSchedulerTest.java +++ b/services/tests/servicestests/src/com/android/server/biometrics/sensors/BiometricSchedulerTest.java @@ -16,11 +16,21 @@ package com.android.server.biometrics.sensors; +import static junit.framework.Assert.assertTrue; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + import android.content.Context; +import android.hardware.biometrics.IBiometricService; import android.platform.test.annotations.Presubmit; import androidx.annotation.NonNull; -import androidx.annotation.Nullable; import androidx.test.InstrumentationRegistry; import androidx.test.filters.SmallTest; @@ -40,18 +50,21 @@ public class BiometricSchedulerTest { @Mock private Context mContext; @Mock - private ClientMonitor.LazyDaemon mLazyDaemon; + private IBiometricService mBiometricService; @Before public void setUp() { MockitoAnnotations.initMocks(this); - mScheduler = new BiometricScheduler(TAG, null /* gestureAvailabilityTracker */); + mScheduler = new BiometricScheduler(TAG, null /* gestureAvailabilityTracker */, + mBiometricService); } @Test public void testClientDuplicateFinish_ignoredBySchedulerAndDoesNotCrash() { - final ClientMonitor client1 = new TestClientMonitor(mContext, mLazyDaemon); - final ClientMonitor client2 = new TestClientMonitor(mContext, mLazyDaemon); + final ClientMonitor.LazyDaemon nonNullDaemon = () -> mock(Object.class); + + final ClientMonitor client1 = new TestClientMonitor(mContext, nonNullDaemon); + final ClientMonitor client2 = new TestClientMonitor(mContext, nonNullDaemon); mScheduler.scheduleClientMonitor(client1); mScheduler.scheduleClientMonitor(client2); @@ -59,7 +72,93 @@ public class BiometricSchedulerTest { client1.mCallback.onClientFinished(client1, true /* success */); } + @Test + public void testRemovesPendingOperations_whenNullHal_andNotBiometricPrompt() { + // Even if second client has a non-null daemon, it needs to be canceled. + Object daemon2 = mock(Object.class); + + final ClientMonitor.LazyDaemon lazyDaemon1 = () -> null; + final ClientMonitor.LazyDaemon lazyDaemon2 = () -> daemon2; + + final TestClientMonitor client1 = new TestClientMonitor(mContext, lazyDaemon1); + final TestClientMonitor client2 = new TestClientMonitor(mContext, lazyDaemon2); + + final ClientMonitor.Callback callback1 = mock(ClientMonitor.Callback.class); + final ClientMonitor.Callback callback2 = mock(ClientMonitor.Callback.class); + + // Pretend the scheduler is busy so the first operation doesn't start right away. We want + // to pretend like there are two operations in the queue before kicking things off + mScheduler.mCurrentOperation = new BiometricScheduler.Operation( + mock(ClientMonitor.class), mock(ClientMonitor.Callback.class)); + + mScheduler.scheduleClientMonitor(client1, callback1); + assertEquals(1, mScheduler.mPendingOperations.size()); + // client1 is pending. Allow the scheduler to start once second client is added. + mScheduler.mCurrentOperation = null; + mScheduler.scheduleClientMonitor(client2, callback2); + waitForIdle(); + + assertTrue(client1.wasUnableToStart()); + verify(callback1).onClientFinished(eq(client1), eq(false) /* success */); + verify(callback1, never()).onClientStarted(any()); + + assertTrue(client2.wasUnableToStart()); + verify(callback2).onClientFinished(eq(client2), eq(false) /* success */); + verify(callback2, never()).onClientStarted(any()); + + assertTrue(mScheduler.mPendingOperations.isEmpty()); + } + + @Test + public void testRemovesOnlyBiometricPromptOperation_whenNullHal() { + // Second non-BiometricPrompt client has a valid daemon + final Object daemon2 = mock(Object.class); + + final ClientMonitor.LazyDaemon lazyDaemon1 = () -> null; + final ClientMonitor.LazyDaemon lazyDaemon2 = () -> daemon2; + + final TestClientMonitor client1 = + new TestBiometricPromptClientMonitor(mContext, lazyDaemon1); + final TestClientMonitor client2 = new TestClientMonitor(mContext, lazyDaemon2); + + final ClientMonitor.Callback callback1 = mock(ClientMonitor.Callback.class); + final ClientMonitor.Callback callback2 = mock(ClientMonitor.Callback.class); + + // Pretend the scheduler is busy so the first operation doesn't start right away. We want + // to pretend like there are two operations in the queue before kicking things off + mScheduler.mCurrentOperation = new BiometricScheduler.Operation( + mock(ClientMonitor.class), mock(ClientMonitor.Callback.class)); + + mScheduler.scheduleClientMonitor(client1, callback1); + assertEquals(1, mScheduler.mPendingOperations.size()); + // client1 is pending. Allow the scheduler to start once second client is added. + mScheduler.mCurrentOperation = null; + mScheduler.scheduleClientMonitor(client2, callback2); + waitForIdle(); + + // Simulate that the BiometricPrompt client's sensor is ready + mScheduler.startPreparedClient(client1.getCookie()); + + assertTrue(client1.wasUnableToStart()); + verify(callback1).onClientFinished(eq(client1), eq(false) /* success */); + verify(callback1, never()).onClientStarted(any()); + + // Client 2 was able to start + assertFalse(client2.wasUnableToStart()); + assertTrue(client2.hasStarted()); + verify(callback2).onClientStarted(eq(client2)); + } + + private static class TestBiometricPromptClientMonitor extends TestClientMonitor { + public TestBiometricPromptClientMonitor(@NonNull Context context, + @NonNull LazyDaemon lazyDaemon) { + super(context, lazyDaemon, 1 /* cookie */); + } + } + private static class TestClientMonitor extends ClientMonitor { + private boolean mUnableToStart; + private boolean mStarted; public TestClientMonitor(@NonNull Context context, @NonNull LazyDaemon lazyDaemon) { super(context, lazyDaemon, null /* token */, null /* listener */, 0 /* userId */, @@ -67,14 +166,42 @@ public class BiometricSchedulerTest { 0 /* statsAction */, 0 /* statsClient */); } + public TestClientMonitor(@NonNull Context context, @NonNull LazyDaemon lazyDaemon, + int cookie) { + super(context, lazyDaemon, null /* token */, null /* listener */, 0 /* userId */, + TAG, cookie, 0 /* sensorId */, 0 /* statsModality */, + 0 /* statsAction */, 0 /* statsClient */); + } + + @Override public void unableToStart() { + assertFalse(mUnableToStart); + mUnableToStart = true; + } + @Override + public void start(@NonNull Callback callback) { + super.start(callback); + assertFalse(mStarted); + mStarted = true; } @Override protected void startHalOperation() { } + + public boolean wasUnableToStart() { + return mUnableToStart; + } + + public boolean hasStarted() { + return mStarted; + } + } + + private static void waitForIdle() { + InstrumentationRegistry.getInstrumentation().waitForIdleSync(); } }