Unmap memory in MemoryFile.close().

As reported in http://b/issue?id=1398215 MemoryFile did not
munmap(2) the ashmem region after closing it. This
causes the process to leak virtual address space.

This change fixes the problem by calling munmap(2) in
close(). The unmapping is done by a helper method deactivate().
The change also replaces the use of an int for the
file descriptor with a FileDescriptor object to
make sure that we keep track of when the file descriptor
has been closed. I chose to implement it this way because I
will need decativate() and a FileDescriptor object in an
upcoming change that allows sending MemoryFile file
descriptors between processes.

The change also adds a number of tests for the behavior
of close(). The testCloseRead() and testCloseWrite() fail
with the old MemoryFile implementation, and testCloseLeak()
causes a segfault. They all pass now.

2 files changed, 81 insertions, 28 deletions

diff --git a/core/java/android/os/MemoryFile.java b/core/java/android/os/MemoryFile.java
index 2e4d9b1..65e83c7 100644
--- a/core/java/android/os/MemoryFile.java
+++ b/core/java/android/os/MemoryFile.java
@@ -18,6 +18,7 @@ package android.os;
 
 import android.util.Log;
 
+import java.io.FileDescriptor;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -35,19 +36,19 @@ import java.io.OutputStream;
 public class MemoryFile
 {
     private static String TAG = "MemoryFile";
- 
-    // returns fd
-    private static native int native_open(String name, int length) throws IOException;
+
+    private static native FileDescriptor native_open(String name, int length) throws IOException;
     // returns memory address for ashmem region
-    private static native int native_mmap(int fd, int length) throws IOException;
-    private static native void native_close(int fd);
-    private static native int native_read(int fd, int address, byte[] buffer,
+    private static native int native_mmap(FileDescriptor fd, int length) throws IOException;
+    private static native void native_munmap(int addr, int length) throws IOException;
+    private static native void native_close(FileDescriptor fd);
+    private static native int native_read(FileDescriptor fd, int address, byte[] buffer,
             int srcOffset, int destOffset, int count, boolean isUnpinned) throws IOException;
-    private static native void native_write(int fd, int address, byte[] buffer,
+    private static native void native_write(FileDescriptor fd, int address, byte[] buffer,
             int srcOffset, int destOffset, int count, boolean isUnpinned) throws IOException;
-    private static native void native_pin(int fd, boolean pin) throws IOException;
+    private static native void native_pin(FileDescriptor fd, boolean pin) throws IOException;
 
-    private int mFD;        // ashmem file descriptor
+    private FileDescriptor mFD;        // ashmem file descriptor
     private int mAddress;   // address of ashmem memory
     private int mLength;    // total length of our ashmem region
     private boolean mAllowPurging = false;  // true if our ashmem region is unpinned
@@ -69,15 +70,40 @@ public class MemoryFile
      * Closes and releases all resources for the memory file.
      */
     public void close() {
-        if (mFD > 0) {
+        deactivate();
+        if (!isClosed()) {
             native_close(mFD);
-            mFD = 0;
         }
     }
 
+    private void deactivate() {
+        if (!isDeactivated()) {
+            try {
+                native_munmap(mAddress, mLength);
+                mAddress = 0;
+            } catch (IOException ex) {
+                Log.e(TAG, ex.toString());
+            }
+        }
+    }
+
+    /**
+     * Checks whether the memory file has been deactivated.
+     */
+    private boolean isDeactivated() {
+        return mAddress == 0;
+    }
+
+    /**
+     * Checks whether the memory file has been closed.
+     */
+    private boolean isClosed() {
+        return !mFD.valid();
+    }
+
     @Override
     protected void finalize() {
-        if (mFD > 0) {
+        if (!isClosed()) {
             Log.e(TAG, "MemoryFile.finalize() called while ashmem still open");
             close();
         }
@@ -132,7 +158,6 @@ public class MemoryFile
      @return OutputStream
      */
      public OutputStream getOutputStream() {
-
         return new MemoryOutputStream();
     }
 
@@ -145,9 +170,13 @@ public class MemoryFile
      * @param destOffset offset into the byte array buffer to read into.
      * @param count number of bytes to read.
      * @return number of bytes read.
+     * @throws IOException if the memory file has been purged or deactivated.
      */
     public int readBytes(byte[] buffer, int srcOffset, int destOffset, int count) 
             throws IOException {
+        if (isDeactivated()) {
+            throw new IOException("Can't read from deactivated memory file.");
+        }
         if (destOffset < 0 || destOffset > buffer.length || count < 0
                 || count > buffer.length - destOffset
                 || srcOffset < 0 || srcOffset > mLength
@@ -165,9 +194,13 @@ public class MemoryFile
      * @param srcOffset offset into the byte array buffer to write from.
      * @param destOffset offset  into the memory file to write to.
      * @param count number of bytes to write.
+     * @throws IOException if the memory file has been purged or deactivated.
      */
     public void writeBytes(byte[] buffer, int srcOffset, int destOffset, int count)
             throws IOException {
+        if (isDeactivated()) {
+            throw new IOException("Can't write to deactivated memory file.");
+        }
         if (srcOffset < 0 || srcOffset > buffer.length || count < 0
                 || count > buffer.length - srcOffset
                 || destOffset < 0 || destOffset > mLength
diff --git a/core/jni/android_os_MemoryFile.cpp b/core/jni/android_os_MemoryFile.cpp
index edf7dc4..9450e15 100644
--- a/core/jni/android_os_MemoryFile.cpp
+++ b/core/jni/android_os_MemoryFile.cpp
@@ -26,7 +26,7 @@
 
 namespace android {
 
-static jint android_os_MemoryFile_open(JNIEnv* env, jobject clazz, jstring name, jint length)
+static jobject android_os_MemoryFile_open(JNIEnv* env, jobject clazz, jstring name, jint length)
 {
     const char* namestr = (name ? env->GetStringUTFChars(name, NULL) : NULL);
 
@@ -37,28 +37,45 @@ static jint android_os_MemoryFile_open(JNIEnv* env, jobject clazz, jstring name,
     if (name)
         env->ReleaseStringUTFChars(name, namestr);
 
-    if (result < 0)
+    if (result < 0) {
         jniThrowException(env, "java/io/IOException", "ashmem_create_region failed");
-    return result;
+	return NULL;
+    }
+
+    return jniCreateFileDescriptor(env, result);
 }
 
-static jint android_os_MemoryFile_mmap(JNIEnv* env, jobject clazz, jint fd, jint length)
+static jint android_os_MemoryFile_mmap(JNIEnv* env, jobject clazz, jobject fileDescriptor,
+        jint length)
 {
+    int fd = jniGetFDFromFileDescriptor(env, fileDescriptor);
     jint result = (jint)mmap(NULL, length, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
     if (!result)
         jniThrowException(env, "java/io/IOException", "mmap failed");
     return result;
 }
 
-static void android_os_MemoryFile_close(JNIEnv* env, jobject clazz, jint fd)
+static void android_os_MemoryFile_munmap(JNIEnv* env, jobject clazz, jint addr, jint length)
 {
-    close(fd);
+    int result = munmap((void *)addr, length);
+    if (result < 0)
+        jniThrowException(env, "java/io/IOException", "munmap failed");
+}
+
+static void android_os_MemoryFile_close(JNIEnv* env, jobject clazz, jobject fileDescriptor)
+{
+    int fd = jniGetFDFromFileDescriptor(env, fileDescriptor);
+    if (fd >= 0) {
+        jniSetFileDescriptorOfFD(env, fileDescriptor, -1);
+        close(fd);
+    }
 }
 
 static jint android_os_MemoryFile_read(JNIEnv* env, jobject clazz,
-        jint fd, jint address, jbyteArray buffer, jint srcOffset, jint destOffset,
+        jobject fileDescriptor, jint address, jbyteArray buffer, jint srcOffset, jint destOffset,
         jint count, jboolean unpinned)
 {
+    int fd = jniGetFDFromFileDescriptor(env, fileDescriptor);
     if (unpinned && ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) {
         ashmem_unpin_region(fd, 0, 0);
         jniThrowException(env, "java/io/IOException", "ashmem region was purged");
@@ -76,9 +93,10 @@ static jint android_os_MemoryFile_read(JNIEnv* env, jobject clazz,
 }
 
 static jint android_os_MemoryFile_write(JNIEnv* env, jobject clazz,
-        jint fd, jint address, jbyteArray buffer, jint srcOffset, jint destOffset,
+        jobject fileDescriptor, jint address, jbyteArray buffer, jint srcOffset, jint destOffset,
         jint count, jboolean unpinned)
 {
+    int fd = jniGetFDFromFileDescriptor(env, fileDescriptor);
     if (unpinned && ashmem_pin_region(fd, 0, 0) == ASHMEM_WAS_PURGED) {
         ashmem_unpin_region(fd, 0, 0);
         jniThrowException(env, "java/io/IOException", "ashmem region was purged");
@@ -95,8 +113,9 @@ static jint android_os_MemoryFile_write(JNIEnv* env, jobject clazz,
     return count;
 }
 
-static void android_os_MemoryFile_pin(JNIEnv* env, jobject clazz, jint fd, jboolean pin)
+static void android_os_MemoryFile_pin(JNIEnv* env, jobject clazz, jobject fileDescriptor, jboolean pin)
 {
+    int fd = jniGetFDFromFileDescriptor(env, fileDescriptor);
     int result = (pin ? ashmem_pin_region(fd, 0, 0) : ashmem_unpin_region(fd, 0, 0));
     if (result < 0) {
         jniThrowException(env, "java/io/IOException", NULL);
@@ -104,12 +123,13 @@ static void android_os_MemoryFile_pin(JNIEnv* env, jobject clazz, jint fd, jbool
 }
 
 static const JNINativeMethod methods[] = {
-	{"native_open",  "(Ljava/lang/String;I)I", (void*)android_os_MemoryFile_open},
-    {"native_mmap",  "(II)I", (void*)android_os_MemoryFile_mmap},
-    {"native_close", "(I)V", (void*)android_os_MemoryFile_close},
-    {"native_read",  "(II[BIIIZ)I", (void*)android_os_MemoryFile_read},
-    {"native_write", "(II[BIIIZ)V", (void*)android_os_MemoryFile_write},
-    {"native_pin",   "(IZ)V", (void*)android_os_MemoryFile_pin},
+    {"native_open",  "(Ljava/lang/String;I)Ljava/io/FileDescriptor;", (void*)android_os_MemoryFile_open},
+    {"native_mmap",  "(Ljava/io/FileDescriptor;I)I", (void*)android_os_MemoryFile_mmap},
+    {"native_munmap", "(II)V", (void*)android_os_MemoryFile_munmap},
+    {"native_close", "(Ljava/io/FileDescriptor;)V", (void*)android_os_MemoryFile_close},
+    {"native_read",  "(Ljava/io/FileDescriptor;I[BIIIZ)I", (void*)android_os_MemoryFile_read},
+    {"native_write", "(Ljava/io/FileDescriptor;I[BIIIZ)V", (void*)android_os_MemoryFile_write},
+    {"native_pin",   "(Ljava/io/FileDescriptor;Z)V", (void*)android_os_MemoryFile_pin},
 };
 
 static const char* const kClassPathName = "android/os/MemoryFile";