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

Commit 96ad8222 authored by Chia-I Wu's avatar Chia-I Wu
Browse files

libgui: fix a race in CpuConsumer::lockNextBuffer 

mCurrentLockedBuffers must be accessed with the mutex.  Constify
mMaxLockedBuffers as well for clarity.

Test: libgui_test
Change-Id: I7c40d135bcf2aa6a90ecff74d6d8107fc530e815
parent 7e7256ee

libs/gui/CpuConsumer.cpp

+3 −3
Original line number Diff line number Diff line
@@ -168,6 +168,9 @@ status_t CpuConsumer::lockNextBuffer(LockedBuffer *nativeBuffer) { 
    status_t err;



    if (!nativeBuffer) return BAD_VALUE;



    Mutex::Autolock _l(mMutex);



    if (mCurrentLockedBuffers == mMaxLockedBuffers) {

        CC_LOGW("Max buffers have been locked (%zd), cannot lock anymore.",

                mMaxLockedBuffers);
@@ -175,9 +178,6 @@ status_t CpuConsumer::lockNextBuffer(LockedBuffer *nativeBuffer) { 
    }



    BufferItem b;



    Mutex::Autolock _l(mMutex);



    err = acquireBufferLocked(&b, 0);

    if (err != OK) {

        if (err == BufferQueue::NO_BUFFER_AVAILABLE) {

libs/gui/include/gui/CpuConsumer.h

+1 −1
Original line number Diff line number Diff line
@@ -119,7 +119,7 @@ class CpuConsumer : public ConsumerBase 


  private:

    // Maximum number of buffers that can be locked at a time

    size_t mMaxLockedBuffers;

    const size_t mMaxLockedBuffers;



    status_t releaseAcquiredBufferLocked(size_t lockedIdx);

libs/gui/tests/CpuConsumer_test.cpp

+56 −0
Original line number Diff line number Diff line
@@ -33,6 +33,7 @@ 
#include <utils/Mutex.h>

#include <utils/Condition.h>



#include <thread>

#include <vector>

#define CPU_CONSUMER_TEST_FORMAT_RAW 0

#define CPU_CONSUMER_TEST_FORMAT_Y8 0
@@ -690,6 +691,61 @@ TEST_P(CpuConsumerTest, FromCpuInvalid) { 
    ASSERT_EQ(BAD_VALUE, err) << "unlockBuffer did not fail";

}



TEST_P(CpuConsumerTest, FromCpuMultiThread) {

    CpuConsumerTestParams params = GetParam();

    ASSERT_NO_FATAL_FAILURE(configureANW(mANW, params, params.maxLockedBuffers + 1));



    for (int i = 0; i < 10; i++) {

        std::atomic<int> threadReadyCount(0);

        auto lockAndUnlock = [&]() {

            threadReadyCount++;

            // busy wait

            while (threadReadyCount < params.maxLockedBuffers + 1);



            CpuConsumer::LockedBuffer b;

            status_t err = mCC->lockNextBuffer(&b);

            if (err == NO_ERROR) {

                usleep(1000);

                err = mCC->unlockBuffer(b);

                ASSERT_NO_ERROR(err, "Could not unlock buffer: ");

            } else if (err == NOT_ENOUGH_DATA) {

                // there are params.maxLockedBuffers+1 threads so one of the

                // threads might get this error

            } else {

                FAIL() << "Could not lock buffer";

            }

        };



        // produce buffers

        for (int j = 0; j < params.maxLockedBuffers + 1; j++) {

            const int64_t time = 1234L;

            uint32_t stride;

            ASSERT_NO_FATAL_FAILURE(produceOneFrame(mANW, params, time, &stride));

        }



        // spawn threads

        std::vector<std::thread> threads;

        for (int j = 0; j < params.maxLockedBuffers + 1; j++) {

            threads.push_back(std::thread(lockAndUnlock));

        }



        // join threads

        for (auto& thread : threads) {

            thread.join();

        }



        // we produced N+1 buffers, but the threads might only consume N

        CpuConsumer::LockedBuffer b;

        if (mCC->lockNextBuffer(&b) == NO_ERROR) {

            mCC->unlockBuffer(b);

        }



        if (HasFatalFailure()) {

            break;

        }

    }

}



CpuConsumerTestParams y8TestSets[] = {

    { 512,   512, 1, HAL_PIXEL_FORMAT_Y8},

    { 512,   512, 3, HAL_PIXEL_FORMAT_Y8},