Implement importBuffer for BufferHubService
Allow user importBuffer by a previously generated handle from
IBufferClient::duplicate.
Passing a nullptr or invalid token to the service will get a nullptr
IBufferClient and BufferHubStatus::INVALID_TOKEN.
Test: BufferHubBuffer_test (passed)
Change-Id: I0aa969a9706e42039ac851acb991b3aceb924c27
Fix: 118614157
diff --git a/libs/ui/tests/Android.bp b/libs/ui/tests/Android.bp
index b7ad4e5..00f30a6 100644
--- a/libs/ui/tests/Android.bp
+++ b/libs/ui/tests/Android.bp
@@ -53,6 +53,7 @@
],
shared_libs: [
"android.frameworks.bufferhub@1.0",
+ "libcutils",
"libhidlbase",
"libhwbinder",
"libpdx_default_transport",
diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp
index d30636f..553ee0b 100644
--- a/libs/ui/tests/BufferHubBuffer_test.cpp
+++ b/libs/ui/tests/BufferHubBuffer_test.cpp
@@ -19,6 +19,7 @@
#include <android/frameworks/bufferhub/1.0/IBufferClient.h>
#include <android/frameworks/bufferhub/1.0/IBufferHub.h>
#include <android/hardware_buffer.h>
+#include <cutils/native_handle.h>
#include <gtest/gtest.h>
#include <hidl/ServiceManagement.h>
#include <hwbinder/IPCThreadState.h>
@@ -144,7 +145,7 @@
EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk());
}
-TEST_F(BufferHubBufferTest, DuplicateBuffer) {
+TEST_F(BufferHubBufferTest, DuplicateAndImportBuffer) {
// TODO(b/116681016): directly test on BufferHubBuffer instead of the service.
sp<IBufferHub> bufferhub = IBufferHub::getService();
ASSERT_NE(nullptr, bufferhub.get());
@@ -170,11 +171,63 @@
token = outToken;
ret = status;
};
- EXPECT_TRUE(client->duplicate(dup_cb).isOk());
+ ASSERT_TRUE(client->duplicate(dup_cb).isOk());
EXPECT_EQ(ret, BufferHubStatus::NO_ERROR);
ASSERT_NE(token.getNativeHandle(), nullptr);
EXPECT_EQ(token->numInts, 1);
EXPECT_EQ(token->numFds, 0);
+
+ sp<IBufferClient> client2;
+ IBufferHub::importBuffer_cb import_cb = [&](const auto& outClient, const auto& status) {
+ ret = status;
+ client2 = outClient;
+ };
+ ASSERT_TRUE(bufferhub->importBuffer(token, import_cb).isOk());
+ EXPECT_EQ(ret, BufferHubStatus::NO_ERROR);
+ EXPECT_NE(nullptr, client2.get());
+ // TODO(b/116681016): once BufferNode.id() is exposed via BufferHubBuffer, check origin.id =
+ // improted.id here.
+}
+
+// nullptr must not crash the service
+TEST_F(BufferHubBufferTest, ImportNullToken) {
+ // TODO(b/116681016): directly test on BufferHubBuffer instead of the service.
+ sp<IBufferHub> bufferhub = IBufferHub::getService();
+ ASSERT_NE(nullptr, bufferhub.get());
+
+ hidl_handle nullToken;
+ sp<IBufferClient> client;
+ BufferHubStatus ret;
+ IBufferHub::importBuffer_cb import_cb = [&](const auto& outClient, const auto& status) {
+ client = outClient;
+ ret = status;
+ };
+ ASSERT_TRUE(bufferhub->importBuffer(nullToken, import_cb).isOk());
+ EXPECT_EQ(ret, BufferHubStatus::INVALID_TOKEN);
+ EXPECT_EQ(nullptr, client.get());
+}
+
+// This test has a very little chance to fail (number of existing tokens / 2 ^ 32)
+TEST_F(BufferHubBufferTest, ImportInvalidToken) {
+ // TODO(b/116681016): directly test on BufferHubBuffer instead of the service.
+ sp<IBufferHub> bufferhub = IBufferHub::getService();
+ ASSERT_NE(nullptr, bufferhub.get());
+
+ native_handle_t* tokenHandle = native_handle_create(/*numFds=*/0, /*numInts=*/1);
+ tokenHandle->data[0] = 0;
+
+ hidl_handle invalidToken(tokenHandle);
+ sp<IBufferClient> client;
+ BufferHubStatus ret;
+ IBufferHub::importBuffer_cb import_cb = [&](const auto& outClient, const auto& status) {
+ client = outClient;
+ ret = status;
+ };
+ ASSERT_TRUE(bufferhub->importBuffer(invalidToken, import_cb).isOk());
+ EXPECT_EQ(ret, BufferHubStatus::INVALID_TOKEN);
+ EXPECT_EQ(nullptr, client.get());
+
+ native_handle_delete(tokenHandle);
}
} // namespace
diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp
index 1a38dd8..6f97f0d 100644
--- a/services/bufferhub/BufferHubService.cpp
+++ b/services/bufferhub/BufferHubService.cpp
@@ -51,10 +51,44 @@
return Void();
}
-Return<void> BufferHubService::importBuffer(const hidl_handle& /*nativeHandle*/,
+Return<void> BufferHubService::importBuffer(const hidl_handle& tokenHandle,
importBuffer_cb _hidl_cb) {
- // TODO(b/118614157): implement buffer import
- _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::NO_ERROR);
+ if (!tokenHandle.getNativeHandle() || tokenHandle->numFds != 0 || tokenHandle->numInts != 1) {
+ // nullptr handle or wrong format
+ _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::INVALID_TOKEN);
+ return Void();
+ }
+
+ uint32_t token = tokenHandle->data[0];
+
+ wp<BufferClient> originClientWp;
+ {
+ std::lock_guard<std::mutex> lock(mTokenMapMutex);
+ auto iter = mTokenMap.find(token);
+ if (iter == mTokenMap.end()) {
+ // Invalid token
+ _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::INVALID_TOKEN);
+ return Void();
+ }
+
+ originClientWp = iter->second;
+ mTokenMap.erase(iter);
+ }
+
+ // Check if original client is dead
+ sp<BufferClient> originClient = originClientWp.promote();
+ if (!originClient) {
+ // Should not happen since token should be removed if already gone
+ ALOGE("%s: original client %p gone!", __FUNCTION__, originClientWp.unsafe_get());
+ _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::BUFFER_FREED);
+ return Void();
+ }
+
+ sp<BufferClient> client = new BufferClient(*originClient);
+
+ std::lock_guard<std::mutex> lock(mClientListMutex);
+ mClientList.push_back(client);
+ _hidl_cb(/*bufferClient=*/client, /*status=*/BufferHubStatus::NO_ERROR);
return Void();
}
diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h
index 5456ec3..769ec86 100644
--- a/services/bufferhub/include/bufferhub/BufferClient.h
+++ b/services/bufferhub/include/bufferhub/BufferClient.h
@@ -37,15 +37,19 @@
class BufferClient : public IBufferClient {
public:
// Creates a server-side buffer client from an existing BufferNode. Note that
- // this funciton takes ownership of the shared_ptr.
+ // this function takes ownership of the shared_ptr.
// Returns a raw pointer to the BufferClient on success, nullptr on failure.
static BufferClient* create(BufferHubService* service, const std::shared_ptr<BufferNode>& node);
+ // Creates a BufferClient from an existing BufferClient. Will share the same BufferNode.
+ explicit BufferClient(const BufferClient& other)
+ : mService(other.mService), mBufferNode(other.mBufferNode) {}
+
Return<void> duplicate(duplicate_cb _hidl_cb) override;
private:
BufferClient(wp<BufferHubService> service, const std::shared_ptr<BufferNode>& node)
- : mService(service), mBufferNode(node){};
+ : mService(service), mBufferNode(node) {}
wp<BufferHubService> mService;
std::shared_ptr<BufferNode> mBufferNode;
diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h
index e3f657f..6535659 100644
--- a/services/bufferhub/include/bufferhub/BufferHubService.h
+++ b/services/bufferhub/include/bufferhub/BufferHubService.h
@@ -42,7 +42,7 @@
Return<void> allocateBuffer(const HardwareBufferDescription& description,
const uint32_t userMetadataSize,
allocateBuffer_cb _hidl_cb) override;
- Return<void> importBuffer(const hidl_handle& nativeHandle, importBuffer_cb _hidl_cb) override;
+ Return<void> importBuffer(const hidl_handle& tokenHandle, importBuffer_cb _hidl_cb) override;
// Non-binder functions
// Internal help function for IBufferClient::duplicate.