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

Commit 53b6ffe5 authored by Frederick Mayle's avatar Frederick Mayle
Browse files

libbinder: Remove Parcel argument from Parcel::release_func 

This enforces a clearer separation of concerns between the data owner
and the parcel code. It happened to reveal an edge case where FDs are
prematurely closed (tracking a fix in b/239222407).

Test: TH
Bug: 239222407
Change-Id: I54ff0c8e4a8f64afd529092d2038dac40c853371
parent ec602d42

libs/binder/IPCThreadState.cpp

+9 −16
Original line number Diff line number Diff line
@@ -972,16 +972,13 @@ status_t IPCThreadState::waitForResponse(Parcel *reply, status_t *acquireResult) 
                            freeBuffer);

                    } else {

                        err = *reinterpret_cast<const status_t*>(tr.data.ptr.buffer);

                        freeBuffer(nullptr,

                            reinterpret_cast<const uint8_t*>(tr.data.ptr.buffer),

                        freeBuffer(reinterpret_cast<const uint8_t*>(tr.data.ptr.buffer),

                                   tr.data_size,

                                   reinterpret_cast<const binder_size_t*>(tr.data.ptr.offsets),

                                   tr.offsets_size / sizeof(binder_size_t));

                    }

                } else {

                    freeBuffer(nullptr,

                        reinterpret_cast<const uint8_t*>(tr.data.ptr.buffer),

                        tr.data_size,

                    freeBuffer(reinterpret_cast<const uint8_t*>(tr.data.ptr.buffer), tr.data_size,

                               reinterpret_cast<const binder_size_t*>(tr.data.ptr.offsets),

                               tr.offsets_size / sizeof(binder_size_t));

                    continue;
@@ -1473,17 +1470,13 @@ void IPCThreadState::logExtendedError() { 
             ee.id, ee.command, ee.param);

}



void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,

                                size_t /*dataSize*/,

                                const binder_size_t* /*objects*/,

                                size_t /*objectsSize*/)

{

void IPCThreadState::freeBuffer(const uint8_t* data, size_t /*dataSize*/,

                                const binder_size_t* /*objects*/, size_t /*objectsSize*/) {

    //ALOGI("Freeing parcel %p", &parcel);

    IF_LOG_COMMANDS() {

        alog << "Writing BC_FREE_BUFFER for " << data << endl;

    }

    ALOG_ASSERT(data != NULL, "Called with NULL data");

    if (parcel != nullptr) parcel->closeFileDescriptors();

    IPCThreadState* state = self();

    state->mOut.writeInt32(BC_FREE_BUFFER);

    state->mOut.writePointer((uintptr_t)data);

libs/binder/Parcel.cpp

+10 −3
Original line number Diff line number Diff line
@@ -2706,7 +2706,9 @@ void Parcel::freeDataNoInit() 
        LOG_ALLOC("Parcel %p: freeing other owner data", this);

        //ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());

        auto* kernelFields = maybeKernelFields();

        mOwner(this, mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,

        // Close FDs before freeing, otherwise they will leak for kernel binder.

        closeFileDescriptors();

        mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,

               kernelFields ? kernelFields->mObjectsSize : 0);

    } else {

        LOG_ALLOC("Parcel %p: freeing allocated data", this);
@@ -2892,7 +2894,12 @@ status_t Parcel::continueWrite(size_t desired) 
            memcpy(objects, kernelFields->mObjects, objectsSize * sizeof(binder_size_t));

        }

        // ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());

        mOwner(this, mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,

        if (kernelFields) {

            // TODO(b/239222407): This seems wrong. We should only free FDs when

            // they are in a truncated section of the parcel.

            closeFileDescriptors();

        }

        mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,

               kernelFields ? kernelFields->mObjectsSize : 0);

        mOwner = nullptr;

libs/binder/RpcState.cpp

+4 −6
Original line number Diff line number Diff line
@@ -586,13 +586,12 @@ status_t RpcState::transactAddress(const sp<RpcSession::RpcConnection>& connecti 
    return waitForReply(connection, session, reply);

}



static void cleanup_reply_data(Parcel* p, const uint8_t* data, size_t dataSize,

                               const binder_size_t* objects, size_t objectsCount) {

    (void)p;

static void cleanup_reply_data(const uint8_t* data, size_t dataSize, const binder_size_t* objects,

                               size_t objectsCount) {

    delete[] const_cast<uint8_t*>(data);

    (void)dataSize;

    LOG_ALWAYS_FATAL_IF(objects != nullptr);

    LOG_ALWAYS_FATAL_IF(objectsCount != 0, "%zu objects remaining", objectsCount);

    (void)objectsCount;

}



status_t RpcState::waitForReply(const sp<RpcSession::RpcConnection>& connection,
@@ -799,9 +798,8 @@ status_t RpcState::processTransact( 
                                   std::move(ancillaryFds));

}



static void do_nothing_to_transact_data(Parcel* p, const uint8_t* data, size_t dataSize,

static void do_nothing_to_transact_data(const uint8_t* data, size_t dataSize,

                                        const binder_size_t* objects, size_t objectsCount) {

    (void)p;

    (void)data;

    (void)dataSize;

    (void)objects;

libs/binder/include/binder/IPCThreadState.h

+2 −3
Original line number Diff line number Diff line
@@ -217,9 +217,8 @@ private: 
            void                clearCaller();



    static  void                threadDestructor(void *st);

    static  void                freeBuffer(Parcel* parcel,

                                           const uint8_t* data, size_t dataSize,

                                           const binder_size_t* objects, size_t objectsSize);

    static void freeBuffer(const uint8_t* data, size_t dataSize, const binder_size_t* objects,

                           size_t objectsSize);

    static  void                logExtendedError();



    const   sp<ProcessState>    mProcess;

libs/binder/include/binder/Parcel.h

+3 −3
Original line number Diff line number Diff line
@@ -598,9 +598,9 @@ public: 
    void                print(TextOutput& to, uint32_t flags = 0) const;



private:

    typedef void        (*release_func)(Parcel* parcel,

                                        const uint8_t* data, size_t dataSize,

                                        const binder_size_t* objects, size_t objectsSize);

    // `objects` and `objectsSize` always 0 for RPC Parcels.

    typedef void (*release_func)(const uint8_t* data, size_t dataSize, const binder_size_t* objects,

                                 size_t objectsSize);



    uintptr_t           ipcData() const;

    size_t              ipcDataSize() const;