Avoid leaking file descriptors when returning drop box events.

We can't use Parcel.writeValue() to write the ParcelFileDescriptor, otherwise
it leaks when returning the value to the caller (the flag gets lost).  Change
the way DropBoxManager.Entry gets serialized so that it uses a bit of its own
flags value to track whether the data is a byte[] or a ParcelFileDescriptor.

Modify the dropbox unit test to add extensive checking of Entry serialization
and deserialization under various circumstances, and to include a regression
test to ensure that FD leaking doesn't happen.

Bug: 2847738
Change-Id: I4ccd17dd03ffab234340cd359e6f3510fdf81193

diff --git a/core/java/android/os/DropBoxManager.java b/core/java/android/os/DropBoxManager.java
index 7889a92..a9f9bd7 100644 (file)
--- a/core/java/android/os/DropBoxManager.java
+++ b/core/java/android/os/DropBoxManager.java
@@ -53,6 +53,9 @@ public class DropBoxManager {
     /** Flag value: Content can be decompressed with {@link java.util.zip.GZIPOutputStream}. */
     public static final int IS_GZIPPED = 4;
 
+    /** Flag value for serialization only: Value is a byte array, not a file descriptor */
+    private static final int HAS_BYTE_ARRAY = 8;
+
     /**
      * A single entry retrieved from the drop box.
      * This may include a reference to a stream, so you must call
@@ -68,12 +71,25 @@ public class DropBoxManager {
 
         /** Create a new empty Entry with no contents. */
         public Entry(String tag, long millis) {
-            this(tag, millis, (Object) null, IS_EMPTY);
+            if (tag == null) throw new NullPointerException("tag == null");
+
+            mTag = tag;
+            mTimeMillis = millis;
+            mData = null;
+            mFileDescriptor = null;
+            mFlags = IS_EMPTY;
         }
 
         /** Create a new Entry with plain text contents. */
         public Entry(String tag, long millis, String text) {
-            this(tag, millis, (Object) text.getBytes(), IS_TEXT);
+            if (tag == null) throw new NullPointerException("tag == null");
+            if (text == null) throw new NullPointerException("text == null");
+
+            mTag = tag;
+            mTimeMillis = millis;
+            mData = text.getBytes();
+            mFileDescriptor = null;
+            mFlags = IS_TEXT;
         }
 
         /**
@@ -81,7 +97,16 @@ public class DropBoxManager {
          * The data array must not be modified after creating this entry.
          */
         public Entry(String tag, long millis, byte[] data, int flags) {
-            this(tag, millis, (Object) data, flags);
+            if (tag == null) throw new NullPointerException("tag == null");
+            if (((flags & IS_EMPTY) != 0) != (data == null)) {
+                throw new IllegalArgumentException("Bad flags: " + flags);
+            }
+
+            mTag = tag;
+            mTimeMillis = millis;
+            mData = data;
+            mFileDescriptor = null;
+            mFlags = flags;
         }
 
         /**
@@ -89,7 +114,16 @@ public class DropBoxManager {
          * Takes ownership of the ParcelFileDescriptor.
          */
         public Entry(String tag, long millis, ParcelFileDescriptor data, int flags) {
-            this(tag, millis, (Object) data, flags);
+            if (tag == null) throw new NullPointerException("tag == null");
+            if (((flags & IS_EMPTY) != 0) != (data == null)) {
+                throw new IllegalArgumentException("Bad flags: " + flags);
+            }
+
+            mTag = tag;
+            mTimeMillis = millis;
+            mData = null;
+            mFileDescriptor = data;
+            mFlags = flags;
         }
 
         /**
@@ -97,31 +131,14 @@ public class DropBoxManager {
          * The file will be read when the entry's contents are requested.
          */
         public Entry(String tag, long millis, File data, int flags) throws IOException {
-            this(tag, millis, (Object) ParcelFileDescriptor.open(
-                    data, ParcelFileDescriptor.MODE_READ_ONLY), flags);
-        }
-
-        /** Internal constructor for CREATOR.createFromParcel(). */
-        private Entry(String tag, long millis, Object value, int flags) {
-            if (tag == null) throw new NullPointerException();
-            if (((flags & IS_EMPTY) != 0) != (value == null)) throw new IllegalArgumentException();
+            if (tag == null) throw new NullPointerException("tag == null");
+            if ((flags & IS_EMPTY) != 0) throw new IllegalArgumentException("Bad flags: " + flags);
 
             mTag = tag;
             mTimeMillis = millis;
+            mData = null;
+            mFileDescriptor = ParcelFileDescriptor.open(data, ParcelFileDescriptor.MODE_READ_ONLY);
             mFlags = flags;
-
-            if (value == null) {
-                mData = null;
-                mFileDescriptor = null;
-            } else if (value instanceof byte[]) {
-                mData = (byte[]) value;
-                mFileDescriptor = null;
-            } else if (value instanceof ParcelFileDescriptor) {
-                mData = null;
-                mFileDescriptor = (ParcelFileDescriptor) value;
-            } else {
-                throw new IllegalArgumentException();
-            }
         }
 
         /** Close the input stream associated with this entry. */
@@ -149,6 +166,7 @@ public class DropBoxManager {
             InputStream is = null;
             try {
                 is = getInputStream();
+                if (is == null) return null;
                 byte[] buf = new byte[maxBytes];
                 return new String(buf, 0, Math.max(0, is.read(buf)));
             } catch (IOException e) {
@@ -174,8 +192,14 @@ public class DropBoxManager {
         public static final Parcelable.Creator<Entry> CREATOR = new Parcelable.Creator() {
             public Entry[] newArray(int size) { return new Entry[size]; }
             public Entry createFromParcel(Parcel in) {
-                return new Entry(
-                        in.readString(), in.readLong(), in.readValue(null), in.readInt());
+                String tag = in.readString();
+                long millis = in.readLong();
+                int flags = in.readInt();
+                if ((flags & HAS_BYTE_ARRAY) != 0) {
+                    return new Entry(tag, millis, in.createByteArray(), flags & ~HAS_BYTE_ARRAY);
+                } else {
+                    return new Entry(tag, millis, in.readFileDescriptor(), flags);
+                }
             }
         };
 
@@ -187,11 +211,12 @@ public class DropBoxManager {
             out.writeString(mTag);
             out.writeLong(mTimeMillis);
             if (mFileDescriptor != null) {
-                out.writeValue(mFileDescriptor);
+                out.writeInt(mFlags & ~HAS_BYTE_ARRAY);  // Clear bit just to be safe
+                mFileDescriptor.writeToParcel(out, flags);
             } else {
-                out.writeValue(mData);
+                out.writeInt(mFlags | HAS_BYTE_ARRAY);
+                out.writeByteArray(mData);
             }
-            out.writeInt(mFlags);
         }
     }
 
@@ -225,7 +250,7 @@ public class DropBoxManager {
      * @param flags describing the data
      */
     public void addData(String tag, byte[] data, int flags) {
-        if (data == null) throw new NullPointerException();
+        if (data == null) throw new NullPointerException("data == null");
         try { mService.add(new Entry(tag, 0, data, flags)); } catch (RemoteException e) {}
     }
 
@@ -239,7 +264,7 @@ public class DropBoxManager {
      * @throws IOException if the file can't be opened
      */
     public void addFile(String tag, File file, int flags) throws IOException {
-        if (file == null) throw new NullPointerException();
+        if (file == null) throw new NullPointerException("file == null");
         Entry entry = new Entry(tag, 0, file, flags);
         try {
             mService.add(new Entry(tag, 0, file, flags));

diff --git a/services/tests/servicestests/src/com/android/server/DropBoxTest.java b/services/tests/servicestests/src/com/android/server/DropBoxTest.java
index 78a90fb..f3baff4 100644 (file)
--- a/services/tests/servicestests/src/com/android/server/DropBoxTest.java
+++ b/services/tests/servicestests/src/com/android/server/DropBoxTest.java
@@ -20,6 +20,10 @@ import android.content.ContentResolver;
 import android.content.Context;
 import android.content.Intent;
 import android.os.DropBoxManager;
+import android.os.Parcel;
+import android.os.Parcelable;
+import android.os.ParcelFileDescriptor;
+import android.os.Process;
 import android.os.ServiceManager;
 import android.os.StatFs;
 import android.provider.Settings;
@@ -27,10 +31,13 @@ import android.test.AndroidTestCase;
 
 import com.android.server.DropBoxManagerService;
 
+import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.FileWriter;
+import java.io.IOException;
 import java.io.InputStream;
+import java.io.InputStreamReader;
 import java.util.Random;
 import java.util.zip.GZIPOutputStream;
 
@@ -531,6 +538,203 @@ public class DropBoxTest extends AndroidTestCase {
         service.stop();
     }
 
+    public void testDropBoxEntrySerialization() throws Exception {
+        // Make sure DropBoxManager.Entry can be serialized to a Parcel and back
+        // under a variety of conditions.
+
+        Parcel parcel = Parcel.obtain();
+        File dir = getEmptyDir("testDropBoxEntrySerialization");
+
+        new DropBoxManager.Entry("empty", 1000000).writeToParcel(parcel, 0);
+        new DropBoxManager.Entry("string", 2000000, "String Value").writeToParcel(parcel, 0);
+        new DropBoxManager.Entry("bytes", 3000000, "Bytes Value".getBytes(),
+                DropBoxManager.IS_TEXT).writeToParcel(parcel, 0);
+        new DropBoxManager.Entry("zerobytes", 4000000, new byte[0], 0).writeToParcel(parcel, 0);
+        new DropBoxManager.Entry("emptybytes", 5000000, (byte[]) null,
+                DropBoxManager.IS_EMPTY).writeToParcel(parcel, 0);
+
+        try {
+            new DropBoxManager.Entry("badbytes", 99999,
+                    "Bad Bytes Value".getBytes(),
+                    DropBoxManager.IS_EMPTY).writeToParcel(parcel, 0);
+            fail("IllegalArgumentException expected for non-null byte[] and IS_EMPTY flags");
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+
+        try {
+            new DropBoxManager.Entry("badbytes", 99999, (byte[]) null, 0).writeToParcel(parcel, 0);
+            fail("IllegalArgumentException expected for null byte[] and non-IS_EMPTY flags");
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+
+        File f = new File(dir, "file.dat");
+        FileOutputStream os = new FileOutputStream(f);
+        os.write("File Value".getBytes());
+        os.close();
+
+        new DropBoxManager.Entry("file", 6000000, f, DropBoxManager.IS_TEXT).writeToParcel(
+                parcel, Parcelable.PARCELABLE_WRITE_RETURN_VALUE);
+        new DropBoxManager.Entry("binfile", 7000000, f, 0).writeToParcel(
+                parcel, Parcelable.PARCELABLE_WRITE_RETURN_VALUE);
+        new DropBoxManager.Entry("emptyfile", 8000000, (ParcelFileDescriptor) null,
+                DropBoxManager.IS_EMPTY).writeToParcel(parcel, 0);
+
+        try {
+            new DropBoxManager.Entry("badfile", 99999, new File(dir, "nonexist.dat"), 0);
+            fail("IOException expected for nonexistent file");
+        } catch (IOException e) {
+            // expected
+        }
+
+        try {
+            new DropBoxManager.Entry("badfile", 99999, f, DropBoxManager.IS_EMPTY).writeToParcel(
+                    parcel, 0);
+            fail("IllegalArgumentException expected for non-null file and IS_EMPTY flags");
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+
+        try {
+            new DropBoxManager.Entry("badfile", 99999, (ParcelFileDescriptor) null, 0);
+            fail("IllegalArgumentException expected for null PFD and non-IS_EMPTY flags");
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+
+        File gz = new File(dir, "file.gz");
+        GZIPOutputStream gzout = new GZIPOutputStream(new FileOutputStream(gz));
+        gzout.write("Gzip File Value".getBytes());
+        gzout.close();
+
+        new DropBoxManager.Entry("gzipfile", 9000000, gz,
+                DropBoxManager.IS_TEXT | DropBoxManager.IS_GZIPPED).writeToParcel(
+                    parcel, Parcelable.PARCELABLE_WRITE_RETURN_VALUE);
+        new DropBoxManager.Entry("gzipbinfile", 10000000, gz,
+                DropBoxManager.IS_GZIPPED).writeToParcel(
+                    parcel, Parcelable.PARCELABLE_WRITE_RETURN_VALUE);
+
+        //
+        // Switch from writing to reading
+        //
+
+        parcel.setDataPosition(0);
+        DropBoxManager.Entry e;
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("empty", e.getTag());
+        assertEquals(1000000, e.getTimeMillis());
+        assertEquals(DropBoxManager.IS_EMPTY, e.getFlags());
+        assertEquals(null, e.getText(100));
+        assertEquals(null, e.getInputStream());
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("string", e.getTag());
+        assertEquals(2000000, e.getTimeMillis());
+        assertEquals(DropBoxManager.IS_TEXT, e.getFlags());
+        assertEquals("String Value", e.getText(100));
+        assertEquals("String Value",
+                new BufferedReader(new InputStreamReader(e.getInputStream())).readLine());
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("bytes", e.getTag());
+        assertEquals(3000000, e.getTimeMillis());
+        assertEquals(DropBoxManager.IS_TEXT, e.getFlags());
+        assertEquals("Bytes Value", e.getText(100));
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("zerobytes", e.getTag());
+        assertEquals(4000000, e.getTimeMillis());
+        assertEquals(0, e.getFlags());
+        assertEquals(null, e.getText(100));
+        assertEquals(null,
+                new BufferedReader(new InputStreamReader(e.getInputStream())).readLine());
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("emptybytes", e.getTag());
+        assertEquals(5000000, e.getTimeMillis());
+        assertEquals(DropBoxManager.IS_EMPTY, e.getFlags());
+        assertEquals(null, e.getText(100));
+        assertEquals(null, e.getInputStream());
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("file", e.getTag());
+        assertEquals(6000000, e.getTimeMillis());
+        assertEquals(DropBoxManager.IS_TEXT, e.getFlags());
+        assertEquals("File Value", e.getText(100));
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("binfile", e.getTag());
+        assertEquals(7000000, e.getTimeMillis());
+        assertEquals(0, e.getFlags());
+        assertEquals(null, e.getText(100));
+        assertEquals("File Value",
+                new BufferedReader(new InputStreamReader(e.getInputStream())).readLine());
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("emptyfile", e.getTag());
+        assertEquals(8000000, e.getTimeMillis());
+        assertEquals(DropBoxManager.IS_EMPTY, e.getFlags());
+        assertEquals(null, e.getText(100));
+        assertEquals(null, e.getInputStream());
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("gzipfile", e.getTag());
+        assertEquals(9000000, e.getTimeMillis());
+        assertEquals(DropBoxManager.IS_TEXT, e.getFlags());
+        assertEquals("Gzip File Value", e.getText(100));
+        e.close();
+
+        e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+        assertEquals("gzipbinfile", e.getTag());
+        assertEquals(10000000, e.getTimeMillis());
+        assertEquals(0, e.getFlags());
+        assertEquals(null, e.getText(100));
+        assertEquals("Gzip File Value",
+                new BufferedReader(new InputStreamReader(e.getInputStream())).readLine());
+        e.close();
+
+        assertEquals(0, parcel.dataAvail());
+        parcel.recycle();
+    }
+
+    public void testDropBoxEntrySerializationDoesntLeakFileDescriptors() throws Exception {
+        File dir = getEmptyDir("testDropBoxEntrySerialization");
+        File f = new File(dir, "file.dat");
+        FileOutputStream os = new FileOutputStream(f);
+        os.write("File Value".getBytes());
+        os.close();
+
+        int before = countOpenFiles();
+        assertTrue(before > 0);
+
+        for (int i = 0; i < 1000; i++) {
+            Parcel parcel = Parcel.obtain();
+            new DropBoxManager.Entry("file", 1000000, f, 0).writeToParcel(
+                    parcel, Parcelable.PARCELABLE_WRITE_RETURN_VALUE);
+
+            parcel.setDataPosition(0);
+            DropBoxManager.Entry e = DropBoxManager.Entry.CREATOR.createFromParcel(parcel);
+            assertEquals("file", e.getTag());
+            e.close();
+
+            parcel.recycle();
+        }
+
+        int after = countOpenFiles();
+        assertTrue(after > 0);
+        assertTrue(after < before + 20);
+    }
+
     private void addRandomEntry(DropBoxManager dropbox, String tag, int size) throws Exception {
         byte[] bytes = new byte[size];
         new Random(System.currentTimeMillis()).nextBytes(bytes);
@@ -564,4 +768,8 @@ public class DropBoxTest extends AndroidTestCase {
         assertTrue(dir.listFiles().length == 0);
         return dir;
     }
+
+    private int countOpenFiles() {
+        return new File("/proc/" + Process.myPid() + "/fd").listFiles().length;
+    }
 }