From 7c296d7514fe9d9f237c43b3554a2b5646035428 Mon Sep 17 00:00:00 2001
From: Bernardo Rufino <brufino@google.com>
Date: Tue, 2 Nov 2021 10:41:12 +0000
Subject: [PATCH] Refactor Bundle for thread-safety of
 Bundle.hasFileDescriptors()

After aosp/1878338, I realized Bundle.hasFileDescriptors() was accessing
mParcelledData and mMap without a lock. So, to avoid locking
there, I made mParcelledData volatile (since after mParcelledData is
null, it can never be non-null again and mMap is guaranteed to be
non-null). So, moved assignments to mParcelledData to be last wrt mMap
and mParcelledByNative to ensure visibility of those fields.

While refactoring copyInternal(), realized that method was always used
after constructing an uninitialized Bundle, so to avoid mistakes with
objects not properly initialized, I created a constructor in Bundle,
PersistableBundle & BaseBundle that does that copyInternal() did.

Bug: 195622897
Test: atest -d android.os.cts.ParcelTest android.os.cts.BundleTest android.os.BundleTest android.os.ParcelTest
Change-Id: I5c9337496da7ecf87f10370726099dcb247a6789
---
 core/java/android/os/BaseBundle.java        | 100 +++++++++++---------
 core/java/android/os/Bundle.java            |  23 +++--
 core/java/android/os/PersistableBundle.java |  15 +--
 3 files changed, 75 insertions(+), 63 deletions(-)

diff --git a/core/java/android/os/BaseBundle.java b/core/java/android/os/BaseBundle.java
index 2a344f6c2b6ba..ad3de25fecc23 100644
--- a/core/java/android/os/BaseBundle.java
+++ b/core/java/android/os/BaseBundle.java
@@ -103,7 +103,7 @@ public class BaseBundle {
      * are unparcelled, mParcelledData willbe set to null.
      */
     @UnsupportedAppUsage
-    Parcel mParcelledData = null;
+    volatile Parcel mParcelledData = null;
 
     /**
      * Whether {@link #mParcelledData} was generated by native code or not.
@@ -182,13 +182,56 @@ public class BaseBundle {
      * @param b a Bundle to be copied.
      */
     BaseBundle(BaseBundle b) {
-        copyInternal(b, false);
+        this(b, /* deep */ false);
     }
 
     /**
-     * Special constructor that does not initialize the bundle.
+     * Constructs a {@link BaseBundle} containing a copy of {@code from}.
+     *
+     * @param from The bundle to be copied.
+     * @param deep Whether is a deep or shallow copy.
+     *
+     * @hide
      */
-    BaseBundle(boolean doInit) {
+    BaseBundle(BaseBundle from, boolean deep) {
+        synchronized (from) {
+            mClassLoader = from.mClassLoader;
+
+            if (from.mMap != null) {
+                if (!deep) {
+                    mMap = new ArrayMap<>(from.mMap);
+                } else {
+                    final ArrayMap<String, Object> fromMap = from.mMap;
+                    final int n = fromMap.size();
+                    mMap = new ArrayMap<>(n);
+                    for (int i = 0; i < n; i++) {
+                        mMap.append(fromMap.keyAt(i), deepCopyValue(fromMap.valueAt(i)));
+                    }
+                }
+            } else {
+                mMap = null;
+            }
+
+            final Parcel parcelledData;
+            if (from.mParcelledData != null) {
+                if (from.isEmptyParcel()) {
+                    parcelledData = NoImagePreloadHolder.EMPTY_PARCEL;
+                    mParcelledByNative = false;
+                } else {
+                    parcelledData = Parcel.obtain();
+                    parcelledData.appendFrom(from.mParcelledData, 0,
+                            from.mParcelledData.dataSize());
+                    parcelledData.setDataPosition(0);
+                    mParcelledByNative = from.mParcelledByNative;
+                }
+            } else {
+                parcelledData = null;
+                mParcelledByNative = false;
+            }
+
+            // Keep as last statement to ensure visibility of other fields
+            mParcelledData = parcelledData;
+        }
     }
 
     /**
@@ -323,8 +366,8 @@ public class BaseBundle {
             } else {
                 mMap.erase();
             }
-            mParcelledData = null;
             mParcelledByNative = false;
+            mParcelledData = null;
             return;
         }
 
@@ -358,8 +401,8 @@ public class BaseBundle {
             if (recycleParcel) {
                 recycleParcel(parcelledData);
             }
-            mParcelledData = null;
             mParcelledByNative = false;
+            mParcelledData = null;
         }
         if (DEBUG) {
             Log.d(TAG, "unparcel " + Integer.toHexString(System.identityHashCode(this))
@@ -501,44 +544,7 @@ public class BaseBundle {
         mMap.clear();
     }
 
-    void copyInternal(BaseBundle from, boolean deep) {
-        synchronized (from) {
-            if (from.mParcelledData != null) {
-                if (from.isEmptyParcel()) {
-                    mParcelledData = NoImagePreloadHolder.EMPTY_PARCEL;
-                    mParcelledByNative = false;
-                } else {
-                    mParcelledData = Parcel.obtain();
-                    mParcelledData.appendFrom(from.mParcelledData, 0,
-                            from.mParcelledData.dataSize());
-                    mParcelledData.setDataPosition(0);
-                    mParcelledByNative = from.mParcelledByNative;
-                }
-            } else {
-                mParcelledData = null;
-                mParcelledByNative = false;
-            }
-
-            if (from.mMap != null) {
-                if (!deep) {
-                    mMap = new ArrayMap<>(from.mMap);
-                } else {
-                    final ArrayMap<String, Object> fromMap = from.mMap;
-                    final int N = fromMap.size();
-                    mMap = new ArrayMap<>(N);
-                    for (int i = 0; i < N; i++) {
-                        mMap.append(fromMap.keyAt(i), deepCopyValue(fromMap.valueAt(i)));
-                    }
-                }
-            } else {
-                mMap = null;
-            }
-
-            mClassLoader = from.mClassLoader;
-        }
-    }
-
-    Object deepCopyValue(Object value) {
+    private Object deepCopyValue(Object value) {
         if (value == null) {
             return null;
         }
@@ -570,7 +576,7 @@ public class BaseBundle {
         return value;
     }
 
-    ArrayList deepcopyArrayList(ArrayList from) {
+    private ArrayList deepcopyArrayList(ArrayList from) {
         final int N = from.size();
         ArrayList out = new ArrayList(N);
         for (int i=0; i<N; i++) {
@@ -1717,9 +1723,9 @@ public class BaseBundle {
         if (length < 0) {
             throw new RuntimeException("Bad length in parcel: " + length);
         } else if (length == 0) {
+            mParcelledByNative = false;
             // Empty Bundle or end of data.
             mParcelledData = NoImagePreloadHolder.EMPTY_PARCEL;
-            mParcelledByNative = false;
             return;
         } else if (length % 4 != 0) {
             throw new IllegalStateException("Bundle length is not aligned by 4: " + length);
@@ -1757,8 +1763,8 @@ public class BaseBundle {
                 + ": " + length + " bundle bytes starting at " + offset);
         p.setDataPosition(0);
 
-        mParcelledData = p;
         mParcelledByNative = isNativeBundle;
+        mParcelledData = p;
     }
 
     /** {@hide} */
diff --git a/core/java/android/os/Bundle.java b/core/java/android/os/Bundle.java
index feca81e1aea1b..b2bbfd6c163de 100644
--- a/core/java/android/os/Bundle.java
+++ b/core/java/android/os/Bundle.java
@@ -101,6 +101,18 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable {
         maybePrefillHasFds();
     }
 
+    /**
+     * Constructs a {@link Bundle} containing a copy of {@code from}.
+     *
+     * @param from The bundle to be copied.
+     * @param deep Whether is a deep or shallow copy.
+     *
+     * @hide
+     */
+    Bundle(Bundle from, boolean deep) {
+        super(from, deep);
+    }
+
     /**
      * If {@link #mParcelledData} is not null, copy the HAS FDS bit from it because it's fast.
      * Otherwise (if {@link #mParcelledData} is already null), leave {@link #FLAG_HAS_FDS_KNOWN}
@@ -166,13 +178,6 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable {
         mFlags = FLAG_HAS_FDS_KNOWN | FLAG_ALLOW_FDS;
     }
 
-    /**
-     * Constructs a Bundle without initializing it.
-     */
-    Bundle(boolean doInit) {
-        super(doInit);
-    }
-
     /**
      * Make a Bundle for a single key/value pair.
      *
@@ -260,9 +265,7 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable {
      * are referenced as-is and not copied in any way.
      */
     public Bundle deepCopy() {
-        Bundle b = new Bundle(false);
-        b.copyInternal(this, true);
-        return b;
+        return new Bundle(this, /* deep */ true);
     }
 
     /**
diff --git a/core/java/android/os/PersistableBundle.java b/core/java/android/os/PersistableBundle.java
index 7b55e710caeb3..f4edcb1317f44 100644
--- a/core/java/android/os/PersistableBundle.java
+++ b/core/java/android/os/PersistableBundle.java
@@ -156,10 +156,15 @@ public final class PersistableBundle extends BaseBundle implements Cloneable, Pa
     }
 
     /**
-     * Constructs a PersistableBundle without initializing it.
+     * Constructs a {@link PersistableBundle} containing a copy of {@code from}.
+     *
+     * @param from The bundle to be copied.
+     * @param deep Whether is a deep or shallow copy.
+     *
+     * @hide
      */
-    PersistableBundle(boolean doInit) {
-        super(doInit);
+    PersistableBundle(PersistableBundle from, boolean deep) {
+        super(from, deep);
     }
 
     /**
@@ -190,9 +195,7 @@ public final class PersistableBundle extends BaseBundle implements Cloneable, Pa
      * are referenced as-is and not copied in any way.
      */
     public PersistableBundle deepCopy() {
-        PersistableBundle b = new PersistableBundle(false);
-        b.copyInternal(this, true);
-        return b;
+        return new PersistableBundle(this, /* deep */ true);
     }
 
     /**