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

Commit 5b028525 authored by Andy Hung's avatar Andy Hung
Browse files

Fix binder memory handling for 64 bit devices 

Change MemoryHeap offset to use off_t.
Always transmit Memory related size and offset as 64 bits.

Test: CTS, native binder tests, sanity
Bug: 117556990
Change-Id: Icaabf7442f561a53941f9ebebe4029ddc533b0c2
parent 10ca1c36

libs/binder/IMemory.cpp

+28 −20
Original line number Diff line number Diff line
@@ -86,7 +86,7 @@ public: 
    virtual void* getBase() const;

    virtual size_t getSize() const;

    virtual uint32_t getFlags() const;

    virtual uint32_t getOffset() const;

    off_t getOffset() const override;



private:

    friend class IMemory;
@@ -113,7 +113,7 @@ private: 
    mutable void*       mBase;

    mutable size_t      mSize;

    mutable uint32_t    mFlags;

    mutable uint32_t    mOffset;

    mutable off_t       mOffset;

    mutable bool        mRealHeap;

    mutable Mutex       mLock;

};
@@ -187,13 +187,16 @@ sp<IMemoryHeap> BpMemory::getMemory(ssize_t* offset, size_t* size) const 
        data.writeInterfaceToken(IMemory::getInterfaceDescriptor());

        if (remote()->transact(GET_MEMORY, data, &reply) == NO_ERROR) {

            sp<IBinder> heap = reply.readStrongBinder();

            ssize_t o = reply.readInt32();

            size_t s = reply.readInt32();

            if (heap != nullptr) {

                mHeap = interface_cast<IMemoryHeap>(heap);

                if (mHeap != nullptr) {

                    const int64_t offset64 = reply.readInt64();

                    const uint64_t size64 = reply.readUint64();

                    const ssize_t o = (ssize_t)offset64;

                    const size_t s = (size_t)size64;

                    size_t heapSize = mHeap->getSize();

                    if (s <= heapSize

                    if (s == size64 && o == offset64 // ILP32 bounds check

                            && s <= heapSize

                            && o >= 0

                            && (static_cast<size_t>(o) <= heapSize - s)) {

                        mOffset = o;
@@ -233,8 +236,8 @@ status_t BnMemory::onTransact( 
            ssize_t offset;

            size_t size;

            reply->writeStrongBinder( IInterface::asBinder(getMemory(&offset, &size)) );

            reply->writeInt32(offset);

            reply->writeInt32(size);

            reply->writeInt64(offset);

            reply->writeUint64(size);

            return NO_ERROR;

        } break;

        default:
@@ -313,18 +316,23 @@ void BpMemoryHeap::assertReallyMapped() const 
        data.writeInterfaceToken(IMemoryHeap::getInterfaceDescriptor());

        status_t err = remote()->transact(HEAP_ID, data, &reply);

        int parcel_fd = reply.readFileDescriptor();

        ssize_t size = reply.readInt32();

        uint32_t flags = reply.readInt32();

        uint32_t offset = reply.readInt32();



        ALOGE_IF(err, "binder=%p transaction failed fd=%d, size=%zd, err=%d (%s)",

        const uint64_t size64 = reply.readUint64();

        const int64_t offset64 = reply.readInt64();

        const uint32_t flags = reply.readUint32();

        const size_t size = (size_t)size64;

        const off_t offset = (off_t)offset64;

        if (err != NO_ERROR || // failed transaction

                size != size64 || offset != offset64) { // ILP32 size check

            ALOGE("binder=%p transaction failed fd=%d, size=%zu, err=%d (%s)",

                    IInterface::asBinder(this).get(),

                    parcel_fd, size, err, strerror(-err));

            return;

        }



        Mutex::Autolock _l(mLock);

        if (mHeapId.load(memory_order_relaxed) == -1) {

            int fd = fcntl(parcel_fd, F_DUPFD_CLOEXEC, 0);

            ALOGE_IF(fd==-1, "cannot dup fd=%d, size=%zd, err=%d (%s)",

            ALOGE_IF(fd == -1, "cannot dup fd=%d, size=%zu, err=%d (%s)",

                    parcel_fd, size, err, strerror(errno));



            int access = PROT_READ;
@@ -334,7 +342,7 @@ void BpMemoryHeap::assertReallyMapped() const 
            mRealHeap = true;

            mBase = mmap(nullptr, size, access, MAP_SHARED, fd, offset);

            if (mBase == MAP_FAILED) {

                ALOGE("cannot map BpMemoryHeap (binder=%p), size=%zd, fd=%d (%s)",

                ALOGE("cannot map BpMemoryHeap (binder=%p), size=%zu, fd=%d (%s)",

                        IInterface::asBinder(this).get(), size, fd, strerror(errno));

                close(fd);

            } else {
@@ -368,7 +376,7 @@ uint32_t BpMemoryHeap::getFlags() const { 
    return mFlags;

}



uint32_t BpMemoryHeap::getOffset() const {

off_t BpMemoryHeap::getOffset() const {

    assertMapped();

    return mOffset;

}
@@ -390,9 +398,9 @@ status_t BnMemoryHeap::onTransact( 
       case HEAP_ID: {

            CHECK_INTERFACE(IMemoryHeap, data, reply);

            reply->writeFileDescriptor(getHeapID());

            reply->writeInt32(getSize());

            reply->writeInt32(getFlags());

            reply->writeInt32(getOffset());

            reply->writeUint64(getSize());

            reply->writeInt64(getOffset());

            reply->writeUint32(getFlags());

            return NO_ERROR;

        } break;

        default:

libs/binder/MemoryHeapBase.cpp

+17 −10
Original line number Diff line number Diff line
@@ -76,7 +76,7 @@ MemoryHeapBase::MemoryHeapBase(const char* device, size_t size, uint32_t flags) 
    }

}



MemoryHeapBase::MemoryHeapBase(int fd, size_t size, uint32_t flags, uint32_t offset)

MemoryHeapBase::MemoryHeapBase(int fd, size_t size, uint32_t flags, off_t offset)

    : mFD(-1), mSize(0), mBase(MAP_FAILED), mFlags(flags),

      mDevice(nullptr), mNeedUnmap(false), mOffset(0)

{
@@ -85,7 +85,7 @@ MemoryHeapBase::MemoryHeapBase(int fd, size_t size, uint32_t flags, uint32_t off 
    mapfd(fcntl(fd, F_DUPFD_CLOEXEC, 0), size, offset);

}



status_t MemoryHeapBase::init(int fd, void *base, int size, int flags, const char* device)

status_t MemoryHeapBase::init(int fd, void *base, size_t size, int flags, const char* device)

{

    if (mFD != -1) {

        return INVALID_OPERATION;
@@ -98,13 +98,20 @@ status_t MemoryHeapBase::init(int fd, void *base, int size, int flags, const cha 
    return NO_ERROR;

}



status_t MemoryHeapBase::mapfd(int fd, size_t size, uint32_t offset)

status_t MemoryHeapBase::mapfd(int fd, size_t size, off_t offset)

{

    if (size == 0) {

        // try to figure out the size automatically

        struct stat sb;

        if (fstat(fd, &sb) == 0)

            size = sb.st_size;

        if (fstat(fd, &sb) == 0) {

            size = (size_t)sb.st_size;

            // sb.st_size is off_t which on ILP32 may be 64 bits while size_t is 32 bits.

            if ((off_t)size != sb.st_size) {

                ALOGE("%s: size of file %lld cannot fit in memory",

                        __func__, (long long)sb.st_size);

                return INVALID_OPERATION;

            }

        }

        // if it didn't work, let mmap() fail.

    }
@@ -112,12 +119,12 @@ status_t MemoryHeapBase::mapfd(int fd, size_t size, uint32_t offset) 
        void* base = (uint8_t*)mmap(nullptr, size,

                PROT_READ|PROT_WRITE, MAP_SHARED, fd, offset);

        if (base == MAP_FAILED) {

            ALOGE("mmap(fd=%d, size=%u) failed (%s)",

                    fd, uint32_t(size), strerror(errno));

            ALOGE("mmap(fd=%d, size=%zu) failed (%s)",

                    fd, size, strerror(errno));

            close(fd);

            return -errno;

        }

        //ALOGD("mmap(fd=%d, base=%p, size=%lu)", fd, base, size);

        //ALOGD("mmap(fd=%d, base=%p, size=%zu)", fd, base, size);

        mBase = base;

        mNeedUnmap = true;

    } else  {
@@ -140,7 +147,7 @@ void MemoryHeapBase::dispose() 
    int fd = android_atomic_or(-1, &mFD);

    if (fd >= 0) {

        if (mNeedUnmap) {

            //ALOGD("munmap(fd=%d, base=%p, size=%lu)", fd, mBase, mSize);

            //ALOGD("munmap(fd=%d, base=%p, size=%zu)", fd, mBase, mSize);

            munmap(mBase, mSize);

        }

        mBase = nullptr;
@@ -169,7 +176,7 @@ const char* MemoryHeapBase::getDevice() const { 
    return mDevice;

}



uint32_t MemoryHeapBase::getOffset() const {

off_t MemoryHeapBase::getOffset() const {

    return mOffset;

}

libs/binder/include/binder/IMemory.h

+1 −1
Original line number Diff line number Diff line
@@ -43,7 +43,7 @@ public: 
    virtual void*       getBase() const = 0;

    virtual size_t      getSize() const = 0;

    virtual uint32_t    getFlags() const = 0;

    virtual uint32_t    getOffset() const = 0;

    virtual off_t       getOffset() const = 0;



    // these are there just for backward source compatibility

    int32_t heapID() const { return getHeapID(); }

libs/binder/include/binder/MemoryHeapBase.h

+5 −5
Original line number Diff line number Diff line
@@ -42,7 +42,7 @@ public: 
     * maps the memory referenced by fd. but DOESN'T take ownership

     * of the filedescriptor (it makes a copy with dup()

     */

    MemoryHeapBase(int fd, size_t size, uint32_t flags = 0, uint32_t offset = 0);

    MemoryHeapBase(int fd, size_t size, uint32_t flags = 0, off_t offset = 0);



    /*

     * maps memory from the given device
@@ -64,7 +64,7 @@ public: 


    virtual size_t      getSize() const;

    virtual uint32_t    getFlags() const;

    virtual uint32_t    getOffset() const;

            off_t       getOffset() const override;



    const char*         getDevice() const;
@@ -82,11 +82,11 @@ public: 
protected:

            MemoryHeapBase();

    // init() takes ownership of fd

    status_t init(int fd, void *base, int size,

    status_t init(int fd, void *base, size_t size,

            int flags = 0, const char* device = nullptr);



private:

    status_t mapfd(int fd, size_t size, uint32_t offset = 0);

    status_t mapfd(int fd, size_t size, off_t offset = 0);



    int         mFD;

    size_t      mSize;
@@ -94,7 +94,7 @@ private: 
    uint32_t    mFlags;

    const char* mDevice;

    bool        mNeedUnmap;

    uint32_t    mOffset;

    off_t       mOffset;

};



// ---------------------------------------------------------------------------