Fix race condition in ConsumerBase::addReleaseFence()

This needs the ConsumerBase mutex locked, but wasn't locking it. Two
of the four places that called it already held the lock so were fine.
Now addReleaseFence() takes the lock itself, and I added
addReleaseFenceLocked() for the two already-locked callers, since in
one of them dropping the lock would be inconvenient.

Bug: 7289269
Change-Id: I7a5628adb516f8eec782aa6c14128202f96d7b0a

diff --git a/include/gui/ConsumerBase.h b/include/gui/ConsumerBase.h
index 68cce5a..ee5cb29 100644 (file)
--- a/include/gui/ConsumerBase.h
+++ b/include/gui/ConsumerBase.h
@@ -150,16 +150,17 @@ protected:
     // Derived classes should override this method to perform any cleanup that
     // must take place when a buffer is released back to the BufferQueue.  If
     // it is overridden the derived class's implementation must call
-    // ConsumerBase::acquireBufferLocked.
+    // ConsumerBase::releaseBufferLocked.
     virtual status_t releaseBufferLocked(int buf, EGLDisplay display,
            EGLSyncKHR eglFence);
 
-    // addReleaseFence adds the sync points associated with a fence to the set
+    // addReleaseFence* adds the sync points associated with a fence to the set
     // of sync points that must be reached before the buffer in the given slot
     // may be used after the slot has been released.  This should be called by
     // derived classes each time some asynchronous work is kicked off that
     // references the buffer.
     status_t addReleaseFence(int slot, const sp<Fence>& fence);
+    status_t addReleaseFenceLocked(int slot, const sp<Fence>& fence);
 
     // Slot contains the information and object references that
     // ConsumerBase maintains about a BufferQueue buffer slot.

diff --git a/libs/gui/BufferItemConsumer.cpp b/libs/gui/BufferItemConsumer.cpp
index fdfd15e..5079883 100644 (file)
--- a/libs/gui/BufferItemConsumer.cpp
+++ b/libs/gui/BufferItemConsumer.cpp
@@ -82,7 +82,7 @@ status_t BufferItemConsumer::releaseBuffer(const BufferItem &item,
 
     Mutex::Autolock _l(mMutex);
 
-    err = addReleaseFence(item.mBuf, releaseFence);
+    err = addReleaseFenceLocked(item.mBuf, releaseFence);
 
     err = releaseBufferLocked(item.mBuf, EGL_NO_DISPLAY,
             EGL_NO_SYNC_KHR);

diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp
index f5d6ff0..624d7e0 100644 (file)
--- a/libs/gui/ConsumerBase.cpp
+++ b/libs/gui/ConsumerBase.cpp
@@ -193,7 +193,12 @@ status_t ConsumerBase::acquireBufferLocked(BufferQueue::BufferItem *item) {
 }
 
 status_t ConsumerBase::addReleaseFence(int slot, const sp<Fence>& fence) {
-    CB_LOGV("addReleaseFence: slot=%d", slot);
+    Mutex::Autolock lock(mMutex);
+    return addReleaseFenceLocked(slot, fence);
+}
+
+status_t ConsumerBase::addReleaseFenceLocked(int slot, const sp<Fence>& fence) {
+    CB_LOGV("addReleaseFenceLocked: slot=%d", slot);
 
     if (!mSlots[slot].mFence.get()) {
         mSlots[slot].mFence = fence;

diff --git a/libs/gui/SurfaceTexture.cpp b/libs/gui/SurfaceTexture.cpp
index 57bcd2c..b4dfb5e 100644 (file)
--- a/libs/gui/SurfaceTexture.cpp
+++ b/libs/gui/SurfaceTexture.cpp
@@ -484,7 +484,7 @@ status_t SurfaceTexture::syncForReleaseLocked(EGLDisplay dpy) {
                 return UNKNOWN_ERROR;
             }
             sp<Fence> fence(new Fence(fenceFd));
-            status_t err = addReleaseFence(mCurrentTexture, fence);
+            status_t err = addReleaseFenceLocked(mCurrentTexture, fence);
             if (err != OK) {
                 ST_LOGE("syncForReleaseLocked: error adding release fence: "
                         "%s (%d)", strerror(-err), err);