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

Commit ce9ecd5f authored by Sherif Eid's avatar Sherif Eid
Browse files

Remove AdbDebuggingHandler dependency from AdbKeyStore 

The dependency to request keystore persistance from AdbDebuggingHandler
is problematic. If AdbKeyStore gets eagerly initialized before
AdbDebuggingHandler and AdbKeyStore requests persistance during its
construction, then sending persist messages happens on a null instance
of AdbDebuggingHandler.

The issue isn't visible now because AdbKeyStore construction happens
lazily after AdbDebuggingHandler is fully initialized. Relying on that
lazy mechanism can be problematic if in the future we try to initialize
AdbKeyStore in a different code path where AdbDebuggingHandler isn't
fully initialized.

Test: AdbDebuggingManagerTest
Test lunch sdk_gphone64_x86_64-trunk_staging-userdebug && m && ./prebuilts/android-emulator/linux-x86_64/emulator -wipe-data -show-kernel
Bug: 420613813
Flag: EXEMPT bugfix

Change-Id: If4ce8c36eef6979a0f9e82684d93a0b1e8c95fa1
parent 55aff04e

services/core/java/com/android/server/adb/AdbDebuggingManager.java

+4 −26
Original line number Diff line number Diff line
@@ -571,7 +571,9 @@ public class AdbDebuggingManager { 
        static final String MSG_START_ADB_WIFI = "W1";

        static final String MSG_STOP_ADB_WIFI = "W0";



        @Nullable @VisibleForTesting AdbKeyStore mAdbKeyStore;

        @NonNull @VisibleForTesting

        final AdbKeyStore mAdbKeyStore =

                new AdbKeyStore(mContext, mTempKeysFile, mUserKeyFile, mTicker);



        private final AdbDebuggingThread mThread;
@@ -605,20 +607,6 @@ public class AdbDebuggingManager { 
            }

        }



        /** Initialize the AdbKeyStore so tests can grab mAdbKeyStore immediately. */

        @VisibleForTesting

        void initKeyStore() {

            if (mAdbKeyStore == null) {

                mAdbKeyStore =

                        new AdbKeyStore(

                                mContext,

                                mTempKeysFile,

                                mUserKeyFile,

                                mTicker,

                                () -> sendPersistKeyStoreMessage());

            }

        }



        // Show when at least one device is connected.

        public void showAdbConnectedNotification(boolean show) {

            final int id = SystemMessage.NOTE_ADB_WIFI_ACTIVE;
@@ -666,7 +654,6 @@ public class AdbDebuggingManager { 
        }



        public void handleMessage(Message msg) {

            initKeyStore();



            switch (msg.what) {

                case MESSAGE_ADB_ENABLED -> {
@@ -727,9 +714,6 @@ public class AdbDebuggingManager { 
                case MESSAGE_ADB_CLEAR -> {

                    Slog.d(TAG, "Received a request to clear the adb authorizations");

                    mConnectedKeys.clear();

                    // If the key store has not yet been instantiated then do so now; this avoids

                    // the unnecessary creation of the key store when adb is not enabled.

                    initKeyStore();

                    mWifiConnectedKeys.clear();

                    mAdbKeyStore.deleteKeyStore();

                    cancelJobToUpdateAdbKeyStore();
@@ -1453,13 +1437,7 @@ public class AdbDebuggingManager { 


    /** Returns the list of paired devices. */

    public Map<String, PairDevice> getPairedDevices() {

        AdbKeyStore keystore =

                new AdbKeyStore(

                        mContext,

                        mTempKeysFile,

                        mUserKeyFile,

                        mTicker,

                        () -> sendPersistKeyStoreMessage());

        AdbKeyStore keystore = new AdbKeyStore(mContext, mTempKeysFile, mUserKeyFile, mTicker);

        return getPairedDevicesForKeys(keystore.getKeys());

    }

services/core/java/com/android/server/adb/AdbKeyStore.java

+5 −21
Original line number Diff line number Diff line
@@ -69,14 +69,6 @@ class AdbKeyStore { 
    private final Context mContext;

    private final AdbDebuggingManager.Ticker mTicker;



    /**

     * Callback to request that the AdbKeyStore's state be persisted to disk. Schedules a

     * MESSAGE_ADB_PERSIST_KEYSTORE message in the AdbDebuggingHandler to ensure that AdbKeyStore is

     * persisted to disk asynchronously.

     */

    // TODO: verify if we can persist keystore synchronously without scheduling messages on handler.

    private final KeyStorePersistAction mKeyStorePersistAction;



    private static final String SYSTEM_KEY_FILE = "/adb_keys";



    /**
@@ -98,11 +90,9 @@ class AdbKeyStore { 
            Context context,

            File tempKeysFile,

            File userKeyFile,

            AdbDebuggingManager.Ticker ticker,

            KeyStorePersistAction keyStorePersisAction) {

            AdbDebuggingManager.Ticker ticker) {

        mContext = context;

        mTicker = ticker;

        mKeyStorePersistAction = keyStorePersisAction;



        mAdbKeyUser = new AdbdKeyStoreStorage(userKeyFile);

        mAuthStore = new AdbAuthorizationStore(tempKeysFile);
@@ -123,7 +113,7 @@ class AdbKeyStore { 


    synchronized void addTrustedNetwork(String bssid) {

        mAuthEntries.trustedNetworks().add(bssid);

        mKeyStorePersistAction.requestPersist();

        persistKeyStore();

    }



    synchronized Set<String> getKeys() {
@@ -143,7 +133,7 @@ class AdbKeyStore { 
    synchronized void removeKey(String key) {

        if (mAuthEntries.keys().containsKey(key)) {

            mAuthEntries.keys().remove(key);

            mKeyStorePersistAction.requestPersist();

            persistKeyStore();

        }

    }
@@ -158,7 +148,7 @@ class AdbKeyStore { 
     */

    synchronized void updateKeyStore() {

        if (filterOutOldKeys()) {

            mKeyStorePersistAction.requestPersist();

            persistKeyStore();

        }

    }
@@ -179,7 +169,7 @@ class AdbKeyStore { 
            }

        }

        if (mapUpdated) {

            mKeyStorePersistAction.requestPersist();

            persistKeyStore();

        }

    }
@@ -330,10 +320,4 @@ class AdbKeyStore { 
    synchronized boolean isTrustedNetwork(String bssid) {

        return mAuthEntries.trustedNetworks().contains(bssid);

    }



    @FunctionalInterface

    interface KeyStorePersistAction {

        /** Requests that the AdbKeyStore's state be persisted. */

        void requestPersist();

    }

}

services/tests/servicestests/src/com/android/server/adb/AdbDebuggingManagerTest.java

+5 −18
Original line number Diff line number Diff line
@@ -36,8 +36,6 @@ import android.util.Log; 


import androidx.test.InstrumentationRegistry;



import com.android.server.adb.AdbDebuggingManager.AdbDebuggingHandler;



import org.junit.After;

import org.junit.Before;

import org.junit.Test;
@@ -118,7 +116,6 @@ public final class AdbDebuggingManagerTest { 
        mHandler = mManager.mHandler;

        mThread.setHandler(mHandler);



        mHandler.initKeyStore();

        mKeyStore = mHandler.mAdbKeyStore;



        mOriginalAllowedConnectionTime = mKeyStore.getAllowedConnectionTime();
@@ -283,9 +280,7 @@ public final class AdbDebuggingManagerTest { 
                mContext,

                mAdbKeyXmlFile,

                mAdbKeyFile,

                mFakeTicker,

                () -> mHandler.obtainMessage(AdbDebuggingHandler.MESSAGE_ADB_PERSIST_KEYSTORE)

                        .sendToTarget());

                mFakeTicker);

        assertTrue(

                "The key with the 'Always allow' option selected was not persisted in the keystore",

                newKeyStore.isKeyAuthorized(TEST_KEY_1));
@@ -306,9 +301,7 @@ public final class AdbDebuggingManagerTest { 
                mContext,

                mAdbKeyXmlFile,

                mAdbKeyFile,

                mFakeTicker,

                () -> mHandler.obtainMessage(AdbDebuggingHandler.MESSAGE_ADB_PERSIST_KEYSTORE)

                        .sendToTarget());

                mFakeTicker);

        assertNotEquals(

                "The last connection time in the key file was not updated after the update "

                        + "connection time message", lastConnectionTime,
@@ -661,9 +654,7 @@ public final class AdbDebuggingManagerTest { 
                mContext,

                mAdbKeyXmlFile,

                mAdbKeyFile,

                mFakeTicker,

                () -> mHandler.obtainMessage(AdbDebuggingHandler.MESSAGE_ADB_PERSIST_KEYSTORE)

                        .sendToTarget());

                mFakeTicker);



        // Verify that the connection time for each test key is within a small value of the current

        // time.
@@ -788,9 +779,7 @@ public final class AdbDebuggingManagerTest { 
                mContext,

                mAdbKeyXmlFile,

                mAdbKeyFile,

                mFakeTicker,

                () -> mHandler.obtainMessage(AdbDebuggingHandler.MESSAGE_ADB_PERSIST_KEYSTORE)

                        .sendToTarget());

                mFakeTicker);



        assertEquals(

                "KeyStore not populated from the XML file.",
@@ -857,9 +846,7 @@ public final class AdbDebuggingManagerTest { 
                mContext,

                mAdbKeyXmlFile,

                mAdbKeyFile,

                mFakeTicker,

                () -> mHandler.obtainMessage(AdbDebuggingHandler.MESSAGE_ADB_PERSIST_KEYSTORE)

                        .sendToTarget());

                mFakeTicker);



        assertTrue(

                "Persisted trusted network not found in new keystore instance.",