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

Commit 3171dbf9 authored by Eric Biggers's avatar Eric Biggers
Browse files

LockSettingsStorage: fix user prefetching 

The user prefetching logic doesn't work as intended, for two reasons:

- The key that Cache.setFetched() sets differs from the key that
  Cache.isFetched() looks for, so Cache.isFetched() always returns
  false.  So, LockSettingsStorage.prefetchUser() always runs for a user
  when called, rather than just once per boot as intended.  Fix this by
  making Cache.setFetched() use the same key as Cache.isFetched().

- Since Cache.putIfUnchanged() increments Cache.mVersion, only the first
  call to it within LockSettingsStorage.prefetchUser() has any effect.
  So only the first key-value pair for the user is prefetched, not all
  of them as intended.  Fix this by making Cache.putIfUnchanged() *not*
  increment mVersion, as the backing storage isn't being modified.

Found by code review; these bugs aren't known to have been causing any
real-world problems.

Test: atest --iterations 50 LockSettingsStorageTests
Change-Id: Ife97ba855d014d8e291d46f6a702a41b28bd5707
parent 3329ea3d

services/core/java/com/android/server/locksettings/LockSettingsStorage.java

+32 −15
Original line number Diff line number Diff line
@@ -201,6 +201,16 @@ class LockSettingsStorage extends WatchableImpl { 
        return result == DEFAULT ? defaultValue : (String) result;

    }



    @VisibleForTesting

    boolean isKeyValueCached(String key, int userId) {

        return mCache.hasKeyValue(key, userId);

    }



    @VisibleForTesting

    boolean isUserPrefetched(int userId) {

        return mCache.isFetched(userId);

    }



    @VisibleForTesting

    public void removeKey(String key, int userId) {

        removeKey(mOpenHelper.getWritableDatabase(), key, userId);
@@ -762,19 +772,24 @@ class LockSettingsStorage extends WatchableImpl { 
        }

    }



    /**

     * Cache consistency model:

     * - Writes to storage write directly to the cache, but this MUST happen within the atomic

     *   section either provided by the database transaction or mWriteLock, such that writes to the

     *   cache and writes to the backing storage are guaranteed to occur in the same order

    /*

     * A cache for the following types of data:

     *

     * - Reads can populate the cache, but because they are no strong ordering guarantees with

     *   respect to writes this precaution is taken:

     *   - The cache is assigned a version number that increases every time the cache is modified.

     *     Reads from backing storage can only populate the cache if the backing storage

     *     has not changed since the load operation has begun.

     *     This guarantees that no read operation can shadow a write to the cache that happens

     *     after it had begun.

     *  - Key-value entries from the locksettings database, where the key is the combination of a

     *    userId and a string key, and the value is a string.

     *  - File paths to file contents.

     *  - The per-user "prefetched" flag.

     *

     * Cache consistency model:

     *  - Writes to storage write directly to the cache, but this MUST happen within an atomic

     *    section either provided by the database transaction or mFileWriteLock, such that writes to

     *    the cache and writes to the backing storage are guaranteed to occur in the same order.

     *  - Reads can populate the cache, but because there are no strong ordering guarantees with

     *    respect to writes the following precaution is taken: The cache is assigned a version

     *    number that increases every time the backing storage is modified. Reads from backing

     *    storage can only populate the cache if the backing storage has not changed since the load

     *    operation has begun. This guarantees that a read operation can't clobber a different value

     *    that was written to the cache by a concurrent write operation.

     */

    private static class Cache {

        private final ArrayMap<CacheKey, Object> mCache = new ArrayMap<>();
@@ -819,7 +834,7 @@ class LockSettingsStorage extends WatchableImpl { 
        }



        void setFetched(int userId) {

            put(CacheKey.TYPE_FETCHED, "isFetched", "true", userId);

            put(CacheKey.TYPE_FETCHED, "", "true", userId);

        }



        boolean isFetched(int userId) {
@@ -831,7 +846,7 @@ class LockSettingsStorage extends WatchableImpl { 
        }



        private synchronized void put(int type, String key, Object value, int userId) {

            // Create a new CachKey here because it may be saved in the map if the key is absent.

            // Create a new CacheKey here because it may be saved in the map if the key is absent.

            mCache.put(new CacheKey().set(type, key, userId), value);

            mVersion++;

        }
@@ -839,7 +854,9 @@ class LockSettingsStorage extends WatchableImpl { 
        private synchronized void putIfUnchanged(int type, String key, Object value, int userId,

                int version) {

            if (!contains(type, key, userId) && mVersion == version) {

                put(type, key, value, userId);

                mCache.put(new CacheKey().set(type, key, userId), value);

                // Don't increment mVersion, as this method should only be called in cases where the

                // backing storage isn't being modified.

            }

        }

services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java

+12 −2
Original line number Diff line number Diff line
@@ -232,12 +232,22 @@ public class LockSettingsStorageTests { 


    @Test

    public void testPrefetch() {

        mStorage.writeKeyValue("key", "toBeFetched", 0);

        mStorage.writeKeyValue("key1", "value1", 0);

        mStorage.writeKeyValue("key2", "value2", 0);



        mStorage.clearCache();



        assertFalse(mStorage.isUserPrefetched(0));

        assertFalse(mStorage.isKeyValueCached("key1", 0));

        assertFalse(mStorage.isKeyValueCached("key2", 0));



        mStorage.prefetchUser(0);



        assertEquals("toBeFetched", mStorage.readKeyValue("key", "default", 0));

        assertTrue(mStorage.isUserPrefetched(0));

        assertTrue(mStorage.isKeyValueCached("key1", 0));

        assertTrue(mStorage.isKeyValueCached("key2", 0));

        assertEquals("value1", mStorage.readKeyValue("key1", "default", 0));

        assertEquals("value2", mStorage.readKeyValue("key2", "default", 0));

    }



    @Test