From 47897d5ff923f830bfc322a87c2f48670d35571c Mon Sep 17 00:00:00 2001 From: Sungtak Lee Date: Thu, 25 Jul 2019 14:22:36 -0700 Subject: [PATCH] bufferpool2: fix native handle race condition Return cloned handle as output parameter in order to use it after closing the bufferpool connection. Bug: 138171841 Test: atest CtsMediaTestCases -- --module-arg CtsMediaTestCases:size:small Merged-In: I4ac142fcb7e3ecdb3fb02792f516b343d71bfc38 Change-Id: I4ac142fcb7e3ecdb3fb02792f516b343d71bfc38 (cherry picked from commit dc5cb62ff4528ac52a15fb525874ab47e377e8ac) --- media/bufferpool/2.0/ClientManager.cpp | 25 +++++- .../2.0/include/bufferpool/ClientManager.h | 8 +- media/codec2/hidl/1.0/utils/types.cpp | 5 ++ media/codec2/vndk/C2Buffer.cpp | 80 ++++++++----------- 4 files changed, 68 insertions(+), 50 deletions(-) diff --git a/media/bufferpool/2.0/ClientManager.cpp b/media/bufferpool/2.0/ClientManager.cpp index c31d313ab1..48c2da4be4 100644 --- a/media/bufferpool/2.0/ClientManager.cpp +++ b/media/bufferpool/2.0/ClientManager.cpp @@ -351,7 +351,17 @@ ResultStatus ClientManager::Impl::allocate( } client = it->second; } - return client->allocate(params, handle, buffer); + native_handle_t *origHandle; + ResultStatus res = client->allocate(params, &origHandle, buffer); + if (res != ResultStatus::OK) { + return res; + } + *handle = native_handle_clone(origHandle); + if (handle == NULL) { + buffer->reset(); + return ResultStatus::NO_MEMORY; + } + return ResultStatus::OK; } ResultStatus ClientManager::Impl::receive( @@ -367,7 +377,18 @@ ResultStatus ClientManager::Impl::receive( } client = it->second; } - return client->receive(transactionId, bufferId, timestampUs, handle, buffer); + native_handle_t *origHandle; + ResultStatus res = client->receive( + transactionId, bufferId, timestampUs, &origHandle, buffer); + if (res != ResultStatus::OK) { + return res; + } + *handle = native_handle_clone(origHandle); + if (handle == NULL) { + buffer->reset(); + return ResultStatus::NO_MEMORY; + } + return ResultStatus::OK; } ResultStatus ClientManager::Impl::postSend( diff --git a/media/bufferpool/2.0/include/bufferpool/ClientManager.h b/media/bufferpool/2.0/include/bufferpool/ClientManager.h index 953c3049a4..24b61f4fb5 100644 --- a/media/bufferpool/2.0/include/bufferpool/ClientManager.h +++ b/media/bufferpool/2.0/include/bufferpool/ClientManager.h @@ -104,7 +104,9 @@ struct ClientManager : public IClientManager { ResultStatus flush(ConnectionId connectionId); /** - * Allocates a buffer from the specified connection. + * Allocates a buffer from the specified connection. The output parameter + * handle is cloned from the internal handle. So it is safe to use directly, + * and it should be deleted and destroyed after use. * * @param connectionId The id of the connection. * @param params The allocation parameters. @@ -123,7 +125,9 @@ struct ClientManager : public IClientManager { std::shared_ptr *buffer); /** - * Receives a buffer for the transaction. + * Receives a buffer for the transaction. The output parameter handle is + * cloned from the internal handle. So it is safe to use directly, and it + * should be deleted and destoyed after use. * * @param connectionId The id of the receiving connection. * @param transactionId The id for the transaction. diff --git a/media/codec2/hidl/1.0/utils/types.cpp b/media/codec2/hidl/1.0/utils/types.cpp index 07dbf6777f..04fa59ccd0 100644 --- a/media/codec2/hidl/1.0/utils/types.cpp +++ b/media/codec2/hidl/1.0/utils/types.cpp @@ -1434,6 +1434,11 @@ bool objcpy(C2BaseBlock* d, const BaseBlock& s) { d->type = C2BaseBlock::GRAPHIC; return true; } + if (cHandle) { + // Though we got cloned handle, creating block failed. + native_handle_close(cHandle); + native_handle_delete(cHandle); + } LOG(ERROR) << "Unknown handle type in BaseBlock::pooledBlock."; return false; diff --git a/media/codec2/vndk/C2Buffer.cpp b/media/codec2/vndk/C2Buffer.cpp index 710b536ef7..2d99b53936 100644 --- a/media/codec2/vndk/C2Buffer.cpp +++ b/media/codec2/vndk/C2Buffer.cpp @@ -413,17 +413,14 @@ std::shared_ptr _C2BlockFactory::CreateLinearBlock( std::shared_ptr alloc; if (C2AllocatorIon::isValid(cHandle)) { - native_handle_t *handle = native_handle_clone(cHandle); - if (handle) { - c2_status_t err = sAllocator->priorLinearAllocation(handle, &alloc); - const std::shared_ptr poolData = - std::make_shared(data); - if (err == C2_OK && poolData) { - // TODO: config params? - std::shared_ptr block = - _C2BlockFactory::CreateLinearBlock(alloc, poolData); - return block; - } + c2_status_t err = sAllocator->priorLinearAllocation(cHandle, &alloc); + const std::shared_ptr poolData = + std::make_shared(data); + if (err == C2_OK && poolData) { + // TODO: config params? + std::shared_ptr block = + _C2BlockFactory::CreateLinearBlock(alloc, poolData); + return block; } } return nullptr; @@ -674,17 +671,14 @@ public: ResultStatus status = mBufferPoolManager->allocate( mConnectionId, params, &cHandle, &bufferPoolData); if (status == ResultStatus::OK) { - native_handle_t *handle = native_handle_clone(cHandle); - if (handle) { - std::shared_ptr alloc; - std::shared_ptr poolData = - std::make_shared(bufferPoolData); - c2_status_t err = mAllocator->priorLinearAllocation(handle, &alloc); - if (err == C2_OK && poolData && alloc) { - *block = _C2BlockFactory::CreateLinearBlock(alloc, poolData, 0, capacity); - if (*block) { - return C2_OK; - } + std::shared_ptr alloc; + std::shared_ptr poolData = + std::make_shared(bufferPoolData); + c2_status_t err = mAllocator->priorLinearAllocation(cHandle, &alloc); + if (err == C2_OK && poolData && alloc) { + *block = _C2BlockFactory::CreateLinearBlock(alloc, poolData, 0, capacity); + if (*block) { + return C2_OK; } } return C2_NO_MEMORY; @@ -710,19 +704,16 @@ public: ResultStatus status = mBufferPoolManager->allocate( mConnectionId, params, &cHandle, &bufferPoolData); if (status == ResultStatus::OK) { - native_handle_t *handle = native_handle_clone(cHandle); - if (handle) { - std::shared_ptr alloc; - std::shared_ptr poolData = - std::make_shared(bufferPoolData); - c2_status_t err = mAllocator->priorGraphicAllocation( - handle, &alloc); - if (err == C2_OK && poolData && alloc) { - *block = _C2BlockFactory::CreateGraphicBlock( - alloc, poolData, C2Rect(width, height)); - if (*block) { - return C2_OK; - } + std::shared_ptr alloc; + std::shared_ptr poolData = + std::make_shared(bufferPoolData); + c2_status_t err = mAllocator->priorGraphicAllocation( + cHandle, &alloc); + if (err == C2_OK && poolData && alloc) { + *block = _C2BlockFactory::CreateGraphicBlock( + alloc, poolData, C2Rect(width, height)); + if (*block) { + return C2_OK; } } return C2_NO_MEMORY; @@ -1117,17 +1108,14 @@ std::shared_ptr _C2BlockFactory::CreateGraphicBlock( std::shared_ptr alloc; if (C2AllocatorGralloc::isValid(cHandle)) { - native_handle_t *handle = native_handle_clone(cHandle); - if (handle) { - c2_status_t err = sAllocator->priorGraphicAllocation(handle, &alloc); - const std::shared_ptr poolData = - std::make_shared(data); - if (err == C2_OK && poolData) { - // TODO: config setup? - std::shared_ptr block = - _C2BlockFactory::CreateGraphicBlock(alloc, poolData); - return block; - } + c2_status_t err = sAllocator->priorGraphicAllocation(cHandle, &alloc); + const std::shared_ptr poolData = + std::make_shared(data); + if (err == C2_OK && poolData) { + // TODO: config setup? + std::shared_ptr block = + _C2BlockFactory::CreateGraphicBlock(alloc, poolData); + return block; } } return nullptr;