diff options
author | Steven Moreland <smoreland@google.com> | 2021-11-01 18:17:23 -0700 |
---|---|---|
committer | Android Build Coastguard Worker <android-build-coastguard-worker@google.com> | 2021-11-12 05:37:06 +0000 |
commit | a9d41ef7a9a7e4056cf3729146ecc94a3a9ce038 (patch) | |
tree | cf19116ed2481f070929a89296c2fbeef772332a | |
parent | a7eb8e3c0ee2cded715b980033b2e713da26c809 (diff) | |
download | native-a9d41ef7a9a7e4056cf3729146ecc94a3a9ce038.tar.gz |
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
-rw-r--r-- | libs/binder/Parcel.cpp | 8 | ||||
-rw-r--r-- | libs/binder/tests/binderLibTest.cpp | 45 |
2 files changed, 47 insertions, 6 deletions
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index b7ad660317..efcc042b3c 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -2322,12 +2322,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 40de2db2cb..b37b688a5a 100644 --- a/libs/binder/tests/binderLibTest.cpp +++ b/libs/binder/tests/binderLibTest.cpp @@ -76,7 +76,7 @@ enum BinderLibTestTranscationCode { BINDER_LIB_TEST_CREATE_BINDER_TRANSACTION, BINDER_LIB_TEST_GET_WORK_SOURCE_TRANSACTION, 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) @@ -1050,14 +1050,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: @@ -1340,7 +1379,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: |