From 85b74b7e074f4bf6d1b9b4fd0fe422ba95dddf11 Mon Sep 17 00:00:00 2001
From: Steven Moreland <smoreland@google.com>
Date: Mon, 1 Nov 2021 18:17:23 -0700
Subject: 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/Parcel.cpp              |  8 ++++---
 libs/binder/tests/binderLibTest.cpp | 46 ++++++++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index ee834ea43c..617708f3d4 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -2141,12 +2141,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 64196ba30f..6cb7e7f104 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -94,7 +94,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,
     BINDER_LIB_TEST_CAN_GET_SID,
 };
 
@@ -1127,13 +1127,53 @@ TEST_F(BinderLibTest, BufRejected) {
     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 @@ 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;
             }
             case BINDER_LIB_TEST_CAN_GET_SID: {
-- 
cgit v1.2.3