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

Commit a9d41ef7 authored by Steven Moreland's avatar Steven Moreland Committed by Android Build Coastguard Worker
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)
(cherry picked from commit d668098e)
Merged-In:I34411675687cb3d18bffa082984ebdf308e1c1a6
parent a7eb8e3c

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: