From c81f53f7f114962f757312ac884a279035fe0584 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 9 Aug 2018 15:09:02 -0700 Subject: [PATCH] MemoryIntArray: track the owned file descriptor in a PFD. AUPT is triggering an fdsan diagnostic when creating a temporary ParcelFileDescriptor to write to a Parcel. This code seems correct at first glance, so under the assumption that some other code is closing the file descriptor out from under us, keep our owned file descriptor around as a ParcelFileDescriptor to catch the perpetrator in the act. Bug: http://b/112405224 Test: atest MemoryIntArrayTest (testAshmemSizeMatchesMemoryIntArraySize failed/crashed before, fails now) Change-Id: Ie8ff7562c78ecde4cf1757d572ecb733213cc975 --- core/java/android/util/MemoryIntArray.java | 41 +++++++++---------- .../jni/android_util_MemoryIntArrayTest.cpp | 30 -------------- core/tests/utiltests/jni/registration.cpp | 11 ----- .../src/android/util/MemoryIntArrayTest.java | 16 ++++---- 4 files changed, 26 insertions(+), 72 deletions(-) diff --git a/core/java/android/util/MemoryIntArray.java b/core/java/android/util/MemoryIntArray.java index bf335196edef8..d5bec0fff0880 100644 --- a/core/java/android/util/MemoryIntArray.java +++ b/core/java/android/util/MemoryIntArray.java @@ -20,9 +20,10 @@ import android.os.Parcel; import android.os.ParcelFileDescriptor; import android.os.Parcelable; -import libcore.io.IoUtils; import dalvik.system.CloseGuard; +import libcore.io.IoUtils; + import java.io.Closeable; import java.io.IOException; import java.util.UUID; @@ -56,7 +57,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { private final boolean mIsOwner; private final long mMemoryAddr; - private int mFd = -1; + private ParcelFileDescriptor mFd; /** * Creates a new instance. @@ -71,8 +72,8 @@ public final class MemoryIntArray implements Parcelable, Closeable { } mIsOwner = true; final String name = UUID.randomUUID().toString(); - mFd = nativeCreate(name, size); - mMemoryAddr = nativeOpen(mFd, mIsOwner); + mFd = ParcelFileDescriptor.adoptFd(nativeCreate(name, size)); + mMemoryAddr = nativeOpen(mFd.getFd(), mIsOwner); mCloseGuard.open("close"); } @@ -82,8 +83,8 @@ public final class MemoryIntArray implements Parcelable, Closeable { if (pfd == null) { throw new IOException("No backing file descriptor"); } - mFd = pfd.detachFd(); - mMemoryAddr = nativeOpen(mFd, mIsOwner); + mFd = ParcelFileDescriptor.adoptFd(pfd.detachFd()); + mMemoryAddr = nativeOpen(mFd.getFd(), mIsOwner); mCloseGuard.open("close"); } @@ -105,7 +106,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { public int get(int index) throws IOException { enforceNotClosed(); enforceValidIndex(index); - return nativeGet(mFd, mMemoryAddr, index); + return nativeGet(mFd.getFd(), mMemoryAddr, index); } /** @@ -121,7 +122,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { enforceNotClosed(); enforceWritable(); enforceValidIndex(index); - nativeSet(mFd, mMemoryAddr, index, value); + nativeSet(mFd.getFd(), mMemoryAddr, index, value); } /** @@ -131,7 +132,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { */ public int size() throws IOException { enforceNotClosed(); - return nativeSize(mFd); + return nativeSize(mFd.getFd()); } /** @@ -142,8 +143,9 @@ public final class MemoryIntArray implements Parcelable, Closeable { @Override public void close() throws IOException { if (!isClosed()) { - nativeClose(mFd, mMemoryAddr, mIsOwner); - mFd = -1; + nativeClose(mFd.getFd(), mMemoryAddr, mIsOwner); + mFd.close(); + mFd = null; mCloseGuard.close(); } } @@ -152,7 +154,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { * @return Whether this array is closed and shouldn't be used. */ public boolean isClosed() { - return mFd == -1; + return mFd == null; } @Override @@ -175,13 +177,8 @@ public final class MemoryIntArray implements Parcelable, Closeable { @Override public void writeToParcel(Parcel parcel, int flags) { - ParcelFileDescriptor pfd = ParcelFileDescriptor.adoptFd(mFd); - try { - // Don't let writing to a parcel to close our fd - plz - parcel.writeParcelable(pfd, flags & ~Parcelable.PARCELABLE_WRITE_RETURN_VALUE); - } finally { - pfd.detachFd(); - } + // Don't let writing to a parcel to close our fd - plz + parcel.writeParcelable(mFd, flags & ~Parcelable.PARCELABLE_WRITE_RETURN_VALUE); } @Override @@ -195,13 +192,13 @@ public final class MemoryIntArray implements Parcelable, Closeable { if (getClass() != obj.getClass()) { return false; } - MemoryIntArray other = (MemoryIntArray) obj; - return mFd == other.mFd; + + return false; } @Override public int hashCode() { - return mFd; + return mFd.hashCode(); } private void enforceNotClosed() { diff --git a/core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp b/core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp index 57ee2d5f6cbbb..4b14284fdea56 100644 --- a/core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp +++ b/core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp @@ -21,36 +21,6 @@ #include #include -jint android_util_MemoryIntArrayTest_createAshmem(__attribute__((unused)) JNIEnv* env, - __attribute__((unused)) jobject clazz, - jstring name, jint size) -{ - - if (name == NULL) { - return -1; - } - - if (size < 0) { - return -1; - } - - const char* nameStr = env->GetStringUTFChars(name, NULL); - const int ashmemSize = sizeof(std::atomic_int) * size; - int fd = ashmem_create_region(nameStr, ashmemSize); - env->ReleaseStringUTFChars(name, nameStr); - - if (fd < 0) { - return -1; - } - - int setProtResult = ashmem_set_prot_region(fd, PROT_READ | PROT_WRITE); - if (setProtResult < 0) { - return -1; - } - - return fd; -} - void android_util_MemoryIntArrayTest_setAshmemSize(__attribute__((unused)) JNIEnv* env, __attribute__((unused)) jobject clazz, jint fd, jint size) { diff --git a/core/tests/utiltests/jni/registration.cpp b/core/tests/utiltests/jni/registration.cpp index 0c84d98e9de93..d4fc2fbb83fca 100644 --- a/core/tests/utiltests/jni/registration.cpp +++ b/core/tests/utiltests/jni/registration.cpp @@ -16,25 +16,14 @@ #include -extern jint android_util_MemoryIntArrayTest_createAshmem(JNIEnv* env, - jobject clazz, jstring name, jint size); extern void android_util_MemoryIntArrayTest_setAshmemSize(JNIEnv* env, jobject clazz, jint fd, jint size); extern "C" { - JNIEXPORT jint JNICALL Java_android_util_MemoryIntArrayTest_nativeCreateAshmem( - JNIEnv * env, jobject obj, jstring name, jint size); JNIEXPORT void JNICALL Java_android_util_MemoryIntArrayTest_nativeSetAshmemSize( JNIEnv * env, jobject obj, jint fd, jint size); }; -JNIEXPORT jint JNICALL Java_android_util_MemoryIntArrayTest_nativeCreateAshmem( - __attribute__((unused)) JNIEnv * env,__attribute__((unused)) jobject obj, - jstring name, jint size) -{ - return android_util_MemoryIntArrayTest_createAshmem(env, obj, name, size); -} - JNIEXPORT void JNICALL Java_android_util_MemoryIntArrayTest_nativeSetAshmemSize( __attribute__((unused)) JNIEnv * env,__attribute__((unused)) jobject obj, jint fd, jint size) diff --git a/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java b/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java index 85817bbde1bb0..24b33effdb719 100644 --- a/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java +++ b/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java @@ -23,6 +23,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import android.os.Parcel; +import android.os.ParcelFileDescriptor; import android.support.test.runner.AndroidJUnit4; import libcore.io.IoUtils; import org.junit.Test; @@ -251,13 +252,11 @@ public class MemoryIntArrayTest { // Create a MemoryIntArray to muck with MemoryIntArray array = new MemoryIntArray(1); - // Create the fd to stuff in the MemoryIntArray - final int fd = nativeCreateAshmem("foo", 1); - - // Replace the fd with our ahsmem region - Field fdFiled = MemoryIntArray.class.getDeclaredField("mFd"); - fdFiled.setAccessible(true); - fdFiled.set(array, fd); + // Grab the internal ashmem fd. + Field fdField = MemoryIntArray.class.getDeclaredField("mFd"); + fdField.setAccessible(true); + int fd = ((ParcelFileDescriptor)fdField.get(array)).getFd(); + assertTrue("fd must be valid", fd != -1); CountDownLatch countDownLatch = new CountDownLatch(2); @@ -292,10 +291,9 @@ public class MemoryIntArrayTest { } if (!success) { - fail("MemoryIntArray should catch ahshmem size changing under it"); + fail("MemoryIntArray should catch ashmem size changing under it"); } } - private native int nativeCreateAshmem(String name, int size); private native void nativeSetAshmemSize(int fd, int size); }