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

Commit 55cbf5e4 authored by Patrick Williams's avatar Patrick Williams
Browse files

Fix deadlock that can occur when BBQ's destructor runs while the BQ lock is held 

This change ensures we don't destroy a sp<BLASTBufferQueue> while the BufferQueue lock is held.  Doing so can cause deadlock because BLASTBufferQueue's destructor calls methods that acquire the BufferQueue lock.

To avoid creating / destroying sp<BLASTBufferQueue> while the BufferQueue lock is held, the BufferReleaseReader is given shared ownership across BBQBufferQueueCore, BBQBufferQueueProducer, and BLASTBufferQueue. This allows BBQBufferQueueCore and BBQBufferQueueProducer to use the BufferReleaseReader without obtaining a strong pointer to BBQ.

Bug: 410458641
Flag: com.android.graphics.libgui.flags.buffer_release_channel
Test: BLASTBufferQueueTest
Change-Id: I3d5b6fc52fba0ba52d50863dea072ade63b9bd27
parent e2c01c5f

libs/gui/BLASTBufferQueue.cpp

+82 −35
Original line number Diff line number Diff line
@@ -227,11 +227,6 @@ void BLASTBufferQueue::initialize() { 
            },

            this);



#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL)

    gui::BufferReleaseChannel::open(mName, mBufferReleaseConsumer, mBufferReleaseProducer);

    mBufferReleaseReader.emplace(*this);

#endif



    // safe default, most producers are expected to override this

    mProducer->setMaxDequeuedBufferCount(2);
@@ -1163,20 +1158,50 @@ public: 
};



#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL)



// BufferReleaseReader is used to do blocking but interruptible reads from the buffer

// release channel. To implement this, BufferReleaseReader owns an epoll file descriptor that

// is configured to wake up when either the BufferReleaseReader::ConsumerEndpoint or an eventfd

// becomes readable. Interrupts are necessary because a free buffer may become available for

// reasons other than a buffer release from the producer.

class BufferReleaseReader {

public:

    explicit BufferReleaseReader(std::unique_ptr<gui::BufferReleaseChannel::ConsumerEndpoint>);



    BufferReleaseReader(const BufferReleaseReader&) = delete;

    BufferReleaseReader& operator=(const BufferReleaseReader&) = delete;



    // Block until we can read a buffer release message.

    //

    // Returns:

    // * OK if a ReleaseCallbackId and Fence were successfully read.

    // * WOULD_BLOCK if the blocking read was interrupted by interruptBlockingRead.

    // * TIMED_OUT if the blocking read timed out.

    // * UNKNOWN_ERROR if something went wrong.

    status_t readBlocking(ReleaseCallbackId& outId, sp<Fence>& outReleaseFence,

                          uint32_t& outMaxAcquiredBufferCount, nsecs_t timeout);



    status_t readNonBlocking(ReleaseCallbackId& outId, sp<Fence>& outReleaseFence,

                             uint32_t& outMaxAcquiredBufferCount);



    void interruptBlockingRead();

    void clearInterrupts();



private:

    std::unique_ptr<gui::BufferReleaseChannel::ConsumerEndpoint> mConsumerEndpoint;

    android::base::unique_fd mEpollFd;

    android::base::unique_fd mEventFd;

};



class BBQBufferQueueCore : public BufferQueueCore {

public:

    explicit BBQBufferQueueCore(const wp<BLASTBufferQueue>& bbq) : mBLASTBufferQueue{bbq} {}

    explicit BBQBufferQueueCore(std::shared_ptr<BufferReleaseReader> bufferReleaseReader)

          : mBufferReleaseReader{std::move(bufferReleaseReader)} {}



    void notifyBufferReleased() const override {

        sp<BLASTBufferQueue> bbq = mBLASTBufferQueue.promote();

        if (!bbq) {

            return;

        }

        bbq->mBufferReleaseReader->interruptBlockingRead();

    }

    void notifyBufferReleased() const override { mBufferReleaseReader->interruptBlockingRead(); }



private:

    wp<BLASTBufferQueue> mBLASTBufferQueue;

    std::shared_ptr<BufferReleaseReader> mBufferReleaseReader;

};

#endif
@@ -1184,9 +1209,17 @@ private: 
// can be non-blocking when the producer is in the client process.

class BBQBufferQueueProducer : public BufferQueueProducer {

public:

#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL)

    BBQBufferQueueProducer(const sp<BufferQueueCore>& core, const wp<BLASTBufferQueue>& bbq,

                           std::shared_ptr<BufferReleaseReader> bufferReleaseReader)

          : BufferQueueProducer(core, false /* consumerIsSurfaceFlinger*/),

            mBLASTBufferQueue(bbq),

            mBufferReleaseReader(std::move(bufferReleaseReader)) {}

#else

    BBQBufferQueueProducer(const sp<BufferQueueCore>& core, const wp<BLASTBufferQueue>& bbq)

          : BufferQueueProducer(core, false /* consumerIsSurfaceFlinger*/),

            mBLASTBufferQueue(bbq) {}

#endif



    status_t connect(const sp<IProducerListener>& listener, int api, bool producerControlledByApp,

                     QueueBufferOutput* output) override {
@@ -1234,24 +1267,20 @@ public: 
#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL)

    status_t waitForBufferRelease(std::unique_lock<std::mutex>& bufferQueueLock,

                                  nsecs_t timeout) const override {

        ATRACE_CALL();

        const auto startTime = std::chrono::steady_clock::now();

        sp<BLASTBufferQueue> bbq = mBLASTBufferQueue.promote();

        if (!bbq) {

            return OK;

        }



        // BufferQueue has already checked if we have a free buffer. If there's an unread interrupt,

        // we want to ignore it. This must be done before unlocking the BufferQueue lock to ensure

        // we don't miss an interrupt.

        bbq->mBufferReleaseReader->clearInterrupts();

        mBufferReleaseReader->clearInterrupts();

        UnlockGuard unlockGuard{bufferQueueLock};



        ATRACE_FORMAT("waiting for free buffer");

        ReleaseCallbackId id;

        sp<Fence> fence;

        uint32_t maxAcquiredBufferCount;

        status_t status =

                bbq->mBufferReleaseReader->readBlocking(id, fence, maxAcquiredBufferCount, timeout);

                mBufferReleaseReader->readBlocking(id, fence, maxAcquiredBufferCount, timeout);

        if (status == TIMED_OUT) {

            return TIMED_OUT;

        } else if (status != OK) {
@@ -1260,6 +1289,11 @@ public: 
            return OK;

        }



        sp<BLASTBufferQueue> bbq = mBLASTBufferQueue.promote();

        if (!bbq) {

            return OK;

        }



        bbq->releaseBufferCallback(id, fence, maxAcquiredBufferCount);

        const nsecs_t durationNanos = std::chrono::duration_cast<std::chrono::nanoseconds>(

                                              std::chrono::steady_clock::now() - startTime)
@@ -1275,6 +1309,7 @@ public: 


private:

    const wp<BLASTBufferQueue> mBLASTBufferQueue;

    std::shared_ptr<BufferReleaseReader> mBufferReleaseReader;

};



// Similar to BufferQueue::createBufferQueue but creates an adapter specific bufferqueue producer.
@@ -1287,14 +1322,23 @@ void BLASTBufferQueue::createBufferQueue(sp<IGraphicBufferProducer>* outProducer 
    LOG_ALWAYS_FATAL_IF(outConsumer == nullptr, "BLASTBufferQueue: outConsumer must not be NULL");



#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL)

    auto core = sp<BBQBufferQueueCore>::make(wp<BLASTBufferQueue>::fromExisting(this));

    std::unique_ptr<gui::BufferReleaseChannel::ConsumerEndpoint> bufferReleaseConsumer;

    gui::BufferReleaseChannel::open(mName, bufferReleaseConsumer, mBufferReleaseProducer);

    mBufferReleaseReader = std::make_shared<BufferReleaseReader>(std::move(bufferReleaseConsumer));



    auto core = sp<BBQBufferQueueCore>::make(mBufferReleaseReader);

#else

    auto core = sp<BufferQueueCore>::make();

#endif

    LOG_ALWAYS_FATAL_IF(core == nullptr, "BLASTBufferQueue: failed to create BufferQueueCore");



#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL)

    auto producer = sp<BBQBufferQueueProducer>::make(core, wp<BLASTBufferQueue>::fromExisting(this),

                                                     mBufferReleaseReader);

#else

    auto producer =

            sp<BBQBufferQueueProducer>::make(core, wp<BLASTBufferQueue>::fromExisting(this));

#endif

    LOG_ALWAYS_FATAL_IF(producer == nullptr,

                        "BLASTBufferQueue: failed to create BBQBufferQueueProducer");
@@ -1394,8 +1438,7 @@ void BLASTBufferQueue::drainBufferReleaseConsumer() { 
        ReleaseCallbackId id;

        sp<Fence> fence;

        uint32_t maxAcquiredBufferCount;

        status_t status =

                mBufferReleaseConsumer->readReleaseFence(id, fence, maxAcquiredBufferCount);

        status_t status = mBufferReleaseReader->readNonBlocking(id, fence, maxAcquiredBufferCount);

        if (status != OK) {

            return;

        }
@@ -1403,7 +1446,9 @@ void BLASTBufferQueue::drainBufferReleaseConsumer() { 
    }

}



BLASTBufferQueue::BufferReleaseReader::BufferReleaseReader(BLASTBufferQueue& bbq) : mBbq{bbq} {

BufferReleaseReader::BufferReleaseReader(

        std::unique_ptr<gui::BufferReleaseChannel::ConsumerEndpoint> consumerEndpoint)

      : mConsumerEndpoint{std::move(consumerEndpoint)} {

    mEpollFd = android::base::unique_fd{epoll_create1(EPOLL_CLOEXEC)};

    LOG_ALWAYS_FATAL_IF(!mEpollFd.ok(),

                        "Failed to create buffer release epoll file descriptor. errno=%d "
@@ -1412,8 +1457,8 @@ BLASTBufferQueue::BufferReleaseReader::BufferReleaseReader(BLASTBufferQueue& bbq 


    epoll_event registerEndpointFd{};

    registerEndpointFd.events = EPOLLIN;

    registerEndpointFd.data.fd = mBbq.mBufferReleaseConsumer->getFd();

    status_t status = epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, mBbq.mBufferReleaseConsumer->getFd(),

    registerEndpointFd.data.fd = mConsumerEndpoint->getFd();

    status_t status = epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, mConsumerEndpoint->getFd(),

                                &registerEndpointFd);

    LOG_ALWAYS_FATAL_IF(status == -1,

                        "Failed to register buffer release consumer file descriptor with epoll. "
@@ -1436,10 +1481,8 @@ BLASTBufferQueue::BufferReleaseReader::BufferReleaseReader(BLASTBufferQueue& bbq 
                        errno, strerror(errno));

}



status_t BLASTBufferQueue::BufferReleaseReader::readBlocking(ReleaseCallbackId& outId,

                                                             sp<Fence>& outFence,

                                                             uint32_t& outMaxAcquiredBufferCount,

                                                             nsecs_t timeout) {

status_t BufferReleaseReader::readBlocking(ReleaseCallbackId& outId, sp<Fence>& outFence,

                                           uint32_t& outMaxAcquiredBufferCount, nsecs_t timeout) {

    // TODO(b/363290953) epoll_wait only has millisecond timeout precision. If timeout is less than

    // 1ms, then we round timeout up to 1ms. Otherwise, we round timeout to the nearest

    // millisecond. Once epoll_pwait2 can be used in libgui, we can specify timeout with nanosecond
@@ -1479,17 +1522,21 @@ status_t BLASTBufferQueue::BufferReleaseReader::readBlocking(ReleaseCallbackId& 
        return WOULD_BLOCK;

    }



    return mBbq.mBufferReleaseConsumer->readReleaseFence(outId, outFence,

                                                         outMaxAcquiredBufferCount);

    return mConsumerEndpoint->readReleaseFence(outId, outFence, outMaxAcquiredBufferCount);

}



status_t BufferReleaseReader::readNonBlocking(ReleaseCallbackId& outId, sp<Fence>& outFence,

                                              uint32_t& outMaxAcquiredBufferCount) {

    return mConsumerEndpoint->readReleaseFence(outId, outFence, outMaxAcquiredBufferCount);

}



void BLASTBufferQueue::BufferReleaseReader::interruptBlockingRead() {

void BufferReleaseReader::interruptBlockingRead() {

    if (eventfd_write(mEventFd.get(), 1) == -1) {

        ALOGE("failed to notify dequeue event. errno=%d message='%s'", errno, strerror(errno));

    }

}



void BLASTBufferQueue::BufferReleaseReader::clearInterrupts() {

void BufferReleaseReader::clearInterrupts() {

    eventfd_t value;

    if (eventfd_read(mEventFd.get(), &value) == -1 && errno != EWOULDBLOCK) {

        ALOGE("error while reading from eventfd. errno=%d message='%s'", errno, strerror(errno));

libs/gui/include/gui/BLASTBufferQueue.h

+2 −34
Original line number Diff line number Diff line
@@ -43,6 +43,7 @@ constexpr size_t kDequeueTimestampsMapSizeHint = 32; 


class BLASTBufferQueue;

class BufferItemConsumer;

class BufferReleaseReader;



class BLASTBufferItemConsumer : public BufferItemConsumer {

public:
@@ -345,45 +346,12 @@ private: 
#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL)

    // BufferReleaseChannel is used to communicate buffer releases from SurfaceFlinger to the

    // client.

    std::unique_ptr<gui::BufferReleaseChannel::ConsumerEndpoint> mBufferReleaseConsumer;

    std::shared_ptr<gui::BufferReleaseChannel::ProducerEndpoint> mBufferReleaseProducer;



    void updateBufferReleaseProducer() REQUIRES(mMutex);

    void drainBufferReleaseConsumer();



    // BufferReleaseReader is used to do blocking but interruptible reads from the buffer

    // release channel. To implement this, BufferReleaseReader owns an epoll file descriptor that

    // is configured to wake up when either the BufferReleaseReader::ConsumerEndpoint or an eventfd

    // becomes readable. Interrupts are necessary because a free buffer may become available for

    // reasons other than a buffer release from the producer.

    class BufferReleaseReader {

    public:

        explicit BufferReleaseReader(BLASTBufferQueue&);



        BufferReleaseReader(const BufferReleaseReader&) = delete;

        BufferReleaseReader& operator=(const BufferReleaseReader&) = delete;



        // Block until we can read a buffer release message.

        //

        // Returns:

        // * OK if a ReleaseCallbackId and Fence were successfully read.

        // * WOULD_BLOCK if the blocking read was interrupted by interruptBlockingRead.

        // * TIMED_OUT if the blocking read timed out.

        // * UNKNOWN_ERROR if something went wrong.

        status_t readBlocking(ReleaseCallbackId& outId, sp<Fence>& outReleaseFence,

                              uint32_t& outMaxAcquiredBufferCount, nsecs_t timeout);



        void interruptBlockingRead();

        void clearInterrupts();



    private:

        BLASTBufferQueue& mBbq;



        android::base::unique_fd mEpollFd;

        android::base::unique_fd mEventFd;

    };



    std::optional<BufferReleaseReader> mBufferReleaseReader;

    std::shared_ptr<BufferReleaseReader> mBufferReleaseReader;

#endif

};