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 d668098e4714025b41052207c9332de86dc3936a)
Merged-In:I34411675687cb3d18bffa082984ebdf308e1c1a6

2 files changed, 47 insertions, 6 deletions

diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 64a4f9bf6d..f790b88e8d 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -2334,12 +2334,14 @@ void Parcel::ipcSetDataReference(const uint8_t* data, size_t dataSize,
               type == BINDER_TYPE_FD)) {
             // We should never receive other types (eg BINDER_TYPE_FDA) as long as we don't support
             // them in libbinder. If we do receive them, it probably means a kernel bug; try to
-            // recover gracefully by clearing out the objects, and releasing the objects we do
-            // know about.
+            // recover gracefully by clearing out the objects.
             android_errorWriteLog(0x534e4554, "135930648");
+            android_errorWriteLog(0x534e4554, "203847542");
             ALOGE("%s: unsupported type object (%" PRIu32 ") at offset %" PRIu64 "\n",
                   __func__, type, (uint64_t)offset);
-            releaseObjects();
+
+            // WARNING: callers of ipcSetDataReference need to make sure they
+            // don't rely on mObjectsSize in their release_func.
             mObjectsSize = 0;
             break;
         }
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index 98f0868bca..9caad451d8 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -83,7 +83,7 @@ enum BinderLibTestTranscationCode {
     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,
 };
 
 pid_t start_server_process(int arg2, bool usePoll = false)
@@ -1116,14 +1116,53 @@ TEST_F(BinderLibTest, BufRejected) {
     // And now, overwrite it with the buffer object
     memcpy(parcelData, &obj, sizeof(obj));
     data.setDataSize(sizeof(obj));
+    EXPECT_EQ(data.objectsCount(), 1);
+
+    status_t ret = server->transact(BINDER_LIB_TEST_REJECT_OBJECTS, data, &reply);
 
-    status_t ret = server->transact(BINDER_LIB_TEST_REJECT_BUF, data, &reply);
     // 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_NE(NO_ERROR, ret);
 }
 
+TEST_F(BinderLibTest, WeakRejected) {
+    Parcel data, reply;
+    sp<IBinder> server = addServer();
+    ASSERT_TRUE(server != nullptr);
+
+    sp<BBinder> binder = new BBinder();
+    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(new BBinder()));
+
+    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_EQ(BAD_VALUE, server->transact(BINDER_LIB_TEST_REJECT_OBJECTS, data, &reply));
+    }
+
+    EXPECT_EQ(NO_ERROR, server->pingBinder());
+}
+
 class BinderLibTestService : public BBinder
 {
     public:
@@ -1422,7 +1461,7 @@ class BinderLibTestService : public BBinder
                 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;
             }
             default: