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

Commit c54c1271 authored by Mathias Agopian's avatar Mathias Agopian
Browse files

fix a race condition in undoDequeue(), where 'tail' could be computed incorrectly. 

in the undoDequeue() case, 'tail' was recalculated from 'available' and 'head'
however there was a race between this and retireAndLock(), which could cause
'tail' to be recalculated wrongly.

the interesting thing though is that retireAndLock() shouldn't have any impact
on the value of 'tail', which is client-side only attribute.
we fix the race by saving the value of 'tail' before dequeue() and restore it
in the case of undoDequeue(), since we know it doesn't depend on retireAndLock().

Change-Id: I4bcc4d16b6bc4dd93717ee739c603040b18295a0
parent 3fd6419f

include/private/surfaceflinger/SharedBufferStack.h

+2 −2
Original line number Original line Diff line number Diff line
@@ -167,6 +167,7 @@ protected: 
    SharedBufferStack* const mSharedStack;

    SharedBufferStack* const mSharedStack;

    const int mNumBuffers;

    const int mNumBuffers;

    const int mIdentity;

    const int mIdentity;

    int32_t computeTail() const;





    friend struct Update;

    friend struct Update;

    friend struct QueueUpdate;

    friend struct QueueUpdate;
@@ -260,8 +261,6 @@ private: 
    friend struct DequeueCondition;

    friend struct DequeueCondition;

    friend struct LockCondition;

    friend struct LockCondition;





    int32_t computeTail() const;



    struct QueueUpdate : public UpdateBase {

    struct QueueUpdate : public UpdateBase {

        inline QueueUpdate(SharedBufferBase* sbb);

        inline QueueUpdate(SharedBufferBase* sbb);

        inline ssize_t operator()();

        inline ssize_t operator()();
@@ -288,6 +287,7 @@ private: 
    };

    };





    int32_t tail;

    int32_t tail;

    int32_t undoDequeueTail;

    // statistics...

    // statistics...

    nsecs_t mDequeueTime[NUM_BUFFER_MAX];

    nsecs_t mDequeueTime[NUM_BUFFER_MAX];

};

};

libs/surfaceflinger_client/SharedBufferStack.cpp

+11 −24
Original line number Original line Diff line number Diff line
@@ -179,7 +179,7 @@ String8 SharedBufferBase::dump(char const* prefix) const 
    char buffer[SIZE];

    char buffer[SIZE];

    String8 result;

    String8 result;

    SharedBufferStack& stack( *mSharedStack );

    SharedBufferStack& stack( *mSharedStack );

    int tail = (mNumBuffers + stack.head - stack.available + 1) % mNumBuffers;

    int tail = computeTail();

    snprintf(buffer, SIZE, 

    snprintf(buffer, SIZE, 

            "%s[ head=%2d, available=%2d, queued=%2d, tail=%2d ] "

            "%s[ head=%2d, available=%2d, queued=%2d, tail=%2d ] "

            "reallocMask=%08x, inUse=%2d, identity=%d, status=%d\n",

            "reallocMask=%08x, inUse=%2d, identity=%d, status=%d\n",
@@ -189,6 +189,12 @@ String8 SharedBufferBase::dump(char const* prefix) const 
    return result;

    return result;

}

}





int32_t SharedBufferBase::computeTail() const

{

    SharedBufferStack& stack( *mSharedStack );

    return (mNumBuffers + stack.head - stack.available + 1) % mNumBuffers;

}



// ============================================================================

// ============================================================================

// conditions and updates

// conditions and updates

// ============================================================================

// ============================================================================
@@ -297,32 +303,12 @@ ssize_t SharedBufferServer::StatusUpdate::operator()() { 




SharedBufferClient::SharedBufferClient(SharedClient* sharedClient,

SharedBufferClient::SharedBufferClient(SharedClient* sharedClient,

        int surface, int num, int32_t identity)

        int surface, int num, int32_t identity)

    : SharedBufferBase(sharedClient, surface, num, identity), tail(0)

    : SharedBufferBase(sharedClient, surface, num, identity),

      tail(0), undoDequeueTail(0)

{

{

    tail = computeTail();

    tail = computeTail();

}

}





int32_t SharedBufferClient::computeTail() const

{

    SharedBufferStack& stack( *mSharedStack );

    // we need to make sure we read available and head coherently,

    // w.r.t RetireUpdate.

    int32_t newTail;

    int32_t avail;

    int32_t head;

    do {

        avail = stack.available;

        head = stack.head;

    } while (stack.available != avail);

    newTail = head - avail + 1;

    if (newTail < 0) {

        newTail += mNumBuffers;

    } else if (newTail >= mNumBuffers) {

        newTail -= mNumBuffers;

    }

    return newTail;

}



ssize_t SharedBufferClient::dequeue()

ssize_t SharedBufferClient::dequeue()

{

{

    SharedBufferStack& stack( *mSharedStack );

    SharedBufferStack& stack( *mSharedStack );
@@ -350,6 +336,7 @@ ssize_t SharedBufferClient::dequeue() 




    int dequeued = tail;

    int dequeued = tail;

    tail = ((tail+1 >= mNumBuffers) ? 0 : tail+1);

    tail = ((tail+1 >= mNumBuffers) ? 0 : tail+1);

    undoDequeueTail = dequeued;

    LOGD_IF(DEBUG_ATOMICS, "dequeued=%d, tail=%d, %s",

    LOGD_IF(DEBUG_ATOMICS, "dequeued=%d, tail=%d, %s",

            dequeued, tail, dump("").string());

            dequeued, tail, dump("").string());
@@ -363,7 +350,7 @@ status_t SharedBufferClient::undoDequeue(int buf) 
    UndoDequeueUpdate update(this);

    UndoDequeueUpdate update(this);

    status_t err = updateCondition( update );

    status_t err = updateCondition( update );

    if (err == NO_ERROR) {

    if (err == NO_ERROR) {

        tail = computeTail();

        tail = undoDequeueTail;

    }

    }

    return err;

    return err;

}

}