Polish MemoryIntArray
1. Add close guard
2. Adopt instead of clone the ahsmem fd to fix a dangling fd
3. Clear only the return flag when writing fd to parcel
4. Immediately destroy remote MemoryIntArray if stale
5. Throw Java exception if someone closed the fd under us
Change-Id: I85533fec336c40e3380e10d5448e18c9616ec341
diff --git a/core/java/android/provider/Settings.java b/core/java/android/provider/Settings.java
index a7ff552..879f938 100755
--- a/core/java/android/provider/Settings.java
+++ b/core/java/android/provider/Settings.java
@@ -1658,6 +1658,9 @@
+ cr.getPackageName() + " and user:"
+ userHandle + " with index:" + index);
}
+ if (mGenerationTracker != null) {
+ mGenerationTracker.destroy();
+ }
mGenerationTracker = new GenerationTracker(array, index,
generation, () -> {
synchronized (NameValueCache.this) {
diff --git a/core/java/android/util/MemoryIntArray.java b/core/java/android/util/MemoryIntArray.java
index 8f9b36e..080dd07 100644
--- a/core/java/android/util/MemoryIntArray.java
+++ b/core/java/android/util/MemoryIntArray.java
@@ -20,7 +20,9 @@
import android.os.ParcelFileDescriptor;
import android.os.Parcelable;
import android.os.Process;
+
import libcore.io.IoUtils;
+import dalvik.system.CloseGuard;
import java.io.Closeable;
import java.io.IOException;
@@ -51,6 +53,8 @@
private static final int MAX_SIZE = 1024;
+ private final CloseGuard mCloseGuard = CloseGuard.get();
+
private final int mOwnerPid;
private final boolean mClientWritable;
private final long mMemoryAddr;
@@ -71,8 +75,9 @@
mOwnerPid = Process.myPid();
mClientWritable = clientWritable;
final String name = UUID.randomUUID().toString();
- mFd = ParcelFileDescriptor.fromFd(nativeCreate(name, size));
+ mFd = ParcelFileDescriptor.adoptFd(nativeCreate(name, size));
mMemoryAddr = nativeOpen(mFd.getFd(), true, clientWritable);
+ mCloseGuard.open("close");
}
private MemoryIntArray(Parcel parcel) throws IOException {
@@ -88,6 +93,7 @@
} else {
mMemoryAddr = nativeOpen(mFd.getFd(), false, mClientWritable);
}
+ mCloseGuard.open("close");
}
/**
@@ -148,6 +154,7 @@
ParcelFileDescriptor pfd = mFd;
mFd = null;
nativeClose(pfd.getFd(), mMemoryAddr, isOwner());
+ mCloseGuard.close();
}
}
@@ -160,8 +167,12 @@
@Override
protected void finalize() throws Throwable {
- IoUtils.closeQuietly(this);
- super.finalize();
+ try {
+ mCloseGuard.warnIfOpen();
+ IoUtils.closeQuietly(this);
+ } finally {
+ super.finalize();
+ }
}
@Override
@@ -173,7 +184,8 @@
public void writeToParcel(Parcel parcel, int flags) {
parcel.writeInt(mOwnerPid);
parcel.writeInt(mClientWritable ? 1 : 0);
- parcel.writeParcelable(mFd, 0);
+ // Don't let writing to a parcel to close our fd - plz
+ parcel.writeParcelable(mFd, flags & ~Parcelable.PARCELABLE_WRITE_RETURN_VALUE);
parcel.writeLong(mMemoryAddr);
}
diff --git a/core/jni/android_util_MemoryIntArray.cpp b/core/jni/android_util_MemoryIntArray.cpp
index f45be12..9513c8b 100644
--- a/core/jni/android_util_MemoryIntArray.cpp
+++ b/core/jni/android_util_MemoryIntArray.cpp
@@ -61,6 +61,11 @@
return -1;
}
+ if (!ashmem_valid(fd)) {
+ jniThrowIOException(env, errno);
+ return -1;
+ }
+
int ashmemSize = ashmem_get_size_region(fd);
if (ashmemSize <= 0) {
jniThrowException(env, "java/io/IOException", "bad ashmem size");
@@ -98,6 +103,11 @@
return;
}
+ if (!ashmem_valid(fd)) {
+ jniThrowIOException(env, errno);
+ return;
+ }
+
int ashmemSize = ashmem_get_size_region(fd);
if (ashmemSize <= 0) {
jniThrowException(env, "java/io/IOException", "bad ashmem size");
@@ -128,6 +138,11 @@
return -1;
}
+ if (!ashmem_valid(fd)) {
+ jniThrowIOException(env, errno);
+ return -1;
+ }
+
if (ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) {
jniThrowException(env, "java/io/IOException", "ashmem region was purged");
return -1;
@@ -145,6 +160,11 @@
return;
}
+ if (!ashmem_valid(fd)) {
+ jniThrowIOException(env, errno);
+ return;
+ }
+
if (ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) {
jniThrowException(env, "java/io/IOException", "ashmem region was purged");
return;
@@ -160,17 +180,13 @@
return -1;
}
- // Use ASHMEM_GET_SIZE to find out if the fd refers to an ashmem region.
- // ASHMEM_GET_SIZE should succeed for all ashmem regions, and the kernel
- // should return ENOTTY for all other valid file descriptors
+ if (!ashmem_valid(fd)) {
+ jniThrowIOException(env, errno);
+ return -1;
+ }
+
int ashmemSize = ashmem_get_size_region(fd);
if (ashmemSize < 0) {
- if (errno == ENOTTY) {
- // ENOTTY means that the ioctl does not apply to this object,
- // i.e., it is not an ashmem region.
- return -1;
- }
- // Some other error, throw exception
jniThrowIOException(env, errno);
return -1;
}
diff --git a/packages/SettingsProvider/src/com/android/providers/settings/GenerationRegistry.java b/packages/SettingsProvider/src/com/android/providers/settings/GenerationRegistry.java
index f4f7986..222cc5c 100644
--- a/packages/SettingsProvider/src/com/android/providers/settings/GenerationRegistry.java
+++ b/packages/SettingsProvider/src/com/android/providers/settings/GenerationRegistry.java
@@ -32,7 +32,7 @@
* processes can read to determine if their local caches are stale,
*/
final class GenerationRegistry {
- private static final String LOG_TAG = "GenerationTracker";
+ private static final String LOG_TAG = "GenerationRegistry";
private static final boolean DEBUG = false;
@@ -120,6 +120,9 @@
final int size = 1 + 2 + 10 + 2 * UserManager.getMaxSupportedUsers();
try {
mBackingStore = new MemoryIntArray(size, false);
+ if (DEBUG) {
+ Slog.e(LOG_TAG, "Created backing store " + mBackingStore);
+ }
} catch (IOException e) {
Slog.e(LOG_TAG, "Error creating generation tracker", e);
}
@@ -131,6 +134,9 @@
if (mBackingStore != null) {
try {
mBackingStore.close();
+ if (DEBUG) {
+ Slog.e(LOG_TAG, "Destroyed backing store " + mBackingStore);
+ }
} catch (IOException e) {
Slog.e(LOG_TAG, "Cannot close generation memory array", e);
}