From a04d57070d780ca5d93e9af93c25c5c6dd36ab3a Mon Sep 17 00:00:00 2001 From: Kevin Chyn Date: Thu, 5 Aug 2021 15:31:13 -0700 Subject: [PATCH] Fix KeyguardUpdateMonitor auth lifecycle issues This is split into two high-level issues which together cause strange auth behavior on keyguard: ============= Issue 1 ============= For fingerprint, auth ends when: 1) Success 2) Error For face, auth ends when: 1) Success 2) Reject 3) Error This change ensures that cancellation signal is set to null upon any of these conditions, so that there is never an opportunity of using a stale CancellationSignal. Furthermore, do not invoke stale cancellation signal when starting authentication. In the off chance the bug is re-introduced, or if some other situation can cause a cancellation signal to be non-null when keyguard requests auth again (e.g. bad state management in keyguard), do NOT invoke the stale cancellation signal's cancel method. The framework already handles this case gracefully and will automatically cancel the previous operation. ============= Issue 2 ============= We have a runnable that's scheduled to run after X ms if ERROR_CANCELED is not received after cancel() is requested. However, there are various bugs around that logic: 1) shared runnable for both fp and face, leading to unexpected and incorrect state changes (e.g. face does not respond to cancel within X ms, both fp and face will go to STATE_STOPPED, even though cancel() was never requested of fp 2) Always remove and re-add runnable when requesting cancel() though it should never occur that cancel() is requested in close temporal proximity, it never hurts to have the correct timeout before resetting the state. Bug: 195365422 Bug: 193477749 Test: manual Change-Id: I3702f41c8af7e870798f19c43012a26281a6632a --- .../keyguard/KeyguardUpdateMonitor.java | 75 +++++++++++-------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/packages/SystemUI/src/com/android/keyguard/KeyguardUpdateMonitor.java b/packages/SystemUI/src/com/android/keyguard/KeyguardUpdateMonitor.java index 9bfbe325fc034..0bb5b1ce51791 100644 --- a/packages/SystemUI/src/com/android/keyguard/KeyguardUpdateMonitor.java +++ b/packages/SystemUI/src/com/android/keyguard/KeyguardUpdateMonitor.java @@ -347,13 +347,16 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab private static final int HAL_ERROR_RETRY_TIMEOUT = 500; // ms private static final int HAL_ERROR_RETRY_MAX = 20; - private final Runnable mCancelNotReceived = new Runnable() { - @Override - public void run() { - Log.w(TAG, "Cancel not received, transitioning to STOPPED"); - mFingerprintRunningState = mFaceRunningState = BIOMETRIC_STATE_STOPPED; - updateBiometricListeningState(); - } + private final Runnable mFpCancelNotReceived = () -> { + Log.e(TAG, "Fp cancellation not received, transitioning to STOPPED"); + mFingerprintRunningState = BIOMETRIC_STATE_STOPPED; + updateFingerprintListeningState(); + }; + + private final Runnable mFaceCancelNotReceived = () -> { + Log.e(TAG, "Face cancellation not received, transitioning to STOPPED"); + mFaceRunningState = BIOMETRIC_STATE_STOPPED; + updateFaceListeningState(); }; private final Handler mHandler; @@ -791,19 +794,19 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab private void handleFingerprintError(int msgId, String errString) { Assert.isMainThread(); - if (msgId == FingerprintManager.FINGERPRINT_ERROR_CANCELED && mHandler.hasCallbacks( - mCancelNotReceived)) { - mHandler.removeCallbacks(mCancelNotReceived); + if (mHandler.hasCallbacks(mFpCancelNotReceived)) { + mHandler.removeCallbacks(mFpCancelNotReceived); } + // Error is always the end of authentication lifecycle. + mFingerprintCancelSignal = null; + if (msgId == FingerprintManager.FINGERPRINT_ERROR_CANCELED && mFingerprintRunningState == BIOMETRIC_STATE_CANCELLING_RESTARTING) { setFingerprintRunningState(BIOMETRIC_STATE_STOPPED); updateFingerprintListeningState(); } else { setFingerprintRunningState(BIOMETRIC_STATE_STOPPED); - mFingerprintCancelSignal = null; - mFaceCancelSignal = null; } if (msgId == FingerprintManager.FINGERPRINT_ERROR_HW_UNAVAILABLE) { @@ -905,6 +908,7 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab private void handleFaceAuthFailed() { Assert.isMainThread(); + mFaceCancelSignal = null; setFaceRunningState(BIOMETRIC_STATE_STOPPED); for (int i = 0; i < mCallbacks.size(); i++) { KeyguardUpdateMonitorCallback cb = mCallbacks.get(i).get(); @@ -983,10 +987,13 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab private void handleFaceError(int msgId, String errString) { Assert.isMainThread(); if (DEBUG_FACE) Log.d(TAG, "Face error received: " + errString); - if (msgId == FaceManager.FACE_ERROR_CANCELED && mHandler.hasCallbacks(mCancelNotReceived)) { - mHandler.removeCallbacks(mCancelNotReceived); + if (mHandler.hasCallbacks(mFaceCancelNotReceived)) { + mHandler.removeCallbacks(mFaceCancelNotReceived); } + // Error is always the end of authentication lifecycle + mFaceCancelSignal = null; + if (msgId == FaceManager.FACE_ERROR_CANCELED && mFaceRunningState == BIOMETRIC_STATE_CANCELLING_RESTARTING) { setFaceRunningState(BIOMETRIC_STATE_STOPPED); @@ -2368,6 +2375,14 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab } private void startListeningForFingerprint() { + final int userId = getCurrentUser(); + final boolean unlockPossible = isUnlockWithFingerprintPossible(userId); + if (mFingerprintCancelSignal != null) { + Log.e(TAG, "Cancellation signal is not null, high chance of bug in fp auth lifecycle" + + " management. FP state: " + mFingerprintRunningState + + ", unlockPossible: " + unlockPossible); + } + if (mFingerprintRunningState == BIOMETRIC_STATE_CANCELLING) { setFingerprintRunningState(BIOMETRIC_STATE_CANCELLING_RESTARTING); return; @@ -2377,11 +2392,8 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab return; } if (DEBUG) Log.v(TAG, "startListeningForFingerprint()"); - int userId = getCurrentUser(); - if (isUnlockWithFingerprintPossible(userId)) { - if (mFingerprintCancelSignal != null) { - mFingerprintCancelSignal.cancel(); - } + + if (unlockPossible) { mFingerprintCancelSignal = new CancellationSignal(); if (isEncryptedOrLockdown(userId)) { @@ -2397,6 +2409,14 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab } private void startListeningForFace() { + final int userId = getCurrentUser(); + final boolean unlockPossible = isUnlockWithFacePossible(userId); + if (mFaceCancelSignal != null) { + Log.e(TAG, "Cancellation signal is not null, high chance of bug in face auth lifecycle" + + " management. Face state: " + mFaceRunningState + + ", unlockPossible: " + unlockPossible); + } + if (mFaceRunningState == BIOMETRIC_STATE_CANCELLING) { setFaceRunningState(BIOMETRIC_STATE_CANCELLING_RESTARTING); return; @@ -2405,11 +2425,8 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab return; } if (DEBUG) Log.v(TAG, "startListeningForFace(): " + mFaceRunningState); - int userId = getCurrentUser(); - if (isUnlockWithFacePossible(userId)) { - if (mFaceCancelSignal != null) { - mFaceCancelSignal.cancel(); - } + + if (unlockPossible) { mFaceCancelSignal = new CancellationSignal(); // This would need to be updated for multi-sensor devices @@ -2461,9 +2478,8 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab if (mFingerprintCancelSignal != null) { mFingerprintCancelSignal.cancel(); mFingerprintCancelSignal = null; - if (!mHandler.hasCallbacks(mCancelNotReceived)) { - mHandler.postDelayed(mCancelNotReceived, DEFAULT_CANCEL_SIGNAL_TIMEOUT); - } + mHandler.removeCallbacks(mFpCancelNotReceived); + mHandler.postDelayed(mFpCancelNotReceived, DEFAULT_CANCEL_SIGNAL_TIMEOUT); } setFingerprintRunningState(BIOMETRIC_STATE_CANCELLING); } @@ -2478,9 +2494,8 @@ public class KeyguardUpdateMonitor implements TrustManager.TrustListener, Dumpab if (mFaceCancelSignal != null) { mFaceCancelSignal.cancel(); mFaceCancelSignal = null; - if (!mHandler.hasCallbacks(mCancelNotReceived)) { - mHandler.postDelayed(mCancelNotReceived, DEFAULT_CANCEL_SIGNAL_TIMEOUT); - } + mHandler.removeCallbacks(mFaceCancelNotReceived); + mHandler.postDelayed(mFaceCancelNotReceived, DEFAULT_CANCEL_SIGNAL_TIMEOUT); } setFaceRunningState(BIOMETRIC_STATE_CANCELLING); }