From d8e877d27d8e5a87446b9e1030325edad268d9e3 Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Tue, 3 May 2016 16:57:54 -0700 Subject: [PATCH] Maybe fix issue #28457907: Pebble app crash + reboot Fix this long-standing multi-threading issue in Bundle when multiple threads are trying to read from a Bundle and conflict to due unparceling. There are two critical sections this protects: writing the bundle in to a parcel (when it is doing this from the bundle's already parcelled representation), and unparcelling a bundle into its map of entries. Change-Id: I5470002f090e63dd623a573da6c204d3b5b661f4 --- core/java/android/os/BaseBundle.java | 110 +++++++++++++++------------ 1 file changed, 60 insertions(+), 50 deletions(-) diff --git a/core/java/android/os/BaseBundle.java b/core/java/android/os/BaseBundle.java index 004b2ceab673b..e83c756786e72 100644 --- a/core/java/android/os/BaseBundle.java +++ b/core/java/android/os/BaseBundle.java @@ -169,7 +169,7 @@ public class BaseBundle { } if (b.mMap != null) { - mMap = new ArrayMap(b.mMap); + mMap = new ArrayMap<>(b.mMap); } else { mMap = null; } @@ -226,56 +226,62 @@ public class BaseBundle { * using the currently assigned class loader. */ /* package */ synchronized void unparcel() { - if (mParcelledData == null) { - if (DEBUG) Log.d(TAG, "unparcel " + Integer.toHexString(System.identityHashCode(this)) - + ": no parcelled data"); - return; - } - - if (LOG_DEFUSABLE && sShouldDefuse && (mFlags & FLAG_DEFUSABLE) == 0) { - Slog.wtf(TAG, "Attempting to unparcel a Bundle while in transit; this may " - + "clobber all data inside!", new Throwable()); - } - - if (isEmptyParcel()) { - if (DEBUG) Log.d(TAG, "unparcel " + Integer.toHexString(System.identityHashCode(this)) - + ": empty"); - if (mMap == null) { - mMap = new ArrayMap(1); - } else { - mMap.erase(); + synchronized (this) { + final Parcel parcelledData = mParcelledData; + if (parcelledData == null) { + if (DEBUG) Log.d(TAG, "unparcel " + + Integer.toHexString(System.identityHashCode(this)) + + ": no parcelled data"); + return; } - mParcelledData = null; - return; - } - int N = mParcelledData.readInt(); - if (DEBUG) Log.d(TAG, "unparcel " + Integer.toHexString(System.identityHashCode(this)) - + ": reading " + N + " maps"); - if (N < 0) { - return; - } - if (mMap == null) { - mMap = new ArrayMap(N); - } else { - mMap.erase(); - mMap.ensureCapacity(N); - } - try { - mParcelledData.readArrayMapInternal(mMap, N, mClassLoader); - } catch (BadParcelableException e) { - if (sShouldDefuse) { - Log.w(TAG, "Failed to parse Bundle, but defusing quietly", e); - mMap.erase(); - } else { - throw e; + if (LOG_DEFUSABLE && sShouldDefuse && (mFlags & FLAG_DEFUSABLE) == 0) { + Slog.wtf(TAG, "Attempting to unparcel a Bundle while in transit; this may " + + "clobber all data inside!", new Throwable()); } - } finally { - mParcelledData.recycle(); - mParcelledData = null; + + if (isEmptyParcel()) { + if (DEBUG) Log.d(TAG, "unparcel " + + Integer.toHexString(System.identityHashCode(this)) + ": empty"); + if (mMap == null) { + mMap = new ArrayMap<>(1); + } else { + mMap.erase(); + } + mParcelledData = null; + return; + } + + int N = parcelledData.readInt(); + if (DEBUG) Log.d(TAG, "unparcel " + Integer.toHexString(System.identityHashCode(this)) + + ": reading " + N + " maps"); + if (N < 0) { + return; + } + ArrayMap map = mMap; + if (map == null) { + map = new ArrayMap<>(N); + } else { + map.erase(); + map.ensureCapacity(N); + } + try { + parcelledData.readArrayMapInternal(map, N, mClassLoader); + } catch (BadParcelableException e) { + if (sShouldDefuse) { + Log.w(TAG, "Failed to parse Bundle, but defusing quietly", e); + map.erase(); + } else { + throw e; + } + } finally { + mMap = map; + parcelledData.recycle(); + mParcelledData = null; + } + if (DEBUG) Log.d(TAG, "unparcel " + Integer.toHexString(System.identityHashCode(this)) + + " final map: " + mMap); } - if (DEBUG) Log.d(TAG, "unparcel " + Integer.toHexString(System.identityHashCode(this)) - + " final map: " + mMap); } /** @@ -1375,14 +1381,18 @@ public class BaseBundle { void writeToParcelInner(Parcel parcel, int flags) { // Keep implementation in sync with writeToParcel() in // frameworks/native/libs/binder/PersistableBundle.cpp. - if (mParcelledData != null) { + final Parcel parcelledData; + synchronized (this) { + parcelledData = mParcelledData; + } + if (parcelledData != null) { if (isEmptyParcel()) { parcel.writeInt(0); } else { - int length = mParcelledData.dataSize(); + int length = parcelledData.dataSize(); parcel.writeInt(length); parcel.writeInt(BUNDLE_MAGIC); - parcel.appendFrom(mParcelledData, 0, length); + parcel.appendFrom(parcelledData, 0, length); } } else { // Special case for empty bundles.