Merge "graphics: fix a potential use after free"
diff --git a/graphics/composer/2.1/default/ComposerClient.cpp b/graphics/composer/2.1/default/ComposerClient.cpp
index 0114430..a2d5d4b 100644
--- a/graphics/composer/2.1/default/ComposerClient.cpp
+++ b/graphics/composer/2.1/default/ComposerClient.cpp
@@ -193,30 +193,30 @@
} // anonymous namespace
-BufferClone::BufferClone()
+BufferCacheEntry::BufferCacheEntry()
: mHandle(nullptr)
{
}
-BufferClone::BufferClone(BufferClone&& other)
+BufferCacheEntry::BufferCacheEntry(BufferCacheEntry&& other)
{
mHandle = other.mHandle;
other.mHandle = nullptr;
}
-BufferClone& BufferClone::operator=(buffer_handle_t handle)
+BufferCacheEntry& BufferCacheEntry::operator=(buffer_handle_t handle)
{
clear();
mHandle = handle;
return *this;
}
-BufferClone::~BufferClone()
+BufferCacheEntry::~BufferCacheEntry()
{
clear();
}
-void BufferClone::clear()
+void BufferCacheEntry::clear()
{
if (mHandle) {
sHandleImporter.freeBuffer(mHandle);
@@ -745,10 +745,13 @@
auto damage = readRegion((length - 4) / 4);
auto err = lookupBuffer(BufferCache::CLIENT_TARGETS,
- slot, useCache, clientTarget);
+ slot, useCache, clientTarget, &clientTarget);
if (err == Error::NONE) {
err = mHal.setClientTarget(mDisplay, clientTarget, fence,
dataspace, damage);
+
+ updateBuffer(BufferCache::CLIENT_TARGETS, slot, useCache,
+ clientTarget);
}
if (err != Error::NONE) {
close(fence);
@@ -770,9 +773,12 @@
auto fence = readFence();
auto err = lookupBuffer(BufferCache::OUTPUT_BUFFERS,
- slot, useCache, outputBuffer);
+ slot, useCache, outputBuffer, &outputBuffer);
if (err == Error::NONE) {
err = mHal.setOutputBuffer(mDisplay, outputBuffer, fence);
+
+ updateBuffer(BufferCache::OUTPUT_BUFFERS,
+ slot, useCache, outputBuffer);
}
if (err != Error::NONE) {
close(fence);
@@ -870,9 +876,12 @@
auto fence = readFence();
auto err = lookupBuffer(BufferCache::LAYER_BUFFERS,
- slot, useCache, buffer);
+ slot, useCache, buffer, &buffer);
if (err == Error::NONE) {
err = mHal.setLayerBuffer(mDisplay, mLayer, buffer, fence);
+
+ updateBuffer(BufferCache::LAYER_BUFFERS,
+ slot, useCache, buffer);
}
if (err != Error::NONE) {
close(fence);
@@ -991,9 +1000,11 @@
auto stream = readHandle();
- auto err = lookupLayerSidebandStream(stream);
+ auto err = lookupLayerSidebandStream(stream, &stream);
if (err == Error::NONE) {
err = mHal.setLayerSidebandStream(mDisplay, mLayer, stream);
+
+ updateLayerSidebandStream(stream);
}
if (err != Error::NONE) {
mWriter.setError(getCommandLoc(), err);
@@ -1092,26 +1103,24 @@
};
}
-Error ComposerClient::CommandReader::lookupBuffer(BufferCache cache,
- uint32_t slot, bool useCache, buffer_handle_t& handle)
+Error ComposerClient::CommandReader::lookupBufferCacheEntryLocked(
+ BufferCache cache, uint32_t slot, BufferCacheEntry** outEntry)
{
- std::lock_guard<std::mutex> lock(mClient.mDisplayDataMutex);
-
auto dpy = mClient.mDisplayData.find(mDisplay);
if (dpy == mClient.mDisplayData.end()) {
return Error::BAD_DISPLAY;
}
- BufferClone* clone = nullptr;
+ BufferCacheEntry* entry = nullptr;
switch (cache) {
case BufferCache::CLIENT_TARGETS:
if (slot < dpy->second.ClientTargets.size()) {
- clone = &dpy->second.ClientTargets[slot];
+ entry = &dpy->second.ClientTargets[slot];
}
break;
case BufferCache::OUTPUT_BUFFERS:
if (slot < dpy->second.OutputBuffers.size()) {
- clone = &dpy->second.OutputBuffers[slot];
+ entry = &dpy->second.OutputBuffers[slot];
}
break;
case BufferCache::LAYER_BUFFERS:
@@ -1121,7 +1130,7 @@
return Error::BAD_LAYER;
}
if (slot < ly->second.Buffers.size()) {
- clone = &ly->second.Buffers[slot];
+ entry = &ly->second.Buffers[slot];
}
}
break;
@@ -1132,7 +1141,7 @@
return Error::BAD_LAYER;
}
if (slot == 0) {
- clone = &ly->second.SidebandStream;
+ entry = &ly->second.SidebandStream;
}
}
break;
@@ -1140,25 +1149,59 @@
break;
}
- if (!clone) {
- ALOGW("invalid buffer slot");
+ if (!entry) {
+ ALOGW("invalid buffer slot %" PRIu32, slot);
return Error::BAD_PARAMETER;
}
- // use or update cache
+ *outEntry = entry;
+
+ return Error::NONE;
+}
+
+Error ComposerClient::CommandReader::lookupBuffer(BufferCache cache,
+ uint32_t slot, bool useCache, buffer_handle_t handle,
+ buffer_handle_t* outHandle)
+{
if (useCache) {
- handle = *clone;
+ std::lock_guard<std::mutex> lock(mClient.mDisplayDataMutex);
+
+ BufferCacheEntry* entry;
+ Error error = lookupBufferCacheEntryLocked(cache, slot, &entry);
+ if (error != Error::NONE) {
+ return error;
+ }
+
+ // input handle is ignored
+ *outHandle = entry->getHandle();
} else {
if (!sHandleImporter.importBuffer(handle)) {
return Error::NO_RESOURCES;
}
- *clone = handle;
+ *outHandle = handle;
}
return Error::NONE;
}
+void ComposerClient::CommandReader::updateBuffer(BufferCache cache,
+ uint32_t slot, bool useCache, buffer_handle_t handle)
+{
+ // handle was looked up from cache
+ if (useCache) {
+ return;
+ }
+
+ std::lock_guard<std::mutex> lock(mClient.mDisplayDataMutex);
+
+ BufferCacheEntry* entry = nullptr;
+ lookupBufferCacheEntryLocked(cache, slot, &entry);
+ LOG_FATAL_IF(!entry, "failed to find the cache entry to update");
+
+ *entry = handle;
+}
+
} // namespace implementation
} // namespace V2_1
} // namespace composer
diff --git a/graphics/composer/2.1/default/ComposerClient.h b/graphics/composer/2.1/default/ComposerClient.h
index d351cfb..14da1f8 100644
--- a/graphics/composer/2.1/default/ComposerClient.h
+++ b/graphics/composer/2.1/default/ComposerClient.h
@@ -31,18 +31,18 @@
namespace V2_1 {
namespace implementation {
-class BufferClone {
+class BufferCacheEntry {
public:
- BufferClone();
- BufferClone(BufferClone&& other);
+ BufferCacheEntry();
+ BufferCacheEntry(BufferCacheEntry&& other);
- BufferClone(const BufferClone& other) = delete;
- BufferClone& operator=(const BufferClone& other) = delete;
+ BufferCacheEntry(const BufferCacheEntry& other) = delete;
+ BufferCacheEntry& operator=(const BufferCacheEntry& other) = delete;
- BufferClone& operator=(buffer_handle_t handle);
- ~BufferClone();
+ BufferCacheEntry& operator=(buffer_handle_t handle);
+ ~BufferCacheEntry();
- operator buffer_handle_t() const { return mHandle; }
+ buffer_handle_t getHandle() const { return mHandle; }
private:
void clear();
@@ -108,15 +108,15 @@
protected:
struct LayerBuffers {
- std::vector<BufferClone> Buffers;
- BufferClone SidebandStream;
+ std::vector<BufferCacheEntry> Buffers;
+ BufferCacheEntry SidebandStream;
};
struct DisplayData {
bool IsVirtual;
- std::vector<BufferClone> ClientTargets;
- std::vector<BufferClone> OutputBuffers;
+ std::vector<BufferCacheEntry> ClientTargets;
+ std::vector<BufferCacheEntry> OutputBuffers;
std::unordered_map<Layer, LayerBuffers> Layers;
@@ -167,12 +167,23 @@
LAYER_BUFFERS,
LAYER_SIDEBAND_STREAMS,
};
+ Error lookupBufferCacheEntryLocked(BufferCache cache, uint32_t slot,
+ BufferCacheEntry** outEntry);
Error lookupBuffer(BufferCache cache, uint32_t slot,
- bool useCache, buffer_handle_t& handle);
+ bool useCache, buffer_handle_t handle,
+ buffer_handle_t* outHandle);
+ void updateBuffer(BufferCache cache, uint32_t slot,
+ bool useCache, buffer_handle_t handle);
- Error lookupLayerSidebandStream(buffer_handle_t& handle)
+ Error lookupLayerSidebandStream(buffer_handle_t handle,
+ buffer_handle_t* outHandle)
{
return lookupBuffer(BufferCache::LAYER_SIDEBAND_STREAMS,
+ 0, false, handle, outHandle);
+ }
+ void updateLayerSidebandStream(buffer_handle_t handle)
+ {
+ updateBuffer(BufferCache::LAYER_SIDEBAND_STREAMS,
0, false, handle);
}