From 9da89484bdae926726688dc164b911ca44e6da83 Mon Sep 17 00:00:00 2001
From: Hani Kazmi <hanikazmi@google.com>
Date: Tue, 23 Aug 2022 10:41:17 +0000
Subject: [PATCH] Add a lock when unwrapping lazy value in bundles

This commit addresses a race condition introduced in ag/18909939. If
multiple threads try to read the same lazy value out of mMap in
parallel, it is possible for both to decrement mLazyValues, thus failing
the precondition check, or recycling the parcel twice.

While bundles do not guarantee thread safety, there have already been
enough reports of impacted usecases that it is worth mitigating at the
root.

It is possible to shorten the lock to not include the recycle block, but
both implementations did not show any performance difference
in preliminary testing.

Bug: 241888992
Test: Reproduced race condition on physical device, and validated that
code change fixes it.
Test: atest android.os.cts.ParcelTest android.os.cts.BundleTest
android.os.BundleTest android.os.ParcelTest
android.os.BundleRecylingTest

Change-Id: I5d261d4315a123008b226c364fec73634e08a150
---
 core/java/android/os/BaseBundle.java | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/core/java/android/os/BaseBundle.java b/core/java/android/os/BaseBundle.java
index 87579eb208902..4e3adfbdda3f4 100644
--- a/core/java/android/os/BaseBundle.java
+++ b/core/java/android/os/BaseBundle.java
@@ -27,6 +27,7 @@ import android.util.MathUtils;
 import android.util.Slog;
 import android.util.SparseArray;
 
+import com.android.internal.annotations.GuardedBy;
 import com.android.internal.annotations.VisibleForTesting;
 import com.android.internal.util.IndentingPrintWriter;
 import com.android.internal.util.Preconditions;
@@ -395,6 +396,20 @@ public class BaseBundle {
     @Nullable
     final <T> T getValueAt(int i, @Nullable Class<T> clazz, @Nullable Class<?>... itemTypes) {
         Object object = mMap.valueAt(i);
+        if (object instanceof BiFunction<?, ?, ?>) {
+            synchronized (this) {
+                object = unwrapLazyValueFromMapLocked(i, clazz, itemTypes);
+            }
+        }
+        return (clazz != null) ? clazz.cast(object) : (T) object;
+    }
+
+    @SuppressWarnings("unchecked")
+    @Nullable
+    @GuardedBy("this")
+    private Object unwrapLazyValueFromMapLocked(int i, @Nullable Class<?> clazz,
+            @Nullable Class<?>... itemTypes) {
+        Object object = mMap.valueAt(i);
         if (object instanceof BiFunction<?, ?, ?>) {
             try {
                 object = ((BiFunction<Class<?>, Class<?>[], ?>) object).apply(clazz, itemTypes);
@@ -409,7 +424,8 @@ public class BaseBundle {
             mMap.setValueAt(i, object);
             mLazyValues--;
             if (mOwnsLazyValues) {
-                Preconditions.checkState(mLazyValues >= 0, "Lazy values ref count below 0");
+                Preconditions.checkState(mLazyValues >= 0,
+                        "Lazy values ref count below 0");
                 // No more lazy values in mMap, so we can recycle the parcel early rather than
                 // waiting for the next GC run
                 if (mLazyValues == 0) {
@@ -420,7 +436,7 @@ public class BaseBundle {
                 }
             }
         }
-        return (clazz != null) ? clazz.cast(object) : (T) object;
+        return object;
     }
 
     private void initializeFromParcelLocked(@NonNull Parcel parcelledData, boolean ownsParcel,