From 9e26212dac8bd00a4bd881ba87a0a5e7fb7ed921 Mon Sep 17 00:00:00 2001 From: Bernardo Rufino Date: Mon, 27 Sep 2021 17:28:38 +0100 Subject: [PATCH] Avoid full unparcelling where possible in Bundle and put a warning where it's not possible. This is to make sure we know when to touch bundles provided by apps. In this CL: * deepCopy() now doesn't deserialize lazy objects before copying. It doesn't copy them either, it merely passes them into the new map as the same reference. This works because we implemented fine-grained locking into each lazy value, so concurrent access won't be a problem. Furthermore, LazyValue caches the deserialized object, so we'd still honor the contract of deepCopy() that "Other types of objects (such as Parcelable or Serializable) are referenced as-is and not copied in any way". I had to perform one extra check in the synchronized block in LazyValue to guarantee this (double-checked locking), I explain that in the comments. * Removed filterValues() and codepaths that used it. This was created with the purpose of removing items whose classes weren't available to the system to prevent crashes coming from full deserialization. This is not a concern anymore with lazy bundle, hence we can remove the codepaths altogether (see email for more details). * Put warnings in javadoc of getMap() and PeristableBundle(). Test: Boots Test: atest -d android.os.cts.ParcelTest android.os.cts.BundleTest android.os.BundleTest android.os.ParcelTest Bug: 195622897 Change-Id: I14bb6a7874814f42cbcc6b5fd372c42752aa74c8 --- core/java/android/app/ActivityThread.java | 1 - core/java/android/content/Intent.java | 10 ----- core/java/android/os/BaseBundle.java | 16 +++++-- core/java/android/os/Bundle.java | 50 --------------------- core/java/android/os/Parcel.java | 17 ++++--- core/java/android/os/PersistableBundle.java | 7 ++- 6 files changed, 28 insertions(+), 73 deletions(-) diff --git a/core/java/android/app/ActivityThread.java b/core/java/android/app/ActivityThread.java index 749e8f548fabe..7c140d6181a67 100644 --- a/core/java/android/app/ActivityThread.java +++ b/core/java/android/app/ActivityThread.java @@ -3724,7 +3724,6 @@ public final class ActivityThread extends ClientTransactionHandler { Intent intent = new Intent(activityIntent); intent.setFlags(intent.getFlags() & ~(Intent.FLAG_GRANT_WRITE_URI_PERMISSION | Intent.FLAG_GRANT_PERSISTABLE_URI_PERMISSION)); - intent.removeUnsafeExtras(); content.setDefaultIntent(intent); } } else { diff --git a/core/java/android/content/Intent.java b/core/java/android/content/Intent.java index 0fad63f192bba..9746066695753 100644 --- a/core/java/android/content/Intent.java +++ b/core/java/android/content/Intent.java @@ -8567,16 +8567,6 @@ public class Intent implements Parcelable, Cloneable { : null; } - /** - * Filter extras to only basic types. - * @hide - */ - public void removeUnsafeExtras() { - if (mExtras != null) { - mExtras = mExtras.filterValues(); - } - } - /** * @return Whether {@link #maybeStripForHistory} will return an lightened intent or * return itself as-is. diff --git a/core/java/android/os/BaseBundle.java b/core/java/android/os/BaseBundle.java index 64d54b820a92b..c988cd7c9362d 100644 --- a/core/java/android/os/BaseBundle.java +++ b/core/java/android/os/BaseBundle.java @@ -376,8 +376,16 @@ public class BaseBundle { } } - /** @hide */ - ArrayMap getMap() { + /** + * Returns the backing map of this bundle after deserializing every item. + * + *

Warning: This method will deserialize every item on the bundle, including custom + * types such as {@link Parcelable} and {@link Serializable}, so only use this when you trust + * the source. Specifically don't use this method on app-provided bundles. + * + * @hide + */ + ArrayMap getItemwiseMap() { unparcel(/* itemwise */ true); return mMap; } @@ -500,7 +508,7 @@ public class BaseBundle { final int N = fromMap.size(); mMap = new ArrayMap<>(N); for (int i = 0; i < N; i++) { - mMap.append(fromMap.keyAt(i), deepCopyValue(from.getValueAt(i))); + mMap.append(fromMap.keyAt(i), deepCopyValue(fromMap.valueAt(i))); } } } else { @@ -1772,7 +1780,7 @@ public class BaseBundle { pw.println("[null]"); return; } - final ArrayMap map = bundle.getMap(); + final ArrayMap map = bundle.getItemwiseMap(); for (int i = 0; i < map.size(); i++) { dumpStats(pw, map.keyAt(i), map.valueAt(i)); } diff --git a/core/java/android/os/Bundle.java b/core/java/android/os/Bundle.java index 5626bde6c926b..b3827b3106b80 100644 --- a/core/java/android/os/Bundle.java +++ b/core/java/android/os/Bundle.java @@ -347,56 +347,6 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable { return (mFlags & FLAG_HAS_FDS) != 0; } - /** - * Filter values in Bundle to only basic types. - * @hide - */ - @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) - public Bundle filterValues() { - unparcel(/* itemwise */ true); - Bundle bundle = this; - if (mMap != null) { - ArrayMap map = mMap; - for (int i = map.size() - 1; i >= 0; i--) { - Object value = map.valueAt(i); - if (PersistableBundle.isValidType(value)) { - continue; - } - if (value instanceof Bundle) { - Bundle newBundle = ((Bundle)value).filterValues(); - if (newBundle != value) { - if (map == mMap) { - // The filter had to generate a new bundle, but we have not yet - // created a new one here. Do that now. - bundle = new Bundle(this); - // Note the ArrayMap<> constructor is guaranteed to generate - // a new object with items in the same order as the original. - map = bundle.mMap; - } - // Replace this current entry with the new child bundle. - map.setValueAt(i, newBundle); - } - continue; - } - if (value.getClass().getName().startsWith("android.")) { - continue; - } - if (map == mMap) { - // This is the first time we have had to remove something, that means we - // need to switch to a new Bundle. - bundle = new Bundle(this); - // Note the ArrayMap<> constructor is guaranteed to generate - // a new object with items in the same order as the original. - map = bundle.mMap; - } - map.removeAt(i); - } - } - mFlags |= FLAG_HAS_FDS_KNOWN; - mFlags &= ~FLAG_HAS_FDS; - return bundle; - } - /** {@hide} */ @Override public void putObject(@Nullable String key, @Nullable Object value) { diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java index 9af8e6ff3451b..17a227db2dd16 100644 --- a/core/java/android/os/Parcel.java +++ b/core/java/android/os/Parcel.java @@ -3470,14 +3470,17 @@ public final class Parcel { Parcel source = mSource; if (source != null) { synchronized (source) { - int restore = source.dataPosition(); - try { - source.setDataPosition(mPosition); - mObject = source.readValue(mLoader); - } finally { - source.setDataPosition(restore); + // Check mSource != null guarantees callers won't ever see different objects. + if (mSource != null) { + int restore = source.dataPosition(); + try { + source.setDataPosition(mPosition); + mObject = source.readValue(mLoader); + } finally { + source.setDataPosition(restore); + } + mSource = null; } - mSource = null; } } return mObject; diff --git a/core/java/android/os/PersistableBundle.java b/core/java/android/os/PersistableBundle.java index 339371b5047c2..c7edbec6749da 100644 --- a/core/java/android/os/PersistableBundle.java +++ b/core/java/android/os/PersistableBundle.java @@ -34,6 +34,7 @@ import org.xmlpull.v1.XmlSerializer; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.io.Serializable; import java.util.ArrayList; /** @@ -100,6 +101,10 @@ public final class PersistableBundle extends BaseBundle implements Cloneable, Pa /** * Constructs a PersistableBundle from a Bundle. Does only a shallow copy of the Bundle. * + *

Warning: This method will deserialize every item on the bundle, including custom + * types such as {@link Parcelable} and {@link Serializable}, so only use this when you trust + * the source. Specifically don't use this method on app-provided bundles. + * * @param b a Bundle to be copied. * * @throws IllegalArgumentException if any element of {@code b} cannot be persisted. @@ -107,7 +112,7 @@ public final class PersistableBundle extends BaseBundle implements Cloneable, Pa * @hide */ public PersistableBundle(Bundle b) { - this(b.getMap()); + this(b.getItemwiseMap()); } /**