From f6ea6733b06809740a3a0dc651335a1635f7ff63 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Tue, 20 Aug 2019 19:08:15 -0700 Subject: [PATCH] Address static cache access issues. ArrayMap and ArraySet are not thread safe, so callers are expected to lock around their instances before calling any methods. However, if a caller fails to lock, there can be races that end up corrupting the cache pool (despite having the class level lock held). A corrupted pool causes issues for other ArrayMap/Set instances, even if those instances are properly used under locks. There's no way to guard against cache pool corruption without making ArrayMap/Set fully thread-safe, changing the pool structure, or appropriately locking around each ArrayMap/Set instance. For now, we make ArraySet throw ConcurrentModificationException on a best effort basis to provide better awareness when an ArraySet is accessed concurrently without any locks and try to detect when the cache is corrupted. Bug: 62282384 Bug: 73549921 Bug: 122969275 Bug: 139015193 Bug: 139401479 Test: atest CtsUtilTestCases:ArrayMapTest Test: atest CtsUtilTestCases:ArraySetTest Test: atest FrameworksCoreTests:ArrayMapTest Test: atest FrameworksCoreTests:ArraySetTest Change-Id: Icd76630b25afd9f3627239301ffa5c37da25ea18 --- core/java/android/util/ArrayMap.java | 78 +++++++--- core/java/android/util/ArraySet.java | 146 ++++++++++++------ .../src/android/util/ArraySetTest.java | 113 ++++++++++++++ 3 files changed, 272 insertions(+), 65 deletions(-) create mode 100644 core/tests/coretests/src/android/util/ArraySetTest.java diff --git a/core/java/android/util/ArrayMap.java b/core/java/android/util/ArrayMap.java index d899dde9ef594..3f44e48d8efbd 100644 --- a/core/java/android/util/ArrayMap.java +++ b/core/java/android/util/ArrayMap.java @@ -48,6 +48,8 @@ import java.util.Set; * you have no control over this shrinking -- if you set a capacity and then remove an * item, it may reduce the capacity to better match the current size. In the future an * explicit call to set the capacity should turn off this aggressive shrinking behavior.

+ * + *

This structure is NOT thread-safe.

*/ public final class ArrayMap implements Map { private static final boolean DEBUG = false; @@ -103,15 +105,21 @@ public final class ArrayMap implements Map { static Object[] mTwiceBaseCache; @UnsupportedAppUsage(maxTargetSdk = 28) // Allocations are an implementation detail. static int mTwiceBaseCacheSize; + /** + * Separate locks for each cache since each can be accessed independently of the other without + * risk of a deadlock. + */ + private static final Object sBaseCacheLock = new Object(); + private static final Object sTwiceBaseCacheLock = new Object(); - final boolean mIdentityHashCode; + private final boolean mIdentityHashCode; @UnsupportedAppUsage(maxTargetSdk = 28) // Hashes are an implementation detail. Use public key/value API. int[] mHashes; @UnsupportedAppUsage(maxTargetSdk = 28) // Storage is an implementation detail. Use public key/value API. Object[] mArray; @UnsupportedAppUsage(maxTargetSdk = 28) // Use size() int mSize; - MapCollections mCollections; + private MapCollections mCollections; private static int binarySearchHashes(int[] hashes, int N, int hash) { try { @@ -209,31 +217,57 @@ public final class ArrayMap implements Map { throw new UnsupportedOperationException("ArrayMap is immutable"); } if (size == (BASE_SIZE*2)) { - synchronized (ArrayMap.class) { + synchronized (sTwiceBaseCacheLock) { if (mTwiceBaseCache != null) { final Object[] array = mTwiceBaseCache; mArray = array; - mTwiceBaseCache = (Object[])array[0]; - mHashes = (int[])array[1]; - array[0] = array[1] = null; - mTwiceBaseCacheSize--; - if (DEBUG) Log.d(TAG, "Retrieving 2x cache " + mHashes - + " now have " + mTwiceBaseCacheSize + " entries"); - return; + try { + mTwiceBaseCache = (Object[]) array[0]; + mHashes = (int[]) array[1]; + if (mHashes != null) { + array[0] = array[1] = null; + mTwiceBaseCacheSize--; + if (DEBUG) { + Log.d(TAG, "Retrieving 2x cache " + mHashes + + " now have " + mTwiceBaseCacheSize + " entries"); + } + return; + } + } catch (ClassCastException e) { + } + // Whoops! Someone trampled the array (probably due to not protecting + // their access with a lock). Our cache is corrupt; report and give up. + Slog.wtf(TAG, "Found corrupt ArrayMap cache: [0]=" + array[0] + + " [1]=" + array[1]); + mTwiceBaseCache = null; + mTwiceBaseCacheSize = 0; } } } else if (size == BASE_SIZE) { - synchronized (ArrayMap.class) { + synchronized (sBaseCacheLock) { if (mBaseCache != null) { final Object[] array = mBaseCache; mArray = array; - mBaseCache = (Object[])array[0]; - mHashes = (int[])array[1]; - array[0] = array[1] = null; - mBaseCacheSize--; - if (DEBUG) Log.d(TAG, "Retrieving 1x cache " + mHashes - + " now have " + mBaseCacheSize + " entries"); - return; + try { + mBaseCache = (Object[]) array[0]; + mHashes = (int[]) array[1]; + if (mHashes != null) { + array[0] = array[1] = null; + mBaseCacheSize--; + if (DEBUG) { + Log.d(TAG, "Retrieving 1x cache " + mHashes + + " now have " + mBaseCacheSize + " entries"); + } + return; + } + } catch (ClassCastException e) { + } + // Whoops! Someone trampled the array (probably due to not protecting + // their access with a lock). Our cache is corrupt; report and give up. + Slog.wtf(TAG, "Found corrupt ArrayMap cache: [0]=" + array[0] + + " [1]=" + array[1]); + mBaseCache = null; + mBaseCacheSize = 0; } } } @@ -242,10 +276,14 @@ public final class ArrayMap implements Map { mArray = new Object[size<<1]; } + /** + * Make sure NOT to call this method with arrays that can still be modified. In other + * words, don't pass mHashes or mArray in directly. + */ @UnsupportedAppUsage(maxTargetSdk = 28) // Allocations are an implementation detail. private static void freeArrays(final int[] hashes, final Object[] array, final int size) { if (hashes.length == (BASE_SIZE*2)) { - synchronized (ArrayMap.class) { + synchronized (sTwiceBaseCacheLock) { if (mTwiceBaseCacheSize < CACHE_SIZE) { array[0] = mTwiceBaseCache; array[1] = hashes; @@ -259,7 +297,7 @@ public final class ArrayMap implements Map { } } } else if (hashes.length == BASE_SIZE) { - synchronized (ArrayMap.class) { + synchronized (sBaseCacheLock) { if (mBaseCacheSize < CACHE_SIZE) { array[0] = mBaseCache; array[1] = hashes; diff --git a/core/java/android/util/ArraySet.java b/core/java/android/util/ArraySet.java index 3fa914f9ad02d..44c5af20d8913 100644 --- a/core/java/android/util/ArraySet.java +++ b/core/java/android/util/ArraySet.java @@ -23,6 +23,7 @@ import libcore.util.EmptyArray; import java.lang.reflect.Array; import java.util.Collection; +import java.util.ConcurrentModificationException; import java.util.Iterator; import java.util.Map; import java.util.Set; @@ -46,6 +47,8 @@ import java.util.function.Predicate; * you have no control over this shrinking -- if you set a capacity and then remove an * item, it may reduce the capacity to better match the current size. In the future an * explicit call to set the capacity should turn off this aggressive shrinking behavior.

+ * + *

This structure is NOT thread-safe.

*/ public final class ArraySet implements Collection, Set { private static final boolean DEBUG = false; @@ -72,15 +75,30 @@ public final class ArraySet implements Collection, Set { static int sBaseCacheSize; static Object[] sTwiceBaseCache; static int sTwiceBaseCacheSize; + /** + * Separate locks for each cache since each can be accessed independently of the other without + * risk of a deadlock. + */ + private static final Object sBaseCacheLock = new Object(); + private static final Object sTwiceBaseCacheLock = new Object(); - final boolean mIdentityHashCode; + private final boolean mIdentityHashCode; @UnsupportedAppUsage(maxTargetSdk = 28) // Hashes are an implementation detail. Use public API. int[] mHashes; @UnsupportedAppUsage(maxTargetSdk = 28) // Storage is an implementation detail. Use public API. Object[] mArray; @UnsupportedAppUsage(maxTargetSdk = 28) // Use size() int mSize; - MapCollections mCollections; + private MapCollections mCollections; + + private int binarySearch(int[] hashes, int hash) { + try { + return ContainerHelpers.binarySearch(hashes, mSize, hash); + } catch (ArrayIndexOutOfBoundsException e) { + throw new ConcurrentModificationException(); + } + } + @UnsupportedAppUsage(maxTargetSdk = 28) // Hashes are an implementation detail. Use indexOfKey(Object). private int indexOf(Object key, int hash) { @@ -91,7 +109,7 @@ public final class ArraySet implements Collection, Set { return ~0; } - int index = ContainerHelpers.binarySearch(mHashes, N, hash); + int index = binarySearch(mHashes, hash); // If the hash code wasn't found, then we have no entry for this key. if (index < 0) { @@ -130,7 +148,7 @@ public final class ArraySet implements Collection, Set { return ~0; } - int index = ContainerHelpers.binarySearch(mHashes, N, 0); + int index = binarySearch(mHashes, 0); // If the hash code wasn't found, then we have no entry for this key. if (index < 0) { @@ -163,20 +181,22 @@ public final class ArraySet implements Collection, Set { @UnsupportedAppUsage(maxTargetSdk = 28) // Allocations are an implementation detail. private void allocArrays(final int size) { if (size == (BASE_SIZE * 2)) { - synchronized (ArraySet.class) { + synchronized (sTwiceBaseCacheLock) { if (sTwiceBaseCache != null) { final Object[] array = sTwiceBaseCache; try { mArray = array; sTwiceBaseCache = (Object[]) array[0]; mHashes = (int[]) array[1]; - array[0] = array[1] = null; - sTwiceBaseCacheSize--; - if (DEBUG) { - Log.d(TAG, "Retrieving 2x cache " + mHashes + " now have " - + sTwiceBaseCacheSize + " entries"); - } - return; + if (mHashes != null) { + array[0] = array[1] = null; + sTwiceBaseCacheSize--; + if (DEBUG) { + Log.d(TAG, "Retrieving 2x cache " + mHashes + " now have " + + sTwiceBaseCacheSize + " entries"); + } + return; + } } catch (ClassCastException e) { } // Whoops! Someone trampled the array (probably due to not protecting @@ -188,20 +208,22 @@ public final class ArraySet implements Collection, Set { } } } else if (size == BASE_SIZE) { - synchronized (ArraySet.class) { + synchronized (sBaseCacheLock) { if (sBaseCache != null) { final Object[] array = sBaseCache; try { mArray = array; sBaseCache = (Object[]) array[0]; mHashes = (int[]) array[1]; - array[0] = array[1] = null; - sBaseCacheSize--; - if (DEBUG) { - Log.d(TAG, "Retrieving 1x cache " + mHashes + " now have " + sBaseCacheSize - + " entries"); + if (mHashes != null) { + array[0] = array[1] = null; + sBaseCacheSize--; + if (DEBUG) { + Log.d(TAG, "Retrieving 1x cache " + mHashes + " now have " + + sBaseCacheSize + " entries"); + } + return; } - return; } catch (ClassCastException e) { } // Whoops! Someone trampled the array (probably due to not protecting @@ -218,10 +240,14 @@ public final class ArraySet implements Collection, Set { mArray = new Object[size]; } + /** + * Make sure NOT to call this method with arrays that can still be modified. In other + * words, don't pass mHashes or mArray in directly. + */ @UnsupportedAppUsage(maxTargetSdk = 28) // Allocations are an implementation detail. private static void freeArrays(final int[] hashes, final Object[] array, final int size) { if (hashes.length == (BASE_SIZE * 2)) { - synchronized (ArraySet.class) { + synchronized (sTwiceBaseCacheLock) { if (sTwiceBaseCacheSize < CACHE_SIZE) { array[0] = sTwiceBaseCache; array[1] = hashes; @@ -237,7 +263,7 @@ public final class ArraySet implements Collection, Set { } } } else if (hashes.length == BASE_SIZE) { - synchronized (ArraySet.class) { + synchronized (sBaseCacheLock) { if (sBaseCacheSize < CACHE_SIZE) { array[0] = sBaseCache; array[1] = hashes; @@ -308,10 +334,16 @@ public final class ArraySet implements Collection, Set { @Override public void clear() { if (mSize != 0) { - freeArrays(mHashes, mArray, mSize); + final int[] ohashes = mHashes; + final Object[] oarray = mArray; + final int osize = mSize; mHashes = EmptyArray.INT; mArray = EmptyArray.OBJECT; mSize = 0; + freeArrays(ohashes, oarray, osize); + } + if (mSize != 0) { + throw new ConcurrentModificationException(); } } @@ -320,6 +352,7 @@ public final class ArraySet implements Collection, Set { * items. */ public void ensureCapacity(int minimumCapacity) { + final int oSize = mSize; if (mHashes.length < minimumCapacity) { final int[] ohashes = mHashes; final Object[] oarray = mArray; @@ -330,6 +363,9 @@ public final class ArraySet implements Collection, Set { } freeArrays(ohashes, oarray, mSize); } + if (mSize != oSize) { + throw new ConcurrentModificationException(); + } } /** @@ -400,11 +436,10 @@ public final class ArraySet implements Collection, Set { * * @param value the object to add. * @return {@code true} if this set is modified, {@code false} otherwise. - * @throws ClassCastException - * when the class of the object is inappropriate for this set. */ @Override public boolean add(E value) { + final int oSize = mSize; final int hash; int index; if (value == null) { @@ -419,9 +454,9 @@ public final class ArraySet implements Collection, Set { } index = ~index; - if (mSize >= mHashes.length) { - final int n = mSize >= (BASE_SIZE * 2) ? (mSize + (mSize >> 1)) - : (mSize >= BASE_SIZE ? (BASE_SIZE * 2) : BASE_SIZE); + if (oSize >= mHashes.length) { + final int n = oSize >= (BASE_SIZE * 2) ? (oSize + (oSize >> 1)) + : (oSize >= BASE_SIZE ? (BASE_SIZE * 2) : BASE_SIZE); if (DEBUG) Log.d(TAG, "add: grow from " + mHashes.length + " to " + n); @@ -429,21 +464,29 @@ public final class ArraySet implements Collection, Set { final Object[] oarray = mArray; allocArrays(n); + if (oSize != mSize) { + throw new ConcurrentModificationException(); + } + if (mHashes.length > 0) { - if (DEBUG) Log.d(TAG, "add: copy 0-" + mSize + " to 0"); + if (DEBUG) Log.d(TAG, "add: copy 0-" + oSize + " to 0"); System.arraycopy(ohashes, 0, mHashes, 0, ohashes.length); System.arraycopy(oarray, 0, mArray, 0, oarray.length); } - freeArrays(ohashes, oarray, mSize); + freeArrays(ohashes, oarray, oSize); } - if (index < mSize) { + if (index < oSize) { if (DEBUG) { - Log.d(TAG, "add: move " + index + "-" + (mSize - index) + " to " + (index + 1)); + Log.d(TAG, "add: move " + index + "-" + (oSize - index) + " to " + (index + 1)); } - System.arraycopy(mHashes, index, mHashes, index + 1, mSize - index); - System.arraycopy(mArray, index, mArray, index + 1, mSize - index); + System.arraycopy(mHashes, index, mHashes, index + 1, oSize - index); + System.arraycopy(mArray, index, mArray, index + 1, oSize - index); + } + + if (oSize != mSize || index >= mHashes.length) { + throw new ConcurrentModificationException(); } mHashes[index] = hash; @@ -458,6 +501,7 @@ public final class ArraySet implements Collection, Set { * @hide */ public void append(E value) { + final int oSize = mSize; final int index = mSize; final int hash = value == null ? 0 : (mIdentityHashCode ? System.identityHashCode(value) : value.hashCode()); @@ -476,6 +520,11 @@ public final class ArraySet implements Collection, Set { add(value); return; } + + if (oSize != mSize) { + throw new ConcurrentModificationException(); + } + mSize = index + 1; mHashes[index] = hash; mArray[index] = value; @@ -492,6 +541,9 @@ public final class ArraySet implements Collection, Set { if (N > 0) { System.arraycopy(array.mHashes, 0, mHashes, 0, N); System.arraycopy(array.mArray, 0, mArray, 0, N); + if (0 != mSize) { + throw new ConcurrentModificationException(); + } mSize = N; } } else { @@ -549,12 +601,14 @@ public final class ArraySet implements Collection, Set { // Check if exception should be thrown outside of the critical path. throw new ArrayIndexOutOfBoundsException(index); } + final int oSize = mSize; final Object old = mArray[index]; - if (mSize <= 1) { + if (oSize <= 1) { // Now empty. if (DEBUG) Log.d(TAG, "remove: shrink from " + mHashes.length + " to 0"); clear(); } else { + final int nSize = oSize - 1; if (shouldShrink()) { // Shrunk enough to reduce size of arrays. final int n = getNewShrunkenSize(); @@ -565,31 +619,33 @@ public final class ArraySet implements Collection, Set { final Object[] oarray = mArray; allocArrays(n); - mSize--; if (index > 0) { if (DEBUG) Log.d(TAG, "remove: copy from 0-" + index + " to 0"); System.arraycopy(ohashes, 0, mHashes, 0, index); System.arraycopy(oarray, 0, mArray, 0, index); } - if (index < mSize) { + if (index < nSize) { if (DEBUG) { - Log.d(TAG, "remove: copy from " + (index + 1) + "-" + mSize + Log.d(TAG, "remove: copy from " + (index + 1) + "-" + nSize + " to " + index); } - System.arraycopy(ohashes, index + 1, mHashes, index, mSize - index); - System.arraycopy(oarray, index + 1, mArray, index, mSize - index); + System.arraycopy(ohashes, index + 1, mHashes, index, nSize - index); + System.arraycopy(oarray, index + 1, mArray, index, nSize - index); } } else { - mSize--; - if (index < mSize) { + if (index < nSize) { if (DEBUG) { - Log.d(TAG, "remove: move " + (index + 1) + "-" + mSize + " to " + index); + Log.d(TAG, "remove: move " + (index + 1) + "-" + nSize + " to " + index); } - System.arraycopy(mHashes, index + 1, mHashes, index, mSize - index); - System.arraycopy(mArray, index + 1, mArray, index, mSize - index); + System.arraycopy(mHashes, index + 1, mHashes, index, nSize - index); + System.arraycopy(mArray, index + 1, mArray, index, nSize - index); } - mArray[mSize] = null; + mArray[nSize] = null; } + if (oSize != mSize) { + throw new ConcurrentModificationException(); + } + mSize = nSize; } return (E) old; } diff --git a/core/tests/coretests/src/android/util/ArraySetTest.java b/core/tests/coretests/src/android/util/ArraySetTest.java new file mode 100644 index 0000000000000..f1bebfb9bd933 --- /dev/null +++ b/core/tests/coretests/src/android/util/ArraySetTest.java @@ -0,0 +1,113 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.util; + +import androidx.test.filters.LargeTest; + +import junit.framework.TestCase; + +import org.junit.After; +import org.junit.Test; + +import java.util.ConcurrentModificationException; + +/** + * Unit tests for ArraySet that don't belong in CTS. + */ +@LargeTest +public class ArraySetTest extends TestCase { + private static final String TAG = "ArraySetTest"; + ArraySet mSet = new ArraySet<>(); + + @After + public void tearDown() { + mSet = null; + } + + /** + * Attempt to generate a ConcurrentModificationException in ArraySet. + *

+ * ArraySet is explicitly documented to be non-thread-safe, yet it's easy to accidentally screw + * this up; ArraySet should (in the spirit of the core Java collection types) make an effort to + * catch this and throw ConcurrentModificationException instead of crashing somewhere in its + * internals. + */ + @Test + public void testConcurrentModificationException() throws Exception { + final int testDurMs = 10_000; + System.out.println("Starting ArraySet concurrency test"); + new Thread(() -> { + int i = 0; + while (mSet != null) { + try { + mSet.add(String.format("key %d", i++)); + } catch (ArrayIndexOutOfBoundsException e) { + Log.e(TAG, "concurrent modification uncaught, causing indexing failure", e); + fail("Concurrent modification uncaught, causing indexing failure: " + e); + } catch (ClassCastException e) { + Log.e(TAG, "concurrent modification uncaught, causing cache corruption", e); + fail("Concurrent modification uncaught, causing cache corruption: " + e); + } catch (ConcurrentModificationException e) { + System.out.println("[successfully caught CME at put #" + i + + " size=" + (mSet == null ? "??" : String.valueOf(mSet.size())) + "]"); + if (i % 200 == 0) { + System.out.print("."); + } + } + } + }).start(); + for (int i = 0; i < (testDurMs / 100); i++) { + try { + if (mSet.size() % 4 == 0) { + mSet.clear(); + } + System.out.print("X"); + } catch (ArrayIndexOutOfBoundsException e) { + Log.e(TAG, "concurrent modification uncaught, causing indexing failure", e); + fail("Concurrent modification uncaught, causing indexing failure: " + e); + } catch (ClassCastException e) { + Log.e(TAG, "concurrent modification uncaught, causing cache corruption", e); + fail("Concurrent modification uncaught, causing cache corruption: " + e); + } catch (ConcurrentModificationException e) { + System.out.println( + "[successfully caught CME at clear #" + i + " size=" + mSet.size() + "]"); + } + } + } + + /** + * Check to make sure the same operations behave as expected in a single thread. + */ + @Test + public void testNonConcurrentAccesses() throws Exception { + for (int i = 0; i < 100000; i++) { + try { + mSet.add(String.format("key %d", i++)); + if (i % 200 == 0) { + System.out.print("."); + } + if (i % 500 == 0) { + mSet.clear(); + System.out.print("X"); + } + } catch (ConcurrentModificationException e) { + Log.e(TAG, "concurrent modification caught on single thread", e); + fail(); + } + } + } +}