From d2404405e77a1adbba7d17af30640374f6428308 Mon Sep 17 00:00:00 2001 From: Svet Ganov Date: Fri, 3 May 2019 19:17:49 -0700 Subject: [PATCH] Revert "MemoryIntArray: track the owned file descriptor in a PFD." The reverted change causes a regression where we can get an IllegalStateException during finalization as we are adopting the native fd in a ParcelFileDescriptor which takes ownership of the fd. However, the order of finalization is undefined and if the ParcelFileDescriptor is finalized before the MemoryIntArray we would get an exception when running the finalization of the latter. bug:124056170 This reverts commit c81f53f7f114962f757312ac884a279035fe0584. Change-Id: I8debb9c5f4c87b1a657084139b27f40b7956fe59 --- core/java/android/util/MemoryIntArray.java | 41 ++++++++++--------- core/tests/utiltests/Android.mk | 1 + .../jni/android_util_MemoryIntArrayTest.cpp | 30 ++++++++++++++ core/tests/utiltests/jni/registration.cpp | 11 +++++ .../src/android/util/MemoryIntArrayTest.java | 20 +++++---- 5 files changed, 76 insertions(+), 27 deletions(-) diff --git a/core/java/android/util/MemoryIntArray.java b/core/java/android/util/MemoryIntArray.java index 74fea3f4dd307..80b16075cdf69 100644 --- a/core/java/android/util/MemoryIntArray.java +++ b/core/java/android/util/MemoryIntArray.java @@ -20,9 +20,8 @@ import android.os.Parcel; import android.os.ParcelFileDescriptor; import android.os.Parcelable; -import dalvik.system.CloseGuard; - import libcore.io.IoUtils; +import dalvik.system.CloseGuard; import java.io.Closeable; import java.io.IOException; @@ -57,7 +56,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { private final boolean mIsOwner; private final long mMemoryAddr; - private ParcelFileDescriptor mFd; + private int mFd = -1; /** * Creates a new instance. @@ -72,8 +71,8 @@ public final class MemoryIntArray implements Parcelable, Closeable { } mIsOwner = true; final String name = UUID.randomUUID().toString(); - mFd = ParcelFileDescriptor.adoptFd(nativeCreate(name, size)); - mMemoryAddr = nativeOpen(mFd.getFd(), mIsOwner); + mFd = nativeCreate(name, size); + mMemoryAddr = nativeOpen(mFd, mIsOwner); mCloseGuard.open("close"); } @@ -83,8 +82,8 @@ public final class MemoryIntArray implements Parcelable, Closeable { if (pfd == null) { throw new IOException("No backing file descriptor"); } - mFd = ParcelFileDescriptor.adoptFd(pfd.detachFd()); - mMemoryAddr = nativeOpen(mFd.getFd(), mIsOwner); + mFd = pfd.detachFd(); + mMemoryAddr = nativeOpen(mFd, mIsOwner); mCloseGuard.open("close"); } @@ -106,7 +105,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { public int get(int index) throws IOException { enforceNotClosed(); enforceValidIndex(index); - return nativeGet(mFd.getFd(), mMemoryAddr, index); + return nativeGet(mFd, mMemoryAddr, index); } /** @@ -122,7 +121,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { enforceNotClosed(); enforceWritable(); enforceValidIndex(index); - nativeSet(mFd.getFd(), mMemoryAddr, index, value); + nativeSet(mFd, mMemoryAddr, index, value); } /** @@ -132,7 +131,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { */ public int size() throws IOException { enforceNotClosed(); - return nativeSize(mFd.getFd()); + return nativeSize(mFd); } /** @@ -143,9 +142,8 @@ public final class MemoryIntArray implements Parcelable, Closeable { @Override public void close() throws IOException { if (!isClosed()) { - nativeClose(mFd.getFd(), mMemoryAddr, mIsOwner); - mFd.close(); - mFd = null; + nativeClose(mFd, mMemoryAddr, mIsOwner); + mFd = -1; mCloseGuard.close(); } } @@ -154,7 +152,7 @@ public final class MemoryIntArray implements Parcelable, Closeable { * @return Whether this array is closed and shouldn't be used. */ public boolean isClosed() { - return mFd == null; + return mFd == -1; } @Override @@ -177,8 +175,13 @@ public final class MemoryIntArray implements Parcelable, Closeable { @Override public void writeToParcel(Parcel parcel, int flags) { - // Don't let writing to a parcel to close our fd - plz - parcel.writeParcelable(mFd, flags & ~Parcelable.PARCELABLE_WRITE_RETURN_VALUE); + 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(); + } } @Override @@ -192,13 +195,13 @@ public final class MemoryIntArray implements Parcelable, Closeable { if (getClass() != obj.getClass()) { return false; } - - return false; + MemoryIntArray other = (MemoryIntArray) obj; + return mFd == other.mFd; } @Override public int hashCode() { - return mFd.hashCode(); + return mFd; } private void enforceNotClosed() { diff --git a/core/tests/utiltests/Android.mk b/core/tests/utiltests/Android.mk index 343c07af51dfb..9ef73e9aad939 100644 --- a/core/tests/utiltests/Android.mk +++ b/core/tests/utiltests/Android.mk @@ -18,6 +18,7 @@ LOCAL_STATIC_JAVA_LIBRARIES := \ androidx.test.rules \ frameworks-base-testutils \ mockito-target-minus-junit4 \ + androidx.test.ext.junit LOCAL_JAVA_LIBRARIES := android.test.runner android.test.base android.test.mock diff --git a/core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp b/core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp index 4b14284fdea56..57ee2d5f6cbbb 100644 --- a/core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp +++ b/core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp @@ -21,6 +21,36 @@ #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 d4fc2fbb83fca..0c84d98e9de93 100644 --- a/core/tests/utiltests/jni/registration.cpp +++ b/core/tests/utiltests/jni/registration.cpp @@ -16,14 +16,25 @@ #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 2daefe74eb12c..1966e122ee5b8 100644 --- a/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java +++ b/core/tests/utiltests/src/android/util/MemoryIntArrayTest.java @@ -23,9 +23,8 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import android.os.Parcel; -import android.os.ParcelFileDescriptor; -import androidx.test.runner.AndroidJUnit4; +import androidx.test.ext.junit.runners.AndroidJUnit4; import libcore.io.IoUtils; @@ -36,6 +35,8 @@ import java.lang.reflect.Field; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; + +import androidx.test.ext.junit.runners.AndroidJUnit4; @RunWith(AndroidJUnit4.class) public class MemoryIntArrayTest { static { @@ -255,11 +256,13 @@ public class MemoryIntArrayTest { // Create a MemoryIntArray to muck with MemoryIntArray array = new MemoryIntArray(1); - // 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); + // 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); CountDownLatch countDownLatch = new CountDownLatch(2); @@ -294,9 +297,10 @@ public class MemoryIntArrayTest { } if (!success) { - fail("MemoryIntArray should catch ashmem size changing under it"); + fail("MemoryIntArray should catch ahshmem size changing under it"); } } + private native int nativeCreateAshmem(String name, int size); private native void nativeSetAshmemSize(int fd, int size); }