commit f2e0a95700a937e421647623a60c9fc01d6e5d87
author Steven Moreland <smoreland@google.com> Mon Nov 01 18:17:23 2021 -0700
committer Android Build Coastguard Worker <android-build-coastguard-worker@google.com> Wed Nov 24 23:22:52 2021 +0000
tree 885dc2f1e11fffe9214829b10394e7ed9d11c2ac
parent 2085407c5de522c982f2d0c42b472739479af24c

avoid extra release of unowned objects in Parcel error path

Another bug due to a huge amount of complexity in the Parcel
implementation.

Bug: 203847542
Test: added testcase fails on device w/o Parcel.cpp fix, and it passes
  on a device with the fix
Merged-In: I34411675687cb3d18bffa082984ebdf308e1c1a6
Change-Id: I34411675687cb3d18bffa082984ebdf308e1c1a6
(cherry picked from commit 04390376b043bf6a15ff2943a9ed63d9d8173842)
(cherry picked from commit 7c8497e0127dde63957ee39e90e62b119d09948d)
Merged-In:I34411675687cb3d18bffa082984ebdf308e1c1a6

libs/binder/tests/binderLibTest.cpp

diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index 64196ba..6cb7e7f 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -94,7 +94,7 @@
     BINDER_LIB_TEST_NOP_TRANSACTION_WAIT,
     BINDER_LIB_TEST_GETPID,
     BINDER_LIB_TEST_ECHO_VECTOR,
-    BINDER_LIB_TEST_REJECT_BUF,
+    BINDER_LIB_TEST_REJECT_OBJECTS,
     BINDER_LIB_TEST_CAN_GET_SID,
 };
 
@@ -1127,13 +1127,53 @@
     memcpy(parcelData, &obj, sizeof(obj));
     data.setDataSize(sizeof(obj));
 
+    EXPECT_EQ(data.objectsCount(), 1);
+
     // Either the kernel should reject this transaction (if it's correct), but
     // if it's not, the server implementation should return an error if it
     // finds an object in the received Parcel.
-    EXPECT_THAT(server->transact(BINDER_LIB_TEST_REJECT_BUF, data, &reply),
+    EXPECT_THAT(server->transact(BINDER_LIB_TEST_REJECT_OBJECTS, data, &reply),
                 Not(StatusEq(NO_ERROR)));
 }
 
+TEST_F(BinderLibTest, WeakRejected) {
+    Parcel data, reply;
+    sp<IBinder> server = addServer();
+    ASSERT_TRUE(server != nullptr);
+
+    auto binder = sp<BBinder>::make();
+    wp<BBinder> wpBinder(binder);
+    flat_binder_object obj{
+            .hdr = {.type = BINDER_TYPE_WEAK_BINDER},
+            .flags = 0,
+            .binder = reinterpret_cast<uintptr_t>(wpBinder.get_refs()),
+            .cookie = reinterpret_cast<uintptr_t>(wpBinder.unsafe_get()),
+    };
+    data.setDataCapacity(1024);
+    // Write a bogus object at offset 0 to get an entry in the offset table
+    data.writeFileDescriptor(0);
+    EXPECT_EQ(data.objectsCount(), 1);
+    uint8_t *parcelData = const_cast<uint8_t *>(data.data());
+    // And now, overwrite it with the weak binder
+    memcpy(parcelData, &obj, sizeof(obj));
+    data.setDataSize(sizeof(obj));
+
+    // a previous bug caused other objects to be released an extra time, so we
+    // test with an object that libbinder will actually try to release
+    EXPECT_EQ(OK, data.writeStrongBinder(sp<BBinder>::make()));
+
+    EXPECT_EQ(data.objectsCount(), 2);
+
+    // send it many times, since previous error was memory corruption, make it
+    // more likely that the server crashes
+    for (size_t i = 0; i < 100; i++) {
+        EXPECT_THAT(server->transact(BINDER_LIB_TEST_REJECT_OBJECTS, data, &reply),
+                    StatusEq(BAD_VALUE));
+    }
+
+    EXPECT_THAT(server->pingBinder(), StatusEq(NO_ERROR));
+}
+
 TEST_F(BinderLibTest, GotSid) {
     sp<IBinder> server = addServer();
 
@@ -1440,7 +1480,7 @@
                 reply->writeUint64Vector(vector);
                 return NO_ERROR;
             }
-            case BINDER_LIB_TEST_REJECT_BUF: {
+            case BINDER_LIB_TEST_REJECT_OBJECTS: {
                 return data.objectsCount() == 0 ? BAD_VALUE : NO_ERROR;
             }
             case BINDER_LIB_TEST_CAN_GET_SID: {