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
diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java
index 370cd57..a4c0d67 100755
--- a/core/java/android/provider/Settings.java
+++ b/core/java/android/provider/Settings.java
@@ -1462,12 +1462,15 @@
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 @@
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 @@
+ 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 c3bd963..8f9b36e 100644
--- a/core/java/android/util/MemoryIntArray.java
+++ b/core/java/android/util/MemoryIntArray.java
@@ -20,6 +20,7 @@
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 @@
* @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 @@
@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 @@
@Override
protected void finalize() throws Throwable {
- close();
+ IoUtils.closeQuietly(this);
super.finalize();
}
@@ -230,7 +234,6 @@
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 @@
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 dbe8ed3..f45be12 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 <cutils/ashmem.h>
+#include <linux/ashmem.h>
#include <sys/mman.h>
namespace android {
@@ -44,11 +44,6 @@
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 @@
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<std::atomic_int*>(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 @@
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<std::atomic_int*>(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 3c6c32e..6d1ebb4 100644
--- a/core/tests/utiltests/Android.mk
+++ b/core/tests/utiltests/Android.mk
@@ -10,6 +10,7 @@
# 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 fecaf8e..da09894 100644
--- a/core/tests/utiltests/AndroidManifest.xml
+++ b/core/tests/utiltests/AndroidManifest.xml
@@ -43,6 +43,11 @@
<application>
<uses-library android:name="android.test.runner" />
+
+ <service android:name="android.util.RemoteMemoryIntArrayService"
+ android:process=":remote">
+ </service>
+
</application>
<instrumentation
diff --git a/core/tests/utiltests/src/android/util/IRemoteMemoryIntArray.aidl b/core/tests/utiltests/src/android/util/IRemoteMemoryIntArray.aidl
new file mode 100644
index 0000000..0a65fff2
--- /dev/null
+++ b/core/tests/utiltests/src/android/util/IRemoteMemoryIntArray.aidl
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2016 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 android.util.MemoryIntArray;
+
+interface IRemoteMemoryIntArray {
+ MemoryIntArray peekInstance();
+ void create(int size, boolean clientWritable);
+ boolean isWritable();
+ int get(int index);
+ void set(int index, int value);
+ int size();
+ void close();
+ boolean isClosed();
+}
diff --git a/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java b/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java
index 6b31916..129e6b7 100644
--- a/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java
+++ b/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java
@@ -16,12 +16,22 @@
package android.util;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
import android.os.Parcel;
-import junit.framework.TestCase;
+import android.support.test.runner.AndroidJUnit4;
import libcore.io.IoUtils;
+import org.junit.Test;
+import org.junit.runner.RunWith;
-public class MemoryIntArrayTest extends TestCase {
+@RunWith(AndroidJUnit4.class)
+public class MemoryIntArrayTest {
+ @Test
public void testSize() throws Exception {
MemoryIntArray array = null;
try {
@@ -32,6 +42,7 @@
}
}
+ @Test
public void testGetSet() throws Exception {
MemoryIntArray array = null;
try {
@@ -49,6 +60,7 @@
}
}
+ @Test
public void testWritable() throws Exception {
MemoryIntArray array = null;
try {
@@ -59,6 +71,7 @@
}
}
+ @Test
public void testClose() throws Exception {
MemoryIntArray array = null;
try {
@@ -72,6 +85,7 @@
}
}
+ @Test
public void testMarshalledGetSet() throws Exception {
MemoryIntArray firstArray = null;
MemoryIntArray secondArray = null;
@@ -99,6 +113,7 @@
}
}
+ @Test
public void testInteractOnceClosed() throws Exception {
MemoryIntArray array = null;
try {
@@ -141,6 +156,7 @@
}
}
+ @Test
public void testInteractPutOfBounds() throws Exception {
MemoryIntArray array = null;
try {
@@ -178,6 +194,7 @@
}
}
+ @Test
public void testOverMaxSize() throws Exception {
MemoryIntArray array = null;
try {
@@ -189,4 +206,28 @@
IoUtils.closeQuietly(array);
}
}
+
+ @Test
+ public void testNotMutableByUnprivilegedClients() throws Exception {
+ RemoteIntArray remoteIntArray = new RemoteIntArray(1, false);
+ try {
+ assertNotNull("Couldn't get remote instance", remoteIntArray);
+ MemoryIntArray localIntArray = remoteIntArray.peekInstance();
+ assertNotNull("Couldn't get local instance", localIntArray);
+
+ remoteIntArray.set(0, 1);
+ assertSame("Remote should be able to modify", 1, remoteIntArray.get(0));
+
+ try {
+ localIntArray.set(0, 0);
+ fail("Local shouldn't be able to modify");
+ } catch (UnsupportedOperationException e) {
+ /* expected */
+ }
+ assertSame("Local shouldn't be able to modify", 1, localIntArray.get(0));
+ assertSame("Local shouldn't be able to modify", 1, remoteIntArray.get(0));
+ } finally {
+ remoteIntArray.destroy();
+ }
+ }
}
diff --git a/core/tests/utiltests/src/android/util/RemoteIntArray.java b/core/tests/utiltests/src/android/util/RemoteIntArray.java
new file mode 100644
index 0000000..10c325f
--- /dev/null
+++ b/core/tests/utiltests/src/android/util/RemoteIntArray.java
@@ -0,0 +1,165 @@
+/*
+ * Copyright (C) 2016 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 android.content.ComponentName;
+import android.content.Context;
+import android.content.Intent;
+import android.content.ServiceConnection;
+import android.os.Build;
+import android.os.IBinder;
+import android.os.RemoteException;
+import android.os.SystemClock;
+import android.support.test.InstrumentationRegistry;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.concurrent.TimeoutException;
+
+final class RemoteIntArray implements ServiceConnection, Closeable {
+ private static final long BIND_REMOTE_SERVICE_TIMEOUT =
+ ("eng".equals(Build.TYPE)) ? 120000 : 10000;
+
+ private final Object mLock = new Object();
+
+ private final Intent mIntent = new Intent();
+
+ private android.util.IRemoteMemoryIntArray mRemoteInstance;
+
+ public RemoteIntArray(int size, boolean clientWritable) throws IOException, TimeoutException {
+ mIntent.setComponent(new ComponentName(InstrumentationRegistry.getContext(),
+ RemoteMemoryIntArrayService.class));
+ synchronized (mLock) {
+ if (mRemoteInstance == null) {
+ bindLocked();
+ }
+ try {
+ mRemoteInstance.create(size, clientWritable);
+ } catch (RemoteException e) {
+ throw new IOException(e);
+ }
+ }
+ }
+
+ public MemoryIntArray peekInstance() {
+ try {
+ return mRemoteInstance.peekInstance();
+ } catch (RemoteException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public boolean isWritable() {
+ try {
+ return mRemoteInstance.isWritable();
+ } catch (RemoteException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public int get(int index) throws IOException {
+ try {
+ return mRemoteInstance.get(index);
+ } catch (RemoteException e) {
+ throw new IOException(e);
+ }
+ }
+
+ public void set(int index, int value) throws IOException {
+ try {
+ mRemoteInstance.set(index, value);
+ } catch (RemoteException e) {
+ throw new IOException(e);
+ }
+ }
+
+ public int size() throws IOException {
+ try {
+ return mRemoteInstance.size();
+ } catch (RemoteException e) {
+ throw new IOException(e);
+ }
+ }
+
+ public void close() {
+ try {
+ mRemoteInstance.close();
+ } catch (RemoteException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public boolean isClosed() {
+ try {
+ return mRemoteInstance.isClosed();
+ } catch (RemoteException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private void bindLocked() throws TimeoutException {
+ if (mRemoteInstance != null) {
+ return;
+ }
+
+ InstrumentationRegistry.getContext().bindService(mIntent, this, Context.BIND_AUTO_CREATE);
+
+ final long startMillis = SystemClock.uptimeMillis();
+ while (true) {
+ if (mRemoteInstance != null) {
+ break;
+ }
+ final long elapsedMillis = SystemClock.uptimeMillis() - startMillis;
+ final long remainingMillis = BIND_REMOTE_SERVICE_TIMEOUT - elapsedMillis;
+ if (remainingMillis <= 0) {
+ throw new TimeoutException("Cannot get spooler!");
+ }
+ try {
+ mLock.wait(remainingMillis);
+ } catch (InterruptedException ie) {
+ /* ignore */
+ }
+ }
+
+ mLock.notifyAll();
+ }
+
+ public void destroy() {
+ synchronized (mLock) {
+ if (mRemoteInstance == null) {
+ return;
+ }
+ mRemoteInstance = null;
+ InstrumentationRegistry.getContext().unbindService(this);
+ }
+ }
+
+ @Override
+ public void onServiceConnected(ComponentName name, IBinder service) {
+ synchronized (mLock) {
+ mRemoteInstance = android.util.IRemoteMemoryIntArray.Stub.asInterface(service);
+ mLock.notifyAll();
+ }
+ }
+
+ @Override
+ public void onServiceDisconnected(ComponentName name) {
+ synchronized (mLock) {
+ mRemoteInstance = null;
+ }
+ }
+}
\ No newline at end of file
diff --git a/core/tests/utiltests/src/android/util/RemoteMemoryIntArrayService.java b/core/tests/utiltests/src/android/util/RemoteMemoryIntArrayService.java
new file mode 100644
index 0000000..35ae9a7
--- /dev/null
+++ b/core/tests/utiltests/src/android/util/RemoteMemoryIntArrayService.java
@@ -0,0 +1,114 @@
+/*
+ * Copyright (C) 2016 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 android.app.Service;
+import android.content.Intent;
+import android.os.IBinder;
+
+import java.io.IOException;
+
+/**
+ * Service to interact with a {@link MemoryIntArray} in another process.
+ */
+public class RemoteMemoryIntArrayService extends Service {
+ private final Object mLock = new Object();
+
+ private MemoryIntArray mArray;
+
+ @Override
+ public IBinder onBind(Intent intent) {
+ return new android.util.IRemoteMemoryIntArray.Stub() {
+
+ @Override
+ public void create(int size, boolean clientWritable) {
+ synchronized (mLock) {
+ try {
+ mArray = new MemoryIntArray(size, clientWritable);
+ } catch (IOException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
+ @Override
+ public MemoryIntArray peekInstance() {
+ synchronized (mLock) {
+ return mArray;
+ }
+ }
+
+ @Override
+ public boolean isWritable() {
+ synchronized (mLock) {
+ return mArray.isWritable();
+ }
+ }
+
+ @Override
+ public int get(int index) {
+ synchronized (mLock) {
+ try {
+ return mArray.get(index);
+ } catch (IOException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
+ @Override
+ public void set(int index, int value) {
+ synchronized (mLock) {
+ try {
+ mArray.set(index, value);
+ } catch (IOException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
+ @Override
+ public int size() {
+ synchronized (mLock) {
+ try {
+ return mArray.size();
+ } catch (IOException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
+ @Override
+ public void close() {
+ synchronized (mLock) {
+ try {
+ mArray.close();
+ } catch (IOException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
+ @Override
+ public boolean isClosed() {
+ synchronized (mLock) {
+ return mArray.isClosed();
+ }
+ }
+ };
+ }
+}