From 1826b3e94d11da55e6c6984565dcc59dd2c2c400 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 24 Jun 2022 17:31:51 +0000 Subject: [PATCH] LockSettingsStorage: fix concurrent read clobbering a removal Since Cache.remove() doesn't increment mVersion, the key removal can be clobbered by a concurrent read that loads the key into the cache. Fix this by making Cache.remove() increment mVersion, indicating that the backing storage has been modified. Found by code review; this bug isn't known to have been causing any real-world problems. Add a best-effort test for this, though due to the very tight race needed, it didn't reproduce the bug in 2000 iterations. However, it reproduced the bug in just a few iterations if a 1 millisecond sleep is added in the right place (just before the call to Cache.putKeyValueIfUnchanged() in LockSettingsStorage.readKeyValue()). Test: atest --iterations 50 LockSettingsStorageTests Change-Id: I960cc386ecd040af7517d3d62a3eefe57a518620 --- .../locksettings/LockSettingsStorage.java | 1 + .../LockSettingsStorageTests.java | 56 ++++++++++++++++++- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index a77cc5d8d5fdf..8161503652ffd 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -843,6 +843,7 @@ class LockSettingsStorage extends WatchableImpl { private synchronized void remove(int type, String key, int userId) { mCache.remove(mCacheKey.set(type, key, userId)); + mVersion++; } private synchronized void put(int type, String key, Object value, int userId) { diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java index 15bbb10c6a1e5..c4df05149d4ba 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java @@ -19,6 +19,7 @@ package com.android.server.locksettings; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -60,7 +61,10 @@ import java.io.File; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.atomic.AtomicReference; /** * atest FrameworksServicesTests:LockSettingsStorageTests @@ -137,12 +141,12 @@ public class LockSettingsStorageTests { } @Test - public void testKeyValue_Concurrency() { + public void testKeyValue_ReadWriteConcurrency() { final CountDownLatch latch = new CountDownLatch(1); List threads = new ArrayList<>(); for (int i = 0; i < 100; i++) { final int threadId = i; - threads.add(new Thread("testKeyValue_Concurrency_" + i) { + threads.add(new Thread("testKeyValue_ReadWriteConcurrency_" + i) { @Override public void run() { try { @@ -164,7 +168,7 @@ public class LockSettingsStorageTests { }); threads.get(i).start(); } - mStorage.writeKeyValue("key", "initalValue", 0); + mStorage.writeKeyValue("key", "initialValue", 0); latch.countDown(); joinAll(threads, 10000); assertEquals('5', mStorage.readKeyValue("key", "default", 0).charAt(0)); @@ -172,6 +176,52 @@ public class LockSettingsStorageTests { assertEquals('5', mStorage.readKeyValue("key", "default", 0).charAt(0)); } + // Test that readKeyValue() doesn't pollute the cache when run concurrently with removeKey(). + @Test + @SuppressWarnings("AssertionFailureIgnored") // intentional try-catch of AssertionError + public void testKeyValue_ReadRemoveConcurrency() { + final int numThreads = 2; + final int numIterations = 50; + final CyclicBarrier barrier = new CyclicBarrier(numThreads); + final List threads = new ArrayList<>(); + final AtomicReference failure = new AtomicReference<>(); + for (int threadId = 0; threadId < numThreads; threadId++) { + final boolean isWriter = (threadId == 0); + threads.add(new Thread("testKeyValue_ReadRemoveConcurrency_" + threadId) { + @Override + public void run() { + try { + for (int iter = 0; iter < numIterations; iter++) { + if (isWriter) { + mStorage.writeKeyValue("key", "value", 0); + mStorage.clearCache(); + } + barrier.await(); + if (isWriter) { + mStorage.removeKey("key", 0); + } else { + mStorage.readKeyValue("key", "default", 0); + } + barrier.await(); + try { + assertEquals("default", mStorage.readKeyValue("key", "default", 0)); + } catch (AssertionError e) { + failure.compareAndSet(null, e); + } + barrier.await(); + } + } catch (InterruptedException | BrokenBarrierException e) { + failure.compareAndSet(null, e); + return; + } + } + }); + threads.get(threadId).start(); + } + joinAll(threads, 60000); + assertNull(failure.get()); + } + @Test public void testKeyValue_CacheStarvedWriter() { final CountDownLatch latch = new CountDownLatch(1);