Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d668098e authored by Steven Moreland's avatar Steven Moreland
Browse files

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 04390376)
parent 62eaabc3

libs/binder/Parcel.cpp

+5 −3
Original line number Diff line number Diff line
@@ -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;

        }

libs/binder/tests/binderLibTest.cpp

+42 −3
Original line number Diff line number Diff line
@@ -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: