From 0a8cd0689a76dbca7405004caac4dde4c0250aea Mon Sep 17 00:00:00 2001
From: Mathias Agopian <mathias@google.com>
Date: Tue, 27 Apr 2010 16:11:38 -0700
Subject: [PATCH] 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
---
 .../surfaceflinger/SharedBufferStack.h        |  4 +--
 .../SharedBufferStack.cpp                     | 35 ++++++-------------
 2 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/include/private/surfaceflinger/SharedBufferStack.h b/include/private/surfaceflinger/SharedBufferStack.h
index 6ace5bc98..39ef3a186 100644
--- a/include/private/surfaceflinger/SharedBufferStack.h
+++ b/include/private/surfaceflinger/SharedBufferStack.h
@@ -167,6 +167,7 @@ protected:
     SharedBufferStack* const mSharedStack;
     const int mNumBuffers;
     const int mIdentity;
+    int32_t computeTail() const;
 
     friend struct Update;
     friend struct QueueUpdate;
@@ -259,8 +260,6 @@ private:
     friend struct Condition;
     friend struct DequeueCondition;
     friend struct LockCondition;
-    
-    int32_t computeTail() const;
 
     struct QueueUpdate : public UpdateBase {
         inline QueueUpdate(SharedBufferBase* sbb);
@@ -288,6 +287,7 @@ private:
     };
 
     int32_t tail;
+    int32_t undoDequeueTail;
     // statistics...
     nsecs_t mDequeueTime[NUM_BUFFER_MAX];
 };
diff --git a/libs/surfaceflinger_client/SharedBufferStack.cpp b/libs/surfaceflinger_client/SharedBufferStack.cpp
index f1aec8ab7..674570477 100644
--- a/libs/surfaceflinger_client/SharedBufferStack.cpp
+++ b/libs/surfaceflinger_client/SharedBufferStack.cpp
@@ -179,7 +179,7 @@ String8 SharedBufferBase::dump(char const* prefix) const
     char buffer[SIZE];
     String8 result;
     SharedBufferStack& stack( *mSharedStack );
-    int tail = (mNumBuffers + stack.head - stack.available + 1) % mNumBuffers;
+    int tail = computeTail();
     snprintf(buffer, SIZE, 
             "%s[ head=%2d, available=%2d, queued=%2d, tail=%2d ] "
             "reallocMask=%08x, inUse=%2d, identity=%d, status=%d\n",
@@ -189,6 +189,12 @@ String8 SharedBufferBase::dump(char const* prefix) const
     return result;
 }
 
+int32_t SharedBufferBase::computeTail() const
+{
+    SharedBufferStack& stack( *mSharedStack );
+    return (mNumBuffers + stack.head - stack.available + 1) % mNumBuffers;
+}
+
 // ============================================================================
 // conditions and updates
 // ============================================================================
@@ -297,32 +303,12 @@ ssize_t SharedBufferServer::StatusUpdate::operator()() {
 
 SharedBufferClient::SharedBufferClient(SharedClient* sharedClient,
         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();
 }
 
-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()
 {
     SharedBufferStack& stack( *mSharedStack );
@@ -350,6 +336,7 @@ ssize_t SharedBufferClient::dequeue()
 
     int dequeued = tail;
     tail = ((tail+1 >= mNumBuffers) ? 0 : tail+1);
+    undoDequeueTail = dequeued;
     LOGD_IF(DEBUG_ATOMICS, "dequeued=%d, tail=%d, %s",
             dequeued, tail, dump("").string());
 
@@ -363,7 +350,7 @@ status_t SharedBufferClient::undoDequeue(int buf)
     UndoDequeueUpdate update(this);
     status_t err = updateCondition( update );
     if (err == NO_ERROR) {
-        tail = computeTail();
+        tail = undoDequeueTail;
     }
     return err;
 }