From 7e4edf5be1025b5a01c10f6aaf5d2486f254e1ee Mon Sep 17 00:00:00 2001 From: Bernardo Rufino Date: Thu, 14 Oct 2021 15:39:52 +0100 Subject: [PATCH] Add range-based Parcel.compareData() To be used by lazy value. Now, we also remove getValueParcel() that was copying the section of the parcel since we don't need that anymore. Test: atest -d android.os.cts.ParcelTest android.os.cts.BundleTest android.os.BundleTest android.os.ParcelTest Bug: 195622897 Change-Id: Ic8a0d1b6a268a81df7a1e56fa1e4b307a25210b6 --- core/java/android/os/Parcel.java | 23 ++-- core/jni/android_os_Parcel.cpp | 20 +++ .../coretests/src/android/os/BundleTest.java | 11 +- .../coretests/src/android/os/ParcelTest.java | 129 ++++++++++++++++++ 4 files changed, 166 insertions(+), 17 deletions(-) diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java index ab2c8c0e31d3..8aec2adb4903 100644 --- a/core/java/android/os/Parcel.java +++ b/core/java/android/os/Parcel.java @@ -380,6 +380,8 @@ public final class Parcel { private static native void nativeUnmarshall( long nativePtr, byte[] data, int offset, int length); private static native int nativeCompareData(long thisNativePtr, long otherNativePtr); + private static native boolean nativeCompareDataInRange( + long ptrA, int offsetA, long ptrB, int offsetB, int length); private static native void nativeAppendFrom( long thisNativePtr, long otherNativePtr, int offset, int length); @CriticalNative @@ -677,10 +679,15 @@ public final class Parcel { } /** @hide */ - public final int compareData(Parcel other) { + public int compareData(Parcel other) { return nativeCompareData(mNativePtr, other.mNativePtr); } + /** @hide */ + public static boolean compareData(Parcel a, int offsetA, Parcel b, int offsetB, int length) { + return nativeCompareDataInRange(a.mNativePtr, offsetA, b.mNativePtr, offsetB, length); + } + /** @hide */ public final void setClassCookie(Class clz, Object cookie) { if (mClassCookies == null) { @@ -3580,7 +3587,6 @@ public final class Parcel { private final int mType; @Nullable private final ClassLoader mLoader; @Nullable private Object mObject; - @Nullable private volatile Parcel mValueParcel; /** * This goes from non-null to null once. Always check the nullability of this object before @@ -3678,7 +3684,7 @@ public final class Parcel { return false; } // Finally we compare the payload. - return getValueParcel(source).compareData(value.getValueParcel(otherSource)) == 0; + return Parcel.compareData(source, mPosition, otherSource, value.mPosition, mLength); } @Override @@ -3686,17 +3692,6 @@ public final class Parcel { // Accessing mSource first to provide memory barrier for mObject return Objects.hash(mSource == null, mObject, mLoader, mType, mLength); } - - /** This extracts the parcel section responsible for the object and returns it. */ - private Parcel getValueParcel(Parcel source) { - Parcel parcel = mValueParcel; - if (parcel == null) { - parcel = Parcel.obtain(); - parcel.appendFrom(source, mPosition, mLength); - mValueParcel = parcel; - } - return parcel; - } } /** diff --git a/core/jni/android_os_Parcel.cpp b/core/jni/android_os_Parcel.cpp index be9aaaf407db..0d530f6aa2bb 100644 --- a/core/jni/android_os_Parcel.cpp +++ b/core/jni/android_os_Parcel.cpp @@ -604,6 +604,25 @@ static jint android_os_Parcel_compareData(JNIEnv* env, jclass clazz, jlong thisN return thisParcel->compareData(*otherParcel); } +static jboolean android_os_Parcel_compareDataInRange(JNIEnv* env, jclass clazz, jlong thisNativePtr, + jint thisOffset, jlong otherNativePtr, + jint otherOffset, jint length) { + Parcel* thisParcel = reinterpret_cast(thisNativePtr); + LOG_ALWAYS_FATAL_IF(thisParcel == nullptr, "Should not be null"); + + Parcel* otherParcel = reinterpret_cast(otherNativePtr); + LOG_ALWAYS_FATAL_IF(otherParcel == nullptr, "Should not be null"); + + int result; + status_t err = + thisParcel->compareDataInRange(thisOffset, *otherParcel, otherOffset, length, &result); + if (err != NO_ERROR) { + signalExceptionForError(env, clazz, err); + return JNI_FALSE; + } + return (result == 0) ? JNI_TRUE : JNI_FALSE; +} + static void android_os_Parcel_appendFrom(JNIEnv* env, jclass clazz, jlong thisNativePtr, jlong otherNativePtr, jint offset, jint length) { @@ -841,6 +860,7 @@ static const JNINativeMethod gParcelMethods[] = { {"nativeMarshall", "(J)[B", (void*)android_os_Parcel_marshall}, {"nativeUnmarshall", "(J[BII)V", (void*)android_os_Parcel_unmarshall}, {"nativeCompareData", "(JJ)I", (void*)android_os_Parcel_compareData}, + {"nativeCompareDataInRange", "(JIJII)Z", (void*)android_os_Parcel_compareDataInRange}, {"nativeAppendFrom", "(JJII)V", (void*)android_os_Parcel_appendFrom}, // @CriticalNative {"nativeHasFileDescriptors", "(J)Z", (void*)android_os_Parcel_hasFileDescriptors}, diff --git a/core/tests/coretests/src/android/os/BundleTest.java b/core/tests/coretests/src/android/os/BundleTest.java index ddd0070588ed..09f48403cf31 100644 --- a/core/tests/coretests/src/android/os/BundleTest.java +++ b/core/tests/coretests/src/android/os/BundleTest.java @@ -273,16 +273,21 @@ public class BundleTest { Parcelable p1 = new CustomParcelable(13, "Tiramisu"); Parcelable p2 = new CustomParcelable(13, "Tiramisu"); Bundle a = new Bundle(); - a.putParcelable("key", p1); + a.putParcelable("key1", p1); a.readFromParcel(getParcelledBundle(a)); a.setClassLoader(getClass().getClassLoader()); Bundle b = new Bundle(); - b.putParcelable("key", p2); + // Adding extra element so that the position of the elements of interest in their respective + // source parcels are different so we can cover that case of Parcel.compareData(). We'll + // remove the element later so the map is equal. + b.putString("key0", "string"); + b.putParcelable("key1", p2); b.readFromParcel(getParcelledBundle(b)); b.setClassLoader(getClass().getClassLoader()); - // 2 lazy values with identical parcels inside a.isEmpty(); b.isEmpty(); + b.remove("key0"); + // 2 lazy values with identical parcels inside assertTrue(Bundle.kindofEquals(a, b)); } diff --git a/core/tests/coretests/src/android/os/ParcelTest.java b/core/tests/coretests/src/android/os/ParcelTest.java index dcb3e2f23da8..fdd278b9c621 100644 --- a/core/tests/coretests/src/android/os/ParcelTest.java +++ b/core/tests/coretests/src/android/os/ParcelTest.java @@ -17,6 +17,9 @@ package android.os; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import android.platform.test.annotations.Presubmit; @@ -110,4 +113,130 @@ public class ParcelTest { assertEquals(string, p.readString16()); } } + + @Test + public void testCompareDataInRange_whenSameData() { + Parcel pA = Parcel.obtain(); + int iA = pA.dataPosition(); + pA.writeInt(13); + pA.writeString("Tiramisu"); + int length = pA.dataPosition() - iA; + Parcel pB = Parcel.obtain(); + pB.writeString("Prefix"); + int iB = pB.dataPosition(); + pB.writeInt(13); + pB.writeString("Tiramisu"); + + assertTrue(Parcel.compareData(pA, iA, pB, iB, length)); + } + + @Test + public void testCompareDataInRange_whenSameDataWithBinder() { + Binder binder = new Binder(); + Parcel pA = Parcel.obtain(); + int iA = pA.dataPosition(); + pA.writeInt(13); + pA.writeStrongBinder(binder); + pA.writeString("Tiramisu"); + int length = pA.dataPosition() - iA; + Parcel pB = Parcel.obtain(); + pB.writeString("Prefix"); + int iB = pB.dataPosition(); + pB.writeInt(13); + pB.writeStrongBinder(binder); + pB.writeString("Tiramisu"); + + assertTrue(Parcel.compareData(pA, iA, pB, iB, length)); + } + + @Test + public void testCompareDataInRange_whenDifferentData() { + Parcel pA = Parcel.obtain(); + int iA = pA.dataPosition(); + pA.writeInt(13); + pA.writeString("Tiramisu"); + int length = pA.dataPosition() - iA; + Parcel pB = Parcel.obtain(); + int iB = pB.dataPosition(); + pB.writeString("Prefix"); + pB.writeInt(13); + pB.writeString("Tiramisu"); + + assertFalse(Parcel.compareData(pA, iA, pB, iB, length)); + } + + @Test + public void testCompareDataInRange_whenLimitOutOfBounds_throws() { + Parcel pA = Parcel.obtain(); + int iA = pA.dataPosition(); + pA.writeInt(12); + pA.writeString("Tiramisu"); + int length = pA.dataPosition() - iA; + Parcel pB = Parcel.obtain(); + pB.writeString("Prefix"); + int iB = pB.dataPosition(); + pB.writeInt(13); + pB.writeString("Tiramisu"); + pB.writeInt(-1); + + assertThrows(IllegalArgumentException.class, + () -> Parcel.compareData(pA, iA + length, pB, iB, 1)); + assertThrows(IllegalArgumentException.class, + () -> Parcel.compareData(pA, iA, pB, pB.dataSize(), 1)); + assertThrows(IllegalArgumentException.class, + () -> Parcel.compareData(pA, iA, pB, iB, length + 1)); + assertThrows(IllegalArgumentException.class, + () -> Parcel.compareData(pA, iA + length + 1, pB, iB, 0)); + assertThrows(IllegalArgumentException.class, + () -> Parcel.compareData(pA, iA, pB, iB + pB.dataSize() + 1, 0)); + } + + @Test + public void testCompareDataInRange_whenLengthZero() { + Parcel pA = Parcel.obtain(); + int iA = pA.dataPosition(); + pA.writeInt(12); + pA.writeString("Tiramisu"); + int length = pA.dataPosition() - iA; + Parcel pB = Parcel.obtain(); + pB.writeString("Prefix"); + int iB = pB.dataPosition(); + pB.writeInt(13); + pB.writeString("Tiramisu"); + + assertTrue(Parcel.compareData(pA, 0, pB, iB, 0)); + assertTrue(Parcel.compareData(pA, iA + length, pB, iB, 0)); + assertTrue(Parcel.compareData(pA, iA, pB, pB.dataSize(), 0)); + } + + @Test + public void testCompareDataInRange_whenNegativeLength_throws() { + Parcel pA = Parcel.obtain(); + int iA = pA.dataPosition(); + pA.writeInt(12); + pA.writeString("Tiramisu"); + Parcel pB = Parcel.obtain(); + pB.writeString("Prefix"); + int iB = pB.dataPosition(); + pB.writeInt(13); + pB.writeString("Tiramisu"); + + assertThrows(IllegalArgumentException.class, () -> Parcel.compareData(pA, iA, pB, iB, -1)); + } + + @Test + public void testCompareDataInRange_whenNegativeOffset_throws() { + Parcel pA = Parcel.obtain(); + int iA = pA.dataPosition(); + pA.writeInt(12); + pA.writeString("Tiramisu"); + Parcel pB = Parcel.obtain(); + pB.writeString("Prefix"); + int iB = pB.dataPosition(); + pB.writeInt(13); + pB.writeString("Tiramisu"); + + assertThrows(IllegalArgumentException.class, () -> Parcel.compareData(pA, -1, pB, iB, 0)); + assertThrows(IllegalArgumentException.class, () -> Parcel.compareData(pA, 0, pB, -1, 0)); + } }