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

Commit fd4edb2e authored by Jim Shargo's avatar Jim Shargo
Browse files

BufferQueue: Fix deadlock in setMaxAcquiredBufferCount 

Uncovered this while testing. The deadlock happens when:

- ConsumerBase::setMaxAcquiredBufferCount locks itself
- Calls IGBC::setMaxAcquiredBufferCount, which can call
  ConsumerListener::onBuffersReleased
- Which, in ConsumerBase, will take the lock again

Instead of this, we add a callback to be called instead of the
IConsumerListener. This callback is called on the same stack, with the
lock held, so that we can resolve everything atomically.

Bug: b/393639203
Flag: EXEMPT small cleanup
Test: new test
Change-Id: Iddd8f902d1fd0aeed6aac095eaa6c0b870ffff70
parent b61e79d3

libs/gui/BufferQueueConsumer.cpp

+22 −10
Original line number Diff line number Diff line
@@ -14,10 +14,6 @@ 
 * limitations under the License.

 */



#include <inttypes.h>

#include <pwd.h>

#include <sys/types.h>



#define LOG_TAG "BufferQueueConsumer"

#define ATRACE_TAG ATRACE_TAG_GRAPHICS

//#define LOG_NDEBUG 0
@@ -48,6 +44,11 @@ 


#include <com_android_graphics_libgui_flags.h>



#include <inttypes.h>

#include <pwd.h>

#include <sys/types.h>

#include <optional>



namespace android {



// Macros for include BufferQueueCore information in log messages
@@ -767,11 +768,15 @@ status_t BufferQueueConsumer::setMaxBufferCount(int bufferCount) { 
    return NO_ERROR;

}



status_t BufferQueueConsumer::setMaxAcquiredBufferCount(int maxAcquiredBuffers) {

    return setMaxAcquiredBufferCount(maxAcquiredBuffers, std::nullopt);

}



status_t BufferQueueConsumer::setMaxAcquiredBufferCount(

        int maxAcquiredBuffers) {

        int maxAcquiredBuffers, std::optional<OnBufferReleasedCallback> onBuffersReleasedCallback) {

    ATRACE_FORMAT("%s(%d)", __func__, maxAcquiredBuffers);



    sp<IConsumerListener> listener;

    std::optional<OnBufferReleasedCallback> callback;

    { // Autolock scope

        std::unique_lock<std::mutex> lock(mCore->mMutex);
@@ -833,13 +838,20 @@ status_t BufferQueueConsumer::setMaxAcquiredBufferCount( 
        BQ_LOGV("setMaxAcquiredBufferCount: %d", maxAcquiredBuffers);

        mCore->mMaxAcquiredBufferCount = maxAcquiredBuffers;

        VALIDATE_CONSISTENCY();

        if (delta < 0 && mCore->mBufferReleasedCbEnabled) {

            listener = mCore->mConsumerListener;

        if (delta < 0) {

            if (onBuffersReleasedCallback) {

                callback = std::move(onBuffersReleasedCallback);

            } else if (mCore->mBufferReleasedCbEnabled) {

                callback = [listener = mCore->mConsumerListener]() {

                    listener->onBuffersReleased();

                };

            }

        }

    }



    // Call back without lock held

    if (listener != nullptr) {

        listener->onBuffersReleased();

    if (callback) {

        (*callback)();

    }



    return NO_ERROR;

libs/gui/ConsumerBase.cpp

+5 −1
Original line number Diff line number Diff line
@@ -264,7 +264,10 @@ void ConsumerBase::onFrameReplaced(const BufferItem &item) { 


void ConsumerBase::onBuffersReleased() {

    Mutex::Autolock lock(mMutex);

    onBuffersReleasedLocked();

}



void ConsumerBase::onBuffersReleasedLocked() {

    CB_LOGV("onBuffersReleased");



    if (mAbandoned) {
@@ -481,7 +484,8 @@ status_t ConsumerBase::setMaxAcquiredBufferCount(int maxAcquiredBuffers) { 
        CB_LOGE("setMaxAcquiredBufferCount: ConsumerBase is abandoned!");

        return NO_INIT;

    }

    return mConsumer->setMaxAcquiredBufferCount(maxAcquiredBuffers);

    return mConsumer->setMaxAcquiredBufferCount(maxAcquiredBuffers,

                                                {[this]() { onBuffersReleasedLocked(); }});

}



status_t ConsumerBase::setConsumerIsProtected(bool isProtected) {

libs/gui/include/gui/BufferQueueConsumer.h

+4 −1
Original line number Diff line number Diff line
@@ -122,7 +122,10 @@ public: 
    // setMaxAcquiredBufferCount sets the maximum number of buffers that can

    // be acquired by the consumer at one time (default 1).  This call will

    // fail if a producer is connected to the BufferQueue.

    virtual status_t setMaxAcquiredBufferCount(int maxAcquiredBuffers);

    virtual status_t setMaxAcquiredBufferCount(int maxAcquiredBuffers) override;

    virtual status_t setMaxAcquiredBufferCount(

            int maxAcquiredBuffers,

            std::optional<OnBufferReleasedCallback> onBuffersReleasedCallback) override;



    // setConsumerName sets the name used in logging

    status_t setConsumerName(const String8& name) override;

libs/gui/include/gui/ConsumerBase.h

+2 −0
Original line number Diff line number Diff line
@@ -191,6 +191,8 @@ protected: 
#endif

    virtual int getSlotForBufferLocked(const sp<GraphicBuffer>& buffer);



    virtual void onBuffersReleasedLocked();



    virtual status_t detachBufferLocked(int slotIndex);



    // freeBufferLocked frees up the given buffer slot.  If the slot has been

libs/gui/include/gui/IGraphicBufferConsumer.h

+8 −0
Original line number Diff line number Diff line
@@ -243,6 +243,9 @@ public: 
    // maxAcquiredBuffers must be (inclusive) between 1 and MAX_MAX_ACQUIRED_BUFFERS. It also cannot

    // cause the maxBufferCount value to be exceeded.

    //

    // If called with onBuffersReleasedCallback, that call back will be called in lieu of

    // IConsumerListener::onBuffersReleased.

    //

    // Return of a value other than NO_ERROR means an error has occurred:

    // * NO_INIT - the BufferQueue has been abandoned

    // * BAD_VALUE - one of the below conditions occurred:
@@ -253,6 +256,11 @@ public: 
    // * INVALID_OPERATION - attempting to call this after a producer connected.

    virtual status_t setMaxAcquiredBufferCount(int maxAcquiredBuffers) = 0;



    using OnBufferReleasedCallback = std::function<void(void)>;

    virtual status_t setMaxAcquiredBufferCount(

            int maxAcquiredBuffers,

            std::optional<OnBufferReleasedCallback> onBuffersReleasedCallback) = 0;



    // setConsumerName sets the name used in logging

    virtual status_t setConsumerName(const String8& name) = 0;