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
Change-Id: I4ac142fcb7e3ecdb3fb02792f516b343d71bfc38
diff --git a/media/bufferpool/2.0/ClientManager.cpp b/media/bufferpool/2.0/ClientManager.cpp
index c31d313..48c2da4 100644
--- a/media/bufferpool/2.0/ClientManager.cpp
+++ b/media/bufferpool/2.0/ClientManager.cpp
@@ -351,7 +351,17 @@
}
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 @@
}
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 953c304..24b61f4 100644
--- a/media/bufferpool/2.0/include/bufferpool/ClientManager.h
+++ b/media/bufferpool/2.0/include/bufferpool/ClientManager.h
@@ -104,7 +104,9 @@
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 @@
std::shared_ptr<BufferPoolData> *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 07dbf67..04fa59c 100644
--- a/media/codec2/hidl/1.0/utils/types.cpp
+++ b/media/codec2/hidl/1.0/utils/types.cpp
@@ -1434,6 +1434,11 @@
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 710b536..2d99b53 100644
--- a/media/codec2/vndk/C2Buffer.cpp
+++ b/media/codec2/vndk/C2Buffer.cpp
@@ -413,17 +413,14 @@
std::shared_ptr<C2LinearAllocation> 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<C2PooledBlockPoolData> poolData =
- std::make_shared<C2PooledBlockPoolData>(data);
- if (err == C2_OK && poolData) {
- // TODO: config params?
- std::shared_ptr<C2LinearBlock> block =
- _C2BlockFactory::CreateLinearBlock(alloc, poolData);
- return block;
- }
+ c2_status_t err = sAllocator->priorLinearAllocation(cHandle, &alloc);
+ const std::shared_ptr<C2PooledBlockPoolData> poolData =
+ std::make_shared<C2PooledBlockPoolData>(data);
+ if (err == C2_OK && poolData) {
+ // TODO: config params?
+ std::shared_ptr<C2LinearBlock> block =
+ _C2BlockFactory::CreateLinearBlock(alloc, poolData);
+ return block;
}
}
return nullptr;
@@ -674,17 +671,14 @@
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<C2LinearAllocation> alloc;
- std::shared_ptr<C2PooledBlockPoolData> poolData =
- std::make_shared<C2PooledBlockPoolData>(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<C2LinearAllocation> alloc;
+ std::shared_ptr<C2PooledBlockPoolData> poolData =
+ std::make_shared<C2PooledBlockPoolData>(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 @@
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<C2GraphicAllocation> alloc;
- std::shared_ptr<C2PooledBlockPoolData> poolData =
- std::make_shared<C2PooledBlockPoolData>(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<C2GraphicAllocation> alloc;
+ std::shared_ptr<C2PooledBlockPoolData> poolData =
+ std::make_shared<C2PooledBlockPoolData>(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<C2GraphicAllocation> 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<C2PooledBlockPoolData> poolData =
- std::make_shared<C2PooledBlockPoolData>(data);
- if (err == C2_OK && poolData) {
- // TODO: config setup?
- std::shared_ptr<C2GraphicBlock> block =
- _C2BlockFactory::CreateGraphicBlock(alloc, poolData);
- return block;
- }
+ c2_status_t err = sAllocator->priorGraphicAllocation(cHandle, &alloc);
+ const std::shared_ptr<C2PooledBlockPoolData> poolData =
+ std::make_shared<C2PooledBlockPoolData>(data);
+ if (err == C2_OK && poolData) {
+ // TODO: config setup?
+ std::shared_ptr<C2GraphicBlock> block =
+ _C2BlockFactory::CreateGraphicBlock(alloc, poolData);
+ return block;
}
}
return nullptr;