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

Commit 7c296d75 authored by Bernardo Rufino's avatar Bernardo Rufino
Browse files

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
parent 31d70842

core/java/android/os/BaseBundle.java

+53 −47
Original line number Diff line number Diff line
@@ -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} */

core/java/android/os/Bundle.java

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

    }



    /**

core/java/android/os/PersistableBundle.java

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

    }



    /**