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

Commit 86149953 authored by Frederick Mayle's avatar Frederick Mayle Committed by Android (Google) Code Review
Browse files

Merge "binder: fix FD handling in continueWrite" into sc-dev

parents 0db4fced 8010cbb3

libs/binder/IPCThreadState.cpp

+1 −2
Original line number Diff line number Diff line
@@ -1419,7 +1419,7 @@ status_t IPCThreadState::freeze(pid_t pid, bool enable, uint32_t timeout_ms) { 
    return ret;

}



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

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

                                size_t /*dataSize*/,

                                const binder_size_t* /*objects*/,

                                size_t /*objectsSize*/)
@@ -1429,7 +1429,6 @@ void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data, 
        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

+18 −2
Original line number Diff line number Diff line
@@ -2076,11 +2076,15 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const 


void Parcel::closeFileDescriptors()

{

    truncateFileDescriptors(0);

}



void Parcel::truncateFileDescriptors(size_t newObjectsSize) {

    size_t i = mObjectsSize;

    if (i > 0) {

        //ALOGI("Closing file descriptors for %zu objects...", i);

    }

    while (i > 0) {

    while (i > newObjectsSize) {

        i--;

        const flat_binder_object* flat

            = reinterpret_cast<flat_binder_object*>(mData+mObjects[i]);
@@ -2228,6 +2232,7 @@ void Parcel::freeDataNoInit() 
    if (mOwner) {

        LOG_ALLOC("Parcel %p: freeing other owner data", this);

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

        closeFileDescriptors();

        mOwner(this, mData, mDataSize, mObjects, mObjectsSize);

    } else {

        LOG_ALLOC("Parcel %p: freeing allocated data", this);
@@ -2351,8 +2356,9 @@ status_t Parcel::continueWrite(size_t desired) 
        if (desired == 0) {

            objectsSize = 0;

        } else {

            validateReadData(mDataSize); // hack to sort the objects

            while (objectsSize > 0) {

                if (mObjects[objectsSize-1] < desired)

                if (mObjects[objectsSize-1] + sizeof(flat_binder_object) <= desired)

                    break;

                objectsSize--;

            }
@@ -2397,8 +2403,18 @@ status_t Parcel::continueWrite(size_t desired) 
        }

        if (objects && mObjects) {

            memcpy(objects, mObjects, objectsSize*sizeof(binder_size_t));

            // All FDs are owned when `mOwner`, even when `cookie == 0`. When

            // we switch to `!mOwner`, we need to explicitly mark the FDs as

            // owned.

            for (size_t i = 0; i < objectsSize; i++) {

                flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);

                if (flat->hdr.type == BINDER_TYPE_FD) {

                    flat->cookie = 1;

                }

            }

        }

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

        truncateFileDescriptors(objectsSize);

        mOwner(this, mData, mDataSize, mObjects, mObjectsSize);

        mOwner = nullptr;

libs/binder/include/binder/Parcel.h

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



private:

    // Close all file descriptors in the parcel at object positions >= newObjectsSize.

    __attribute__((__visibility__("hidden"))) void truncateFileDescriptors(size_t newObjectsSize);



    typedef void        (*release_func)(Parcel* parcel,

                                        const uint8_t* data, size_t dataSize,

                                        const binder_size_t* objects, size_t objectsSize);

libs/binder/tests/Android.bp

+4 −0
Original line number Diff line number Diff line
@@ -60,7 +60,9 @@ cc_test { 
    defaults: ["binder_test_defaults"],

    srcs: ["binderLibTest.cpp"],

    shared_libs: [

        "libbase",

        "libbinder",

        "liblog",

        "libutils",

    ],

    static_libs: [
@@ -101,7 +103,9 @@ cc_test { 


    srcs: ["binderLibTest.cpp"],

    shared_libs: [

        "libbase",

        "libbinder",

        "liblog",

        "libutils",

    ],

    static_libs: [

libs/binder/tests/binderLibTest.cpp

+163 −0
Original line number Diff line number Diff line
@@ -26,6 +26,8 @@ 
#include <gmock/gmock.h>

#include <gtest/gtest.h>



#include <android-base/logging.h>

#include <android-base/unique_fd.h>

#include <binder/Binder.h>

#include <binder/IBinder.h>

#include <binder/IPCThreadState.h>
@@ -33,6 +35,7 @@ 


#include <linux/sched.h>

#include <sys/epoll.h>

#include <sys/mman.h>

#include <sys/prctl.h>



#include "../binder_module.h"
@@ -41,6 +44,7 @@ 
#define ARRAY_SIZE(array) (sizeof array / sizeof array[0])



using namespace android;

using android::base::unique_fd;

using testing::Not;



// e.g. EXPECT_THAT(expr, StatusEq(OK)) << "additional message";
@@ -85,6 +89,8 @@ enum BinderLibTestTranscationCode { 
    BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,

    BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,

    BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,

    BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,

    BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,

    BINDER_LIB_TEST_EXIT_TRANSACTION,

    BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,

    BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -404,6 +410,35 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE 
        };

};



ssize_t countFds() {

    DIR* dir = opendir("/proc/self/fd/");

    if (dir == nullptr) return -1;

    ssize_t ret = 0;

    dirent* ent;

    while ((ent = readdir(dir)) != nullptr) ret++;

    closedir(dir);

    return ret;

}



struct FdLeakDetector {

    int startCount;



    FdLeakDetector() {

        // This log statement is load bearing. We have to log something before

        // counting FDs to make sure the logging system is initialized, otherwise

        // the sockets it opens will look like a leak.

        ALOGW("FdLeakDetector counting FDs.");

        startCount = countFds();

    }

    ~FdLeakDetector() {

        int endCount = countFds();

        if (startCount != endCount) {

            ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount

                          << ") fd leak?";

        }

    }

};



TEST_F(BinderLibTest, NopTransaction) {

    Parcel data, reply;

    EXPECT_THAT(m_server->transact(BINDER_LIB_TEST_NOP_TRANSACTION, data, &reply),
@@ -805,6 +840,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) { 
    EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));

}



TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {

    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,

                                 &reply));

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));

}



// Used to trigger fdsan error (b/239222407).

TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {

    GTEST_SKIP() << "triggers fdsan false positive: b/370824489";



    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,

                                 &reply));

    reply.setDataPosition(reply.dataSize());

    reply.writeInt32(0);

    reply.setDataPosition(0);

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));

}



// Used to trigger fdsan error (b/239222407).

TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {

    GTEST_SKIP() << "triggers fdsan false positive: b/370824489";



    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,

                                 &reply));

    reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));

}



TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {

    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,

                                 &reply));

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));

}



// Used to trigger fdsan error (b/239222407).

TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {

    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,

                                 &reply));

    reply.setDataPosition(reply.dataSize());

    reply.writeInt32(0);

    reply.setDataPosition(0);

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));

}



// Used to trigger fdsan error (b/239222407).

TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndTruncate) {

    FdLeakDetector fd_leak_detector;



    Parcel data;

    Parcel reply;

    EXPECT_EQ(NO_ERROR,

              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,

                                 &reply));

    reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));

    unique_fd a, b;

    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));

    EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));

}



TEST_F(BinderLibTest, PromoteLocal) {

    sp<IBinder> strong = new BBinder();

    wp<IBinder> weak = strong;
@@ -1438,6 +1567,40 @@ class BinderLibTestService : public BBinder 
                if (ret != size) return UNKNOWN_ERROR;

                return NO_ERROR;

            }

            case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {

                unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));

                if (!fd1.ok()) {

                    PLOG(ERROR) << "memfd_create failed";

                    return UNKNOWN_ERROR;

                }

                unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));

                if (!fd2.ok()) {

                    PLOG(ERROR) << "memfd_create failed";

                    return UNKNOWN_ERROR;

                }

                status_t ret;

                ret = reply->writeFileDescriptor(fd1.release(), true);

                if (ret != NO_ERROR) {

                    return ret;

                }

                ret = reply->writeFileDescriptor(fd2.release(), true);

                if (ret != NO_ERROR) {

                    return ret;

                }

                return NO_ERROR;

            }

            case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {

                status_t ret;

                ret = reply->writeFileDescriptor(STDOUT_FILENO, false);

                if (ret != NO_ERROR) {

                    return ret;

                }

                ret = reply->writeFileDescriptor(STDERR_FILENO, false);

                if (ret != NO_ERROR) {

                    return ret;

                }

                return NO_ERROR;

            }

            case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:

                alarm(10);

                return NO_ERROR;