From 78b63dc6a1aff1cecd706808aeb950966c654f14 Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Wed, 19 Apr 2017 11:03:00 -0700 Subject: [PATCH] graphics: keep mapper valid during process termination GraphicBufferMapper is valid during process termination. IMapper must stay valid as well. It should not rely on global/static objects that may have been destructed. Bug: 37480313 Test: libgui_test Change-Id: Icb8079153306f2465c26c0f1ce812895ad83f21b --- graphics/mapper/2.0/default/GrallocMapper.cpp | 72 ++++++++++++------- graphics/mapper/2.0/default/GrallocMapper.h | 8 --- 2 files changed, 45 insertions(+), 35 deletions(-) diff --git a/graphics/mapper/2.0/default/GrallocMapper.cpp b/graphics/mapper/2.0/default/GrallocMapper.cpp index 339317aff8..6441af6396 100644 --- a/graphics/mapper/2.0/default/GrallocMapper.cpp +++ b/graphics/mapper/2.0/default/GrallocMapper.cpp @@ -36,8 +36,45 @@ namespace implementation { using android::hardware::graphics::common::V1_0::BufferUsage; using android::hardware::graphics::common::V1_0::PixelFormat; -std::mutex GrallocMapper::mMutex; -std::unordered_set GrallocMapper::mRegisteredHandles; +namespace { + +class RegisteredHandlePool { + public: + bool add(buffer_handle_t bufferHandle) { + std::lock_guard lock(mMutex); + return mHandles.insert(bufferHandle).second; + } + + native_handle_t* pop(void* buffer) { + auto bufferHandle = static_cast(buffer); + + std::lock_guard lock(mMutex); + return mHandles.erase(bufferHandle) == 1 ? bufferHandle : nullptr; + } + + buffer_handle_t get(const void* buffer) { + auto bufferHandle = static_cast(buffer); + + std::lock_guard lock(mMutex); + return mHandles.count(bufferHandle) == 1 ? bufferHandle : nullptr; + } + + private: + std::mutex mMutex; + std::unordered_set mHandles; +}; + +// GraphicBufferMapper is expected to be valid (and leaked) during process +// termination. We need to make sure IMapper, and in turn, gRegisteredHandles +// are valid as well. Create the registered handle pool on the heap, and let +// it leak for simplicity. +// +// However, there is no way to make sure gralloc0/gralloc1 are valid. Any use +// of static/global object in gralloc0/gralloc1 that may have been destructed +// is potentially broken. +RegisteredHandlePool* gRegisteredHandles = new RegisteredHandlePool; + +} // anonymous namespace bool GrallocMapper::validateDescriptorInfo( const BufferDescriptorInfo& descriptorInfo) const { @@ -86,29 +123,10 @@ Return GrallocMapper::createDescriptor( return Void(); } -bool GrallocMapper::addRegisteredHandle(buffer_handle_t bufferHandle) { - std::lock_guard lock(mMutex); - return mRegisteredHandles.insert(bufferHandle).second; -} - -native_handle_t* GrallocMapper::popRegisteredHandle(void* buffer) { - auto bufferHandle = static_cast(buffer); - - std::lock_guard lock(mMutex); - return mRegisteredHandles.erase(bufferHandle) == 1 ? bufferHandle : nullptr; -} - -buffer_handle_t GrallocMapper::getRegisteredHandle(const void* buffer) { - auto bufferHandle = static_cast(buffer); - - std::lock_guard lock(mMutex); - return mRegisteredHandles.count(bufferHandle) == 1 ? bufferHandle : nullptr; -} - Return GrallocMapper::importBuffer(const hidl_handle& rawHandle, importBuffer_cb hidl_cb) { // importing an already imported handle rather than a raw handle - if (getRegisteredHandle(rawHandle.getNativeHandle())) { + if (gRegisteredHandles->get(rawHandle.getNativeHandle())) { hidl_cb(Error::BAD_BUFFER, nullptr); return Void(); } @@ -137,7 +155,7 @@ Return GrallocMapper::importBuffer(const hidl_handle& rawHandle, // The newly cloned handle is already registered? This can only happen // when a handle previously registered was native_handle_delete'd instead // of freeBuffer'd. - if (!addRegisteredHandle(bufferHandle)) { + if (!gRegisteredHandles->add(bufferHandle)) { ALOGE("handle %p has already been imported; potential fd leaking", bufferHandle); unregisterBuffer(bufferHandle); @@ -155,7 +173,7 @@ Return GrallocMapper::importBuffer(const hidl_handle& rawHandle, } Return GrallocMapper::freeBuffer(void* buffer) { - native_handle_t* bufferHandle = popRegisteredHandle(buffer); + native_handle_t* bufferHandle = gRegisteredHandles->pop(buffer); if (!bufferHandle) { return Error::BAD_BUFFER; } @@ -209,7 +227,7 @@ Return GrallocMapper::lock(void* buffer, uint64_t cpuUsage, const IMapper::Rect& accessRegion, const hidl_handle& acquireFence, lock_cb hidl_cb) { - buffer_handle_t bufferHandle = getRegisteredHandle(buffer); + buffer_handle_t bufferHandle = gRegisteredHandles->get(buffer); if (!bufferHandle) { hidl_cb(Error::BAD_BUFFER, nullptr); return Void(); @@ -235,7 +253,7 @@ Return GrallocMapper::lockYCbCr(void* buffer, uint64_t cpuUsage, lockYCbCr_cb hidl_cb) { YCbCrLayout layout = {}; - buffer_handle_t bufferHandle = getRegisteredHandle(buffer); + buffer_handle_t bufferHandle = gRegisteredHandles->get(buffer); if (!bufferHandle) { hidl_cb(Error::BAD_BUFFER, layout); return Void(); @@ -255,7 +273,7 @@ Return GrallocMapper::lockYCbCr(void* buffer, uint64_t cpuUsage, } Return GrallocMapper::unlock(void* buffer, unlock_cb hidl_cb) { - buffer_handle_t bufferHandle = getRegisteredHandle(buffer); + buffer_handle_t bufferHandle = gRegisteredHandles->get(buffer); if (!bufferHandle) { hidl_cb(Error::BAD_BUFFER, nullptr); return Void(); diff --git a/graphics/mapper/2.0/default/GrallocMapper.h b/graphics/mapper/2.0/default/GrallocMapper.h index c9c6d8a918..aa1aeaa404 100644 --- a/graphics/mapper/2.0/default/GrallocMapper.h +++ b/graphics/mapper/2.0/default/GrallocMapper.h @@ -80,16 +80,8 @@ class GrallocMapper : public IMapper { virtual Error unlockBuffer(buffer_handle_t bufferHandle, int* outFenceFd) = 0; - static bool addRegisteredHandle(buffer_handle_t bufferHandle); - static buffer_handle_t getRegisteredHandle(const void* buffer); - static native_handle_t* popRegisteredHandle(void* buffer); - static bool getFenceFd(const hidl_handle& fenceHandle, int* outFenceFd); static hidl_handle getFenceHandle(int fenceFd, char* handleStorage); - - // these are static and shared by all mappers - static std::mutex mMutex; - static std::unordered_set mRegisteredHandles; }; extern "C" IMapper* HIDL_FETCH_IMapper(const char* name);