Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c81f53f7 authored by Josh Gao's avatar Josh Gao
Browse files

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
parent 9257722c

core/java/android/util/MemoryIntArray.java

+19 −22
Original line number Diff line number Diff line
@@ -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();

        }

        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() {

core/tests/utiltests/jni/android_util_MemoryIntArrayTest.cpp

+0 −30
Original line number Diff line number Diff line
@@ -21,36 +21,6 @@ 
#include <sys/ioctl.h>

#include <sys/mman.h>



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)

{

core/tests/utiltests/jni/registration.cpp

+0 −11
Original line number Diff line number Diff line
@@ -16,25 +16,14 @@ 


#include <jni.h>



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)

core/tests/utiltests/src/android/util/MemoryIntArrayTest.java

+7 −9
Original line number Diff line number Diff line
@@ -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);

}