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

1 files changed, 57 insertions, 32 deletions

diff --git a/core/java/android/os/DropBoxManager.java b/core/java/android/os/DropBoxManager.java
index 7889a92..a9f9bd7 100644
--- 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));