Do not recycle Parcel when lazy value is used
Recycling the parcel when all lazy values are consumed in a bundle may
lead to several UAF issues. However, resources tied to the parcel,
especially file descriptors, should be released as soon as possible, and
it should not wait until the next GC cycle.
To workaround this issue, we expose the destroy() method in Parcel, and
update BaseBundle's implementation to destroy the dangling parcel when
mLazyValues is zero, and never call recycle that may lead to reuse of
these Parcel instances. By doing so, we completely remove any
possibility of UAF with regards to Bundle and lazy values.
Flag: EXEMPT security fix
Test: TH
Bug: 377704076
Change-Id: Ibb28bf81f9028c18baad4e898e387a3e6192db5d
diff --git a/core/java/android/os/BaseBundle.java b/core/java/android/os/BaseBundle.java
index 2604454..dd9e569 100644
--- a/core/java/android/os/BaseBundle.java
+++ b/core/java/android/os/BaseBundle.java
@@ -457,11 +457,11 @@
if (mOwnsLazyValues) {
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
+ // No more lazy values in mMap, so we can destroy the parcel early rather than
// waiting for the next GC run
Parcel parcel = mWeakParcelledData.get();
if (mLazyValues == 0 && parcel != null) {
- recycleParcel(parcel);
+ parcel.destroy();
mWeakParcelledData = null;
}
}
@@ -516,7 +516,8 @@
mWeakParcelledData = null;
if (ownsParcel) {
if (numLazyValues[0] == 0) {
- recycleParcel(parcelledData);
+ // No lazy value, we can directly recycle this parcel
+ parcelledData.recycle();
} else {
mWeakParcelledData = new WeakReference<>(parcelledData);
}
@@ -556,12 +557,6 @@
return p == NoImagePreloadHolder.EMPTY_PARCEL;
}
- private static void recycleParcel(Parcel p) {
- if (p != null && !isEmptyParcel(p)) {
- p.recycle();
- }
- }
-
/**
* Returns the backing map of this bundle after deserializing every item.
*
@@ -667,7 +662,10 @@
public void clear() {
unparcel();
if (mOwnsLazyValues && mWeakParcelledData != null) {
- recycleParcel(mWeakParcelledData.get());
+ Parcel parcel = mWeakParcelledData.get();
+ if (parcel != null) {
+ parcel.destroy();
+ }
}
mWeakParcelledData = null;
diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java
index bb6cb08..abb9e29 100644
--- a/core/java/android/os/Parcel.java
+++ b/core/java/android/os/Parcel.java
@@ -619,7 +619,6 @@
// able to print a stack when a Parcel is recycled twice, that
// is cleared in obtain instead.
- mClassCookies = null;
freeBuffer();
if (mOwnsNativeParcelObject) {
@@ -5612,9 +5611,11 @@
nativeFreeBuffer(mNativePtr);
}
mReadWriteHelper = ReadWriteHelper.DEFAULT;
+ mClassCookies = null;
}
- private void destroy() {
+ /** @hide */
+ public void destroy() {
resetSqaushingState();
if (mNativePtr != 0) {
if (mOwnsNativeParcelObject) {
diff --git a/core/tests/mockingcoretests/src/android/os/BundleRecyclingTest.java b/core/tests/mockingcoretests/src/android/os/BundleRecyclingTest.java
index c88ab90..7994259 100644
--- a/core/tests/mockingcoretests/src/android/os/BundleRecyclingTest.java
+++ b/core/tests/mockingcoretests/src/android/os/BundleRecyclingTest.java
@@ -71,39 +71,39 @@
}
@Test
- public void bundleClear_whenParcelled_recyclesParcel() {
+ public void bundleClear_whenParcelled_destroysParcel() {
setUpBundle(/* lazy */ 1);
assertTrue(mBundle.isParcelled());
- verify(mParcelSpy, times(0)).recycle();
+ verify(mParcelSpy, times(0)).destroy();
mBundle.clear();
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
assertTrue(mBundle.isDefinitelyEmpty());
- // Should not recycle again
+ // Should not destroy again
mBundle.clear();
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
}
@Test
- public void bundleClear_whenUnparcelledWithLazy_recyclesParcel() {
+ public void bundleClear_whenUnparcelledWithLazy_destroysParcel() {
setUpBundle(/* lazy */ 1);
// Will unparcel but keep the CustomParcelable lazy
assertFalse(mBundle.isEmpty());
- verify(mParcelSpy, times(0)).recycle();
+ verify(mParcelSpy, times(0)).destroy();
mBundle.clear();
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
assertTrue(mBundle.isDefinitelyEmpty());
// Should not recycle again
mBundle.clear();
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
}
@Test
- public void bundleClear_whenClearedWithSharedParcel_doesNotRecycleParcel() {
+ public void bundleClear_whenClearedWithSharedParcel_doesNotDestroyParcel() {
setUpBundle(/* lazy */ 1);
Bundle copy = new Bundle();
@@ -115,11 +115,11 @@
copy.clear();
assertTrue(copy.isDefinitelyEmpty());
- verify(mParcelSpy, never()).recycle();
+ verify(mParcelSpy, never()).destroy();
}
@Test
- public void bundleClear_whenClearedWithCopiedParcel_doesNotRecycleParcel() {
+ public void bundleClear_whenClearedWithCopiedParcel_doesNotDestroyParcel() {
setUpBundle(/* lazy */ 1);
// Will unparcel but keep the CustomParcelable lazy
@@ -134,75 +134,75 @@
copy.clear();
assertTrue(copy.isDefinitelyEmpty());
- verify(mParcelSpy, never()).recycle();
+ verify(mParcelSpy, never()).destroy();
}
@Test
- public void bundleGet_whenUnparcelledWithLazyValueUnwrapped_recyclesParcel() {
+ public void bundleGet_whenUnparcelledWithLazyValueUnwrapped_destroysParcel() {
setUpBundle(/* lazy */ 1);
// Will unparcel with a lazy value, and immediately unwrap the lazy value,
// with no lazy values left at the end of getParcelable
// Ref counting should immediately recycle
assertNotNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
// Should not recycle again
assertNotNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
mBundle.clear();
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
}
@Test
- public void bundleGet_whenMultipleLazy_recyclesParcelWhenAllUnwrapped() {
+ public void bundleGet_whenMultipleLazy_destroysParcelWhenAllUnwrapped() {
setUpBundle(/* lazy */ 2);
assertNotNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
- verify(mParcelSpy, times(0)).recycle();
+ verify(mParcelSpy, times(0)).destroy();
assertNotNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
- verify(mParcelSpy, times(0)).recycle();
+ verify(mParcelSpy, times(0)).destroy();
assertNotNull(mBundle.getParcelable("lazy1", CustomParcelable.class));
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
// Should not recycle again
assertNotNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
mBundle.clear();
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
assertTrue(mBundle.isDefinitelyEmpty());
}
@Test
- public void bundleGet_whenLazyAndNonLazy_recyclesParcelWhenAllUnwrapped() {
+ public void bundleGet_whenLazyAndNonLazy_destroysParcelWhenAllUnwrapped() {
setUpBundle(/* lazy */ 1, /* nonLazy */ 1);
assertNotNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
- // Should not recycle again
+ // Should not destroy again
assertNotNull(mBundle.getString("nonLazy0"));
assertNotNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
mBundle.clear();
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
}
@Test
- public void bundleGet_whenLazyAndNonLazy_doesNotRecycleWhenOnlyNonLazyRetrieved() {
+ public void bundleGet_whenLazyAndNonLazy_doesNotDestroyWhenOnlyNonLazyRetrieved() {
setUpBundle(/* lazy */ 1, /* nonLazy */ 1);
assertNotNull(mBundle.getString("nonLazy0"));
- verify(mParcelSpy, times(0)).recycle();
+ verify(mParcelSpy, times(0)).destroy();
assertNotNull(mBundle.getString("nonLazy0"));
- verify(mParcelSpy, times(0)).recycle();
+ verify(mParcelSpy, times(0)).destroy();
assertNotNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
}
@Test
- public void bundleGet_withWithSharedParcel_doesNotRecycleParcel() {
+ public void bundleGet_withWithSharedParcel_doesNotDestroyParcel() {
setUpBundle(/* lazy */ 1);
Bundle copy = new Bundle();
@@ -214,17 +214,17 @@
assertNotNull(copy.getParcelable("lazy0", CustomParcelable.class));
copy.clear();
- verify(mParcelSpy, never()).recycle();
+ verify(mParcelSpy, never()).destroy();
}
@Test
- public void bundleGet_getAfterLazyCleared_doesNotRecycleAgain() {
+ public void bundleGet_getAfterLazyCleared_doesNotDestroyAgain() {
setUpBundle(/* lazy */ 1);
mBundle.clear();
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
assertNull(mBundle.getParcelable("lazy0", CustomParcelable.class));
- verify(mParcelSpy, times(1)).recycle();
+ verify(mParcelSpy, times(1)).destroy();
}
private void setUpBundle(int lazy) {
