libgui: add helper to find CpuConsumer::AcquiredBuffer
Add CpuConsumer::findAcquiredBufferLocked. Rename mBufferPointer to
mLockedBufferId.
This also fixes a nullptr-dereference when unlockBuffer is called
with a LockedBuffer whose data is nullptr.
Test: libgui_test
Change-Id: I36c665099f145731197a6ff8ab940c5d7a326df8
diff --git a/libs/gui/CpuConsumer.cpp b/libs/gui/CpuConsumer.cpp
index ae7c65c..1f6170c 100644
--- a/libs/gui/CpuConsumer.cpp
+++ b/libs/gui/CpuConsumer.cpp
@@ -60,6 +60,21 @@
mConsumer->setConsumerName(name);
}
+size_t CpuConsumer::findAcquiredBufferLocked(uintptr_t id) const {
+ for (size_t i = 0; i < mMaxLockedBuffers; i++) {
+ const auto& ab = mAcquiredBuffers[i];
+ // note that this finds AcquiredBuffer::kUnusedId as well
+ if (ab.mLockedBufferId == id) {
+ return i;
+ }
+ }
+ return mMaxLockedBuffers; // an invalid index
+}
+
+static uintptr_t getLockedBufferId(const CpuConsumer::LockedBuffer& buffer) {
+ return reinterpret_cast<uintptr_t>(buffer.data);
+}
+
static bool isPossiblyYUV(PixelFormat format) {
switch (static_cast<int>(format)) {
case HAL_PIXEL_FORMAT_RGBA_8888:
@@ -165,20 +180,6 @@
}
}
- size_t lockedIdx = 0;
- for (; lockedIdx < static_cast<size_t>(mMaxLockedBuffers); lockedIdx++) {
- if (mAcquiredBuffers[lockedIdx].mSlot ==
- BufferQueue::INVALID_BUFFER_SLOT) {
- break;
- }
- }
- assert(lockedIdx < mMaxLockedBuffers);
-
- AcquiredBuffer &ab = mAcquiredBuffers.editItemAt(lockedIdx);
- ab.mSlot = slot;
- ab.mBufferPointer = bufferPointer;
- ab.mGraphicBuffer = mSlots[slot].mGraphicBuffer;
-
nativeBuffer->data =
reinterpret_cast<uint8_t*>(bufferPointer);
nativeBuffer->width = mSlots[slot].mGraphicBuffer->getWidth();
@@ -201,6 +202,15 @@
nativeBuffer->chromaStride = static_cast<uint32_t>(ycbcr.cstride);
nativeBuffer->chromaStep = static_cast<uint32_t>(ycbcr.chroma_step);
+ // find an unused AcquiredBuffer
+ size_t lockedIdx = findAcquiredBufferLocked(AcquiredBuffer::kUnusedId);
+ ALOG_ASSERT(lockedIdx < mMaxLockedBuffers);
+ AcquiredBuffer& ab = mAcquiredBuffers.editItemAt(lockedIdx);
+
+ ab.mSlot = slot;
+ ab.mGraphicBuffer = mSlots[slot].mGraphicBuffer;
+ ab.mLockedBufferId = getLockedBufferId(*nativeBuffer);
+
mCurrentLockedBuffers++;
return OK;
@@ -208,12 +218,10 @@
status_t CpuConsumer::unlockBuffer(const LockedBuffer &nativeBuffer) {
Mutex::Autolock _l(mMutex);
- size_t lockedIdx = 0;
- void *bufPtr = reinterpret_cast<void *>(nativeBuffer.data);
- for (; lockedIdx < static_cast<size_t>(mMaxLockedBuffers); lockedIdx++) {
- if (bufPtr == mAcquiredBuffers[lockedIdx].mBufferPointer) break;
- }
+ uintptr_t id = getLockedBufferId(nativeBuffer);
+ size_t lockedIdx =
+ (id != AcquiredBuffer::kUnusedId) ? findAcquiredBufferLocked(id) : mMaxLockedBuffers;
if (lockedIdx == mMaxLockedBuffers) {
CC_LOGE("%s: Can't find buffer to free", __FUNCTION__);
return BAD_VALUE;
@@ -252,9 +260,7 @@
}
AcquiredBuffer &ab = mAcquiredBuffers.editItemAt(lockedIdx);
- ab.mSlot = BufferQueue::INVALID_BUFFER_SLOT;
- ab.mBufferPointer = NULL;
- ab.mGraphicBuffer.clear();
+ ab.reset();
mCurrentLockedBuffers--;
return OK;
diff --git a/libs/gui/include/gui/CpuConsumer.h b/libs/gui/include/gui/CpuConsumer.h
index 58602bf..a0891a4 100644
--- a/libs/gui/include/gui/CpuConsumer.h
+++ b/libs/gui/include/gui/CpuConsumer.h
@@ -127,18 +127,29 @@
// Tracking for buffers acquired by the user
struct AcquiredBuffer {
+ static constexpr uintptr_t kUnusedId = 0;
+
// Need to track the original mSlot index and the buffer itself because
// the mSlot entry may be freed/reused before the acquired buffer is
// released.
int mSlot;
sp<GraphicBuffer> mGraphicBuffer;
- void *mBufferPointer;
+ uintptr_t mLockedBufferId;
AcquiredBuffer() :
mSlot(BufferQueue::INVALID_BUFFER_SLOT),
- mBufferPointer(NULL) {
+ mLockedBufferId(kUnusedId) {
+ }
+
+ void reset() {
+ mSlot = BufferQueue::INVALID_BUFFER_SLOT;
+ mGraphicBuffer.clear();
+ mLockedBufferId = kUnusedId;
}
};
+
+ size_t findAcquiredBufferLocked(uintptr_t id) const;
+
Vector<AcquiredBuffer> mAcquiredBuffers;
// Count of currently locked buffers
diff --git a/libs/gui/tests/CpuConsumer_test.cpp b/libs/gui/tests/CpuConsumer_test.cpp
index 588e541..4ef82d8 100644
--- a/libs/gui/tests/CpuConsumer_test.cpp
+++ b/libs/gui/tests/CpuConsumer_test.cpp
@@ -681,6 +681,15 @@
}
}
+TEST_P(CpuConsumerTest, FromCpuInvalid) {
+ status_t err = mCC->lockNextBuffer(nullptr);
+ ASSERT_EQ(BAD_VALUE, err) << "lockNextBuffer did not fail";
+
+ CpuConsumer::LockedBuffer b;
+ err = mCC->unlockBuffer(b);
+ ASSERT_EQ(BAD_VALUE, err) << "unlockBuffer did not fail";
+}
+
CpuConsumerTestParams y8TestSets[] = {
{ 512, 512, 1, HAL_PIXEL_FORMAT_Y8},
{ 512, 512, 3, HAL_PIXEL_FORMAT_Y8},