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

Commit 7f04847a authored by Frederick Mayle's avatar Frederick Mayle Committed by Automerger Merge Worker
Browse files

binder: fix FD handling in continueWrite am: e32c1ab2 am: d87b8c47 

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/native/+/29868448



Change-Id: Ib311677f7f594bb7f287312a96cfb0c355f7560a
Signed-off-by: default avatarAutomerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
parents 3891629b d87b8c47

libs/binder/Parcel.cpp

+57 −9
Original line number Diff line number Diff line
@@ -2505,13 +2505,17 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const 
#endif // BINDER_WITH_KERNEL_IPC



void Parcel::closeFileDescriptors() {

    truncateFileDescriptors(0);

}



void Parcel::truncateFileDescriptors(size_t newObjectsSize) {

    if (auto* kernelFields = maybeKernelFields()) {

#ifdef BINDER_WITH_KERNEL_IPC

        size_t i = kernelFields->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 + kernelFields->mObjects[i]);
@@ -2521,6 +2525,7 @@ void Parcel::closeFileDescriptors() { 
            }

        }

#else  // BINDER_WITH_KERNEL_IPC

        (void)newObjectsSize;

        LOG_ALWAYS_FATAL("Binder kernel driver disabled at build time");

#endif // BINDER_WITH_KERNEL_IPC

    } else if (auto* rpcFields = maybeRpcFields()) {
@@ -2870,13 +2875,38 @@ status_t Parcel::continueWrite(size_t desired) 
            objectsSize = 0;

        } else {

            if (kernelFields) {

#ifdef BINDER_WITH_KERNEL_IPC

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

                while (objectsSize > 0) {

                    if (kernelFields->mObjects[objectsSize - 1] < desired) break;

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

                        desired)

                        break;

                    objectsSize--;

                }

#endif // BINDER_WITH_KERNEL_IPC

            } else {

                while (objectsSize > 0) {

                    if (rpcFields->mObjectPositions[objectsSize - 1] < desired) break;

                    // Object size varies by type.

                    uint32_t pos = rpcFields->mObjectPositions[objectsSize - 1];

                    size_t size = sizeof(RpcFields::ObjectType);

                    uint32_t minObjectEnd;

                    if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||

                        minObjectEnd > mDataSize) {

                        return BAD_VALUE;

                    }

                    const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);

                    switch (type) {

                        case RpcFields::TYPE_BINDER_NULL:

                            break;

                        case RpcFields::TYPE_BINDER:

                            size += sizeof(uint64_t); // address

                            break;

                        case RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR:

                            size += sizeof(int32_t); // fd index

                            break;

                    }



                    if (pos + size <= desired) break;

                    objectsSize--;

                }

            }
@@ -2925,15 +2955,24 @@ status_t Parcel::continueWrite(size_t desired) 
        if (mData) {

            memcpy(data, mData, mDataSize < desired ? mDataSize : desired);

        }

#ifdef BINDER_WITH_KERNEL_IPC

        if (objects && kernelFields && kernelFields->mObjects) {

            memcpy(objects, kernelFields->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());

        if (kernelFields) {

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

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

            closeFileDescriptors();

            truncateFileDescriptors(objectsSize);

        }

#endif // BINDER_WITH_KERNEL_IPC

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

               kernelFields ? kernelFields->mObjectsSize : 0);

        mOwner = nullptr;
@@ -3060,11 +3099,19 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) { 
    }

    while (rpcFields->mObjectPositions.size() > newObjectsSize) {

        uint32_t pos = rpcFields->mObjectPositions.back();

        rpcFields->mObjectPositions.pop_back();

        uint32_t minObjectEnd;

        if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||

            minObjectEnd > mDataSize) {

            return BAD_VALUE;

        }

        const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);

        if (type == RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR) {

            const auto fdIndex =

                    *reinterpret_cast<const int32_t*>(mData + pos + sizeof(RpcFields::ObjectType));

            uint32_t objectEnd;

            if (__builtin_add_overflow(minObjectEnd, sizeof(int32_t), &objectEnd) ||

                objectEnd > mDataSize) {

                return BAD_VALUE;

            }

            const auto fdIndex = *reinterpret_cast<const int32_t*>(mData + minObjectEnd);

            if (rpcFields->mFds == nullptr || fdIndex < 0 ||

                static_cast<size_t>(fdIndex) >= rpcFields->mFds->size()) {

                ALOGE("RPC Parcel contains invalid file descriptor index. index=%d fd_count=%zu",
@@ -3074,6 +3121,7 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) { 
            // In practice, this always removes the last element.

            rpcFields->mFds->erase(rpcFields->mFds->begin() + fdIndex);

        }

        rpcFields->mObjectPositions.pop_back();

    }

    return OK;

}

libs/binder/include/binder/Parcel.h

+3 −0
Original line number Diff line number Diff line
@@ -602,6 +602,9 @@ public: 
    void print(std::ostream& 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);



    // `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);

libs/binder/tests/binderLibTest.cpp

+162 −0
Original line number Diff line number Diff line
@@ -27,6 +27,7 @@ 
#include <gmock/gmock.h>

#include <gtest/gtest.h>



#include <android-base/logging.h>

#include <android-base/properties.h>

#include <android-base/result-gmock.h>

#include <android-base/result.h>
@@ -43,6 +44,7 @@ 


#include <linux/sched.h>

#include <sys/epoll.h>

#include <sys/mman.h>

#include <sys/prctl.h>

#include <sys/socket.h>

#include <sys/un.h>
@@ -57,6 +59,7 @@ using namespace std::string_literals; 
using namespace std::chrono_literals;

using android::base::testing::HasValue;

using android::base::testing::Ok;

using android::base::unique_fd;

using testing::ExplainMatchResult;

using testing::Matcher;

using testing::Not;
@@ -106,6 +109,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,
@@ -445,6 +450,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, CannotUseBinderAfterFork) {

    // EXPECT_DEATH works by forking the process

    EXPECT_DEATH({ ProcessState::self(); }, "libbinder ProcessState can not be used after fork");
@@ -872,6 +906,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;
@@ -1800,6 +1928,40 @@ public: 
                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;