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

Commit 9da89484 authored by Hani Kazmi's avatar Hani Kazmi
Browse files

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
parent bb3501c4

core/java/android/os/BaseBundle.java

+18 −2
Original line number Diff line number Diff line
@@ -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,