From 04df738bcb6584dd82b731a67f4cf8d6925b061e Mon Sep 17 00:00:00 2001 From: Svetoslav Ganov Date: Tue, 10 May 2016 18:55:47 -0700 Subject: [PATCH] Make settings cahches generation mechanism robust. Settings is using a MemoryIntArray to communicate the settings table version enabling apps to have up-to-date local caches. However, ashmem allows an arbitrary process with a handle to the fd (even in read only mode) to unpin the memory which can then be garbage collected. Here we make this mechanism fault tolerant against bad apps unpinning the ashmem region. First, we no longer unpin the ashmem on the client side and if the ashmem region is purged and cannot be pinned we recreate it and hook up again with the local app caches. The change also adds a test that clients can only read while owner can read/write. bug:28764789 Change-Id: I1ef79b4b21e976124b268c9126a55d614157059b --- core/java/android/provider/Settings.java | 34 +++- core/java/android/util/MemoryIntArray.java | 12 +- core/jni/android_util_MemoryIntArray.cpp | 41 +---- core/tests/utiltests/Android.mk | 1 + core/tests/utiltests/AndroidManifest.xml | 5 + .../android/util/IRemoteMemoryIntArray.aidl | 30 +++ .../src/android/util/MemoryIntArrayTest.java | 45 ++++- .../src/android/util/RemoteIntArray.java | 165 +++++++++++++++++ .../util/RemoteMemoryIntArrayService.java | 114 ++++++++++++ .../settings/GenerationRegistry.java | 172 ++++++++++-------- 10 files changed, 502 insertions(+), 117 deletions(-) create mode 100644 core/tests/utiltests/src/android/util/IRemoteMemoryIntArray.aidl create mode 100644 core/tests/utiltests/src/android/util/RemoteIntArray.java create mode 100644 core/tests/utiltests/src/android/util/RemoteMemoryIntArrayService.java diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java index 370cd5722347c..a4c0d6775a2f7 100755 --- a/core/java/android/provider/Settings.java +++ b/core/java/android/provider/Settings.java @@ -1462,12 +1462,15 @@ public final class Settings { private static final class GenerationTracker { private final MemoryIntArray mArray; + private final Runnable mErrorHandler; private final int mIndex; private int mCurrentGeneration; - public GenerationTracker(@NonNull MemoryIntArray array, int index) { + public GenerationTracker(@NonNull MemoryIntArray array, int index, + Runnable errorHandler) { mArray = array; mIndex = index; + mErrorHandler = errorHandler; mCurrentGeneration = readCurrentGeneration(); } @@ -1487,9 +1490,23 @@ public final class Settings { return mArray.get(mIndex); } catch (IOException e) { Log.e(TAG, "Error getting current generation", e); + if (mErrorHandler != null) { + mErrorHandler.run(); + } } return -1; } + + public void destroy() { + try { + mArray.close(); + } catch (IOException e) { + Log.e(TAG, "Error closing backing array", e); + if (mErrorHandler != null) { + mErrorHandler.run(); + } + } + } } // Thread-safe. @@ -1616,7 +1633,20 @@ public final class Settings { + cr.getPackageName() + " and user:" + userHandle + " with index:" + index); } - mGenerationTracker = new GenerationTracker(array, index); + mGenerationTracker = new GenerationTracker(array, index, + () -> { + synchronized (this) { + Log.e(TAG, "Error accessing generation" + + " tracker - removing"); + if (mGenerationTracker != null) { + GenerationTracker generationTracker = + mGenerationTracker; + mGenerationTracker = null; + generationTracker.destroy(); + mValues.clear(); + } + } + }); } } mValues.put(name, value); diff --git a/core/java/android/util/MemoryIntArray.java b/core/java/android/util/MemoryIntArray.java index c3bd963708d7f..8f9b36e84ca27 100644 --- a/core/java/android/util/MemoryIntArray.java +++ b/core/java/android/util/MemoryIntArray.java @@ -20,6 +20,7 @@ import android.os.Parcel; import android.os.ParcelFileDescriptor; import android.os.Parcelable; import android.os.Process; +import libcore.io.IoUtils; import java.io.Closeable; import java.io.IOException; @@ -46,6 +47,8 @@ import java.util.UUID; * @hide */ public final class MemoryIntArray implements Parcelable, Closeable { + private static final String TAG = "MemoryIntArray"; + private static final int MAX_SIZE = 1024; private final int mOwnerPid; @@ -142,8 +145,9 @@ public final class MemoryIntArray implements Parcelable, Closeable { @Override public void close() throws IOException { if (!isClosed()) { - nativeClose(mFd.getFd(), mMemoryAddr, isOwner()); + ParcelFileDescriptor pfd = mFd; mFd = null; + nativeClose(pfd.getFd(), mMemoryAddr, isOwner()); } } @@ -156,7 +160,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { @Override protected void finalize() throws Throwable { - close(); + IoUtils.closeQuietly(this); super.finalize(); } @@ -230,7 +234,6 @@ public final class MemoryIntArray implements Parcelable, Closeable { private native int nativeGet(int fd, long memoryAddr, int index, boolean owner); private native void nativeSet(int fd, long memoryAddr, int index, int value, boolean owner); private native int nativeSize(int fd); - private native static int nativeGetMemoryPageSize(); /** * @return The max array size. @@ -246,7 +249,8 @@ public final class MemoryIntArray implements Parcelable, Closeable { try { return new MemoryIntArray(parcel); } catch (IOException ioe) { - throw new RuntimeException(ioe); + Log.e(TAG, "Error unparceling MemoryIntArray"); + return null; } } diff --git a/core/jni/android_util_MemoryIntArray.cpp b/core/jni/android_util_MemoryIntArray.cpp index dbe8ed3e26d00..f45be127c99c6 100644 --- a/core/jni/android_util_MemoryIntArray.cpp +++ b/core/jni/android_util_MemoryIntArray.cpp @@ -14,9 +14,9 @@ * limitations under the License. */ - #include "core_jni_helpers.h" #include +#include #include namespace android { @@ -44,11 +44,6 @@ static jint android_util_MemoryIntArray_create(JNIEnv* env, jobject clazz, jstri return -1; } - if (ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) { - jniThrowException(env, "java/io/IOException", "ashmem was purged"); - return -1; - } - int setProtResult = ashmem_set_prot_region(fd, PROT_READ | PROT_WRITE); if (setProtResult < 0) { jniThrowException(env, "java/io/IOException", "cannot set ashmem prot mode"); @@ -133,24 +128,13 @@ static jint android_util_MemoryIntArray_get(JNIEnv* env, jobject clazz, return -1; } - bool unpin = false; - - if (!owner) { - if (ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) { - jniThrowException(env, "java/io/IOException", "ashmem region was purged"); - return -1; - } - unpin = true; + if (ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) { + jniThrowException(env, "java/io/IOException", "ashmem region was purged"); + return -1; } std::atomic_int* value = reinterpret_cast(address) + index; - const int result = value->load(std::memory_order_relaxed); - - if (unpin) { - ashmem_unpin_region(fd, 0, 0); - } - - return result; + return value->load(std::memory_order_relaxed); } static void android_util_MemoryIntArray_set(JNIEnv* env, jobject clazz, @@ -161,22 +145,13 @@ static void android_util_MemoryIntArray_set(JNIEnv* env, jobject clazz, return; } - bool unpin = false; - - if (!owner) { - if (ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) { - jniThrowException(env, "java/io/IOException", "ashmem region was purged"); - return; - } - unpin = true; + if (ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) { + jniThrowException(env, "java/io/IOException", "ashmem region was purged"); + return; } std::atomic_int* value = reinterpret_cast(address) + index; value->store(newValue, std::memory_order_relaxed); - - if (unpin) { - ashmem_unpin_region(fd, 0, 0); - } } static jint android_util_MemoryIntArray_size(JNIEnv* env, jobject clazz, jint fd) { diff --git a/core/tests/utiltests/Android.mk b/core/tests/utiltests/Android.mk index 3c6c32ec0d275..6d1ebb4a6f58a 100644 --- a/core/tests/utiltests/Android.mk +++ b/core/tests/utiltests/Android.mk @@ -10,6 +10,7 @@ LOCAL_MODULE_TAGS := tests # Include all test java files. LOCAL_SRC_FILES := $(call all-java-files-under, src) +LOCAL_SRC_FILES += src/android/util/IRemoteMemoryIntArray.aidl LOCAL_STATIC_JAVA_LIBRARIES := \ android-support-test \ diff --git a/core/tests/utiltests/AndroidManifest.xml b/core/tests/utiltests/AndroidManifest.xml index fecaf8e20c3fb..da09894255c36 100644 --- a/core/tests/utiltests/AndroidManifest.xml +++ b/core/tests/utiltests/AndroidManifest.xml @@ -43,6 +43,11 @@ + + + + = 0) { - final int generation = mImpl.get(index) + 1; - mImpl.set(index, generation); + final int generation = backingStore.get(index) + 1; + backingStore.set(index, generation); } } catch (IOException e) { Slog.e(LOG_TAG, "Error updating generation id", e); + destroyBackingStore(); } } } @@ -78,34 +68,98 @@ final class GenerationRegistry { public void addGenerationData(Bundle bundle, int key) { synchronized (mLock) { - if (mImpl != null) { - final int index = getKeyIndexLocked(key); - if (index >= 0) { - bundle.putParcelable(Settings.CALL_METHOD_TRACK_GENERATION_KEY, mImpl); - bundle.putInt(Settings.CALL_METHOD_GENERATION_INDEX_KEY, index); - if (DEBUG) { - Slog.i(LOG_TAG, "Exported index:" + index + " for key:" - + SettingsProvider.keyToString(key)); + MemoryIntArray backingStore = getBackingStoreLocked(); + try { + if (backingStore != null) { + final int index = getKeyIndexLocked(key, mKeyToIndexMap, backingStore); + if (index >= 0) { + bundle.putParcelable(Settings.CALL_METHOD_TRACK_GENERATION_KEY, + backingStore); + bundle.putInt(Settings.CALL_METHOD_GENERATION_INDEX_KEY, index); + if (DEBUG) { + Slog.i(LOG_TAG, "Exported index:" + index + " for key:" + + SettingsProvider.keyToString(key)); + } } } + } catch (IOException e) { + Slog.e(LOG_TAG, "Error adding generation data", e); + destroyBackingStore(); + } + } + } + + public void onUserRemoved(int userId) { + synchronized (mLock) { + MemoryIntArray backingStore = getBackingStoreLocked(); + if (backingStore != null && mKeyToIndexMap.size() > 0) { + try { + final int secureKey = SettingsProvider.makeKey( + SettingsProvider.SETTINGS_TYPE_SECURE, userId); + resetSlotForKeyLocked(secureKey, mKeyToIndexMap, backingStore); + + final int systemKey = SettingsProvider.makeKey( + SettingsProvider.SETTINGS_TYPE_SYSTEM, userId); + resetSlotForKeyLocked(systemKey, mKeyToIndexMap, backingStore); + } catch (IOException e) { + Slog.e(LOG_TAG, "Error cleaning up for user", e); + destroyBackingStore(); + } } } } - private int getKeyIndexLocked(int key) { - int index = mKeyToIndexMap.get(key, -1); + private MemoryIntArray getBackingStoreLocked() { + if (mBackingStore == null) { + // One for the global table, two for system and secure tables for a + // managed profile (managed profile is not included in the max user + // count), ten for partially deleted users if users are quickly removed, + // and twice max user count for system and secure. + final int size = 1 + 2 + 10 + 2 * UserManager.getMaxSupportedUsers(); + try { + mBackingStore = new MemoryIntArray(size, false); + } catch (IOException e) { + Slog.e(LOG_TAG, "Error creating generation tracker", e); + } + } + return mBackingStore; + } + + private void destroyBackingStore() { + if (mBackingStore != null) { + try { + mBackingStore.close(); + } catch (IOException e) { + Slog.e(LOG_TAG, "Cannot close generation memory array", e); + } + mBackingStore = null; + } + } + + private static void resetSlotForKeyLocked(int key, SparseIntArray keyToIndexMap, + MemoryIntArray backingStore) throws IOException { + final int index = keyToIndexMap.get(key, -1); + if (index >= 0) { + keyToIndexMap.delete(key); + backingStore.set(index, 0); + if (DEBUG) { + Slog.i(LOG_TAG, "Freed index:" + index + " for key:" + + SettingsProvider.keyToString(key)); + } + } + } + + private static int getKeyIndexLocked(int key, SparseIntArray keyToIndexMap, + MemoryIntArray backingStore) throws IOException { + int index = keyToIndexMap.get(key, -1); if (index < 0) { - index = findNextEmptyIndex(); + index = findNextEmptyIndex(backingStore); if (index >= 0) { - try { - mImpl.set(index, 1); - mKeyToIndexMap.append(key, index); - if (DEBUG) { - Slog.i(LOG_TAG, "Allocated index:" + index + " for key:" - + SettingsProvider.keyToString(key)); - } - } catch (IOException e) { - Slog.e(LOG_TAG, "Cannot write to generation memory array", e); + backingStore.set(index, 1); + keyToIndexMap.append(key, index); + if (DEBUG) { + Slog.i(LOG_TAG, "Allocated index:" + index + " for key:" + + SettingsProvider.keyToString(key)); } } else { Slog.e(LOG_TAG, "Could not allocate generation index"); @@ -114,47 +168,13 @@ final class GenerationRegistry { return index; } - public void onUserRemoved(int userId) { - synchronized (mLock) { - if (mImpl != null && mKeyToIndexMap.size() > 0) { - final int secureKey = SettingsProvider.makeKey( - SettingsProvider.SETTINGS_TYPE_SECURE, userId); - resetSlotForKeyLocked(secureKey); - - final int systemKey = SettingsProvider.makeKey( - SettingsProvider.SETTINGS_TYPE_SYSTEM, userId); - resetSlotForKeyLocked(systemKey); + private static int findNextEmptyIndex(MemoryIntArray backingStore) throws IOException { + final int size = backingStore.size(); + for (int i = 0; i < size; i++) { + if (backingStore.get(i) == 0) { + return i; } } - } - - private void resetSlotForKeyLocked(int key) { - final int index = mKeyToIndexMap.get(key, -1); - if (index >= 0) { - mKeyToIndexMap.delete(key); - try { - mImpl.set(index, 0); - if (DEBUG) { - Slog.i(LOG_TAG, "Freed index:" + index + " for key:" - + SettingsProvider.keyToString(key)); - } - } catch (IOException e) { - Slog.e(LOG_TAG, "Cannot write to generation memory array", e); - } - } - } - - private int findNextEmptyIndex() { - try { - final int size = mImpl.size(); - for (int i = 0; i < size; i++) { - if (mImpl.get(i) == 0) { - return i; - } - } - } catch (IOException e) { - Slog.e(LOG_TAG, "Error reading generation memory array", e); - } return -1; } } \ No newline at end of file