Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a04d5707 authored by Kevin Chyn's avatar Kevin Chyn
Browse files

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
parent 286530b6

packages/SystemUI/src/com/android/keyguard/KeyguardUpdateMonitor.java

+45 −30
Original line number Diff line number Diff line
@@ -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);

        }