[llcpp] Don't change source value in std::move of VectorView and tracking_ptr
tracking_ptr is intended to match the behavior of raw pointers for
unowned pointers where possible (and the behavior of unique_ptr for
owned pointers).
This wasn't the case for std::move. Example:
int* x = &something;
int* y = std::move(x);
After these statements, x == y rather than x == nullptr.
The DecodedMessage class (used in ResultOf) has a destructor that
walks the object to close handles. The previous behavior of zeroing
pointers after std::move prevented this walk from traversing the
object and led to handle leaks. This change restores the previous
behavior and allows the object to be walked.
Fixed: 49226
Test: fx test llcpp_fidl_types_test
Test: fx test llcpp_fidl_builder_test
Tests originate from fxr/377022
Change-Id: I35b89f575bfef021c1e4ba48fe549c9fd446439f
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/377654
Commit-Queue: Benjamin Prosnitz <bprosnitz@google.com>
Reviewed-by: Chris Tam <godtamit@google.com>
Reviewed-by: Yifei Teng <yifeit@google.com>
Testability-Review: Yifei Teng <yifeit@google.com>
diff --git a/src/lib/fidl/llcpp/tests/protocol.test.fidl b/src/lib/fidl/llcpp/tests/protocol.test.fidl
index 12e97dd..ac4b625 100644
--- a/src/lib/fidl/llcpp/tests/protocol.test.fidl
+++ b/src/lib/fidl/llcpp/tests/protocol.test.fidl
@@ -9,6 +9,22 @@
REALLY_BAD_ERROR = 2;
};
+struct HandleStruct {
+ handle<event> h;
+};
+
+struct VectorStruct {
+ vector<HandleStruct> v;
+};
+
+union HandleUnion {
+ 1: handle<event> h;
+};
+
+struct HandleUnionStruct {
+ HandleUnion u;
+};
+
/// Protocol for testing methods with error types.
/// In the implementation, each method is hardcoded to return either the
/// success or the error case. This should follow the naming of the method,
@@ -23,3 +39,9 @@
Grob(string value) -> (string value);
-> Hrob(string value);
};
+
+protocol HandleProvider {
+ GetHandle() -> (HandleStruct value);
+ GetHandleVector(uint32 count) -> (vector<HandleStruct> value);
+ GetHandleUnion() -> (HandleUnionStruct value);
+};
diff --git a/src/lib/fidl/llcpp/tests/protocol_test.cc b/src/lib/fidl/llcpp/tests/protocol_test.cc
index 7fabc2a..fb01c74 100644
--- a/src/lib/fidl/llcpp/tests/protocol_test.cc
+++ b/src/lib/fidl/llcpp/tests/protocol_test.cc
@@ -6,9 +6,15 @@
#include <lib/async-loop/default.h>
#include <lib/async/wait.h>
#include <lib/fidl-async/cpp/bind.h>
+#include <lib/fidl/llcpp/memory.h>
+#include <lib/fidl/llcpp/vector_view.h>
+#include <lib/zx/object.h>
#include <zircon/errors.h>
#include <zircon/fidl.h>
#include <zircon/status.h>
+#include <zircon/syscalls/object.h>
+
+#include <cstdint>
#include <llcpptest/protocol/test/llcpp/fidl.h>
@@ -18,6 +24,15 @@
namespace {
zx_status_t kErrorStatus = 271;
+
+template <typename T>
+uint32_t GetHandleCount(zx::unowned<T> h) {
+ zx_info_handle_count_t info = {};
+ auto status = h->get_info(ZX_INFO_HANDLE_COUNT, &info, sizeof(info), nullptr, nullptr);
+ ZX_ASSERT(status == ZX_OK);
+ return info.handle_count;
+}
+
} // namespace
class ErrorServer : public test::ErrorMethods::Interface {
@@ -220,3 +235,142 @@
auto resp = client.NoArgsPrimitiveError(false);
ASSERT_EQ(ZX_ERR_BAD_HANDLE, resp.status());
}
+
+class HandleProviderServer : public test::HandleProvider::Interface {
+ public:
+ void GetHandle(GetHandleCompleter::Sync completer) override {
+ test::HandleStruct s;
+ zx::event::create(0, &s.h);
+ completer.Reply(std::move(s));
+ }
+
+ void GetHandleVector(uint32_t count, GetHandleVectorCompleter::Sync completer) override {
+ std::vector<test::HandleStruct> v(count);
+ for (auto& s : v) {
+ zx::event::create(0, &s.h);
+ }
+ completer.Reply(fidl::unowned_vec(v));
+ }
+
+ void GetHandleUnion(GetHandleUnionCompleter::Sync completer) override {
+ zx::event h;
+ zx::event::create(0, &h);
+ test::HandleUnionStruct s = {.u = test::HandleUnion::WithH(fidl::unowned_ptr(&h))};
+ completer.Reply(std::move(s));
+ }
+};
+
+class HandleTest : public ::testing::Test {
+ protected:
+ virtual void SetUp() {
+ loop_ = std::make_unique<async::Loop>(&kAsyncLoopConfigAttachToCurrentThread);
+ ASSERT_EQ(loop_->StartThread("test_llcpp_handle_server"), ZX_OK);
+
+ zx::channel server_end;
+ ASSERT_EQ(zx::channel::create(0, &client_end_, &server_end), ZX_OK);
+ server_ = std::make_unique<HandleProviderServer>();
+ fidl::Bind(loop_->dispatcher(), std::move(server_end), server_.get());
+ }
+
+ test::HandleProvider::SyncClient TakeClient() {
+ EXPECT_TRUE(client_end_.is_valid());
+ return test::HandleProvider::SyncClient(std::move(client_end_));
+ }
+
+ private:
+ std::unique_ptr<async::Loop> loop_;
+ std::unique_ptr<HandleProviderServer> server_;
+ zx::channel client_end_;
+};
+
+TEST_F(HandleTest, HandleClosedAfterResultOfMove) {
+ auto client = TakeClient();
+ auto result = client.GetHandle();
+
+ ASSERT_TRUE(result.ok()) << result.error();
+ ASSERT_TRUE(result->value.h.is_valid());
+
+ // Dupe the event so we can get the handle count after move.
+ zx::event dupe;
+ ASSERT_EQ(result->value.h.duplicate(ZX_RIGHT_SAME_RIGHTS, &dupe), ZX_OK);
+
+ // Moving the ResultOf::GetHandle should move the handle.
+ { auto release = std::move(result); } // ~ResultOf::GetHandle
+
+ // Only remaining handle should be the dupe.
+ ASSERT_EQ(GetHandleCount(dupe.borrow()), 1u);
+}
+
+TEST_F(HandleTest, HandleClosedAfterHandleStructMove) {
+ auto client = TakeClient();
+ auto result = client.GetHandle();
+
+ ASSERT_TRUE(result.ok()) << result.error();
+ ASSERT_TRUE(result->value.h.is_valid());
+
+ // Dupe the event so we can get the handle count after move.
+ zx::event dupe;
+ ASSERT_EQ(result->value.h.duplicate(ZX_RIGHT_SAME_RIGHTS, &dupe), ZX_OK);
+
+ // A move of a struct holding a handle will move the handle from the result, resulting in a close
+ { auto release = std::move(result->value); } // ~HandleStruct
+
+ // Only remaining handle should be the dupe.
+ ASSERT_EQ(GetHandleCount(dupe.borrow()), 1u);
+}
+
+TEST_F(HandleTest, HandleClosedOnResultOfDestructorAfterVectorMove) {
+ constexpr uint32_t kNumHandles = 2;
+
+ auto client = TakeClient();
+ std::vector<zx::event> dupes(kNumHandles);
+
+ {
+ auto result = client.GetHandleVector(kNumHandles);
+
+ ASSERT_TRUE(result.ok()) << result.error();
+ ASSERT_EQ(result->value.count(), kNumHandles);
+
+ for (uint32_t i = 0; i < kNumHandles; i++) {
+ ASSERT_TRUE(result->value[i].h.is_valid());
+ ASSERT_EQ(result->value[i].h.duplicate(ZX_RIGHT_SAME_RIGHTS, &dupes[i]), ZX_OK);
+ }
+
+ { auto release = std::move(result->value); } // ~VectorView<HandleStruct>
+
+ // std::move of VectorView only moves pointers, not handles.
+ // 1 handle in ResultOf + 1 handle in dupe = 2.
+ for (auto& e : dupes) {
+ ASSERT_EQ(GetHandleCount(e.borrow()), 2u);
+ }
+ }
+
+ // Handle cleaned up after ResultOf destructor is called.
+ // Remaining handle is the dupe.
+ for (auto& e : dupes) {
+ ASSERT_EQ(GetHandleCount(e.borrow()), 1u);
+ }
+}
+
+TEST_F(HandleTest, HandleClosedOnResultOfDestructorAfterTrackingPtrMove) {
+ auto client = TakeClient();
+ zx::event dupe;
+
+ {
+ auto result = client.GetHandleUnion();
+
+ ASSERT_TRUE(result.ok()) << result.error();
+ ASSERT_TRUE(result->value.u.h().is_valid());
+ ASSERT_EQ(result->value.u.h().duplicate(ZX_RIGHT_SAME_RIGHTS, &dupe), ZX_OK);
+
+ { auto release = std::move(result->value); } // ~HandleUnion
+
+ // std::move of tracking_ptr in union only moves pointers, not handles.
+ // 1 handle in ResultOf + 1 handle in dupe = 2.
+ ASSERT_EQ(GetHandleCount(dupe.borrow()), 2u);
+ }
+
+ // Handle cleaned up after ResultOf destructor is called.
+ // Remaining handle is the dupe.
+ ASSERT_EQ(GetHandleCount(dupe.borrow()), 1u);
+}
diff --git a/src/lib/fidl/llcpp/tests/tracking_ptr_test.cc b/src/lib/fidl/llcpp/tests/tracking_ptr_test.cc
index e6f2aa7..715e61a 100644
--- a/src/lib/fidl/llcpp/tests/tracking_ptr_test.cc
+++ b/src/lib/fidl/llcpp/tests/tracking_ptr_test.cc
@@ -42,6 +42,8 @@
fidl::tracking_ptr<DestructableObject> ptr1 = fidl::unowned_ptr_t<DestructableObject>(&obj1);
fidl::tracking_ptr<DestructableObject> ptr2 = fidl::unowned_ptr_t<DestructableObject>(&obj2);
ptr2 = std::move(ptr1);
+ EXPECT_EQ(ptr1.get(), &obj1);
+ EXPECT_EQ(ptr2.get(), &obj1);
}
EXPECT_FALSE(ds1.destructor_called);
EXPECT_FALSE(ds2.destructor_called);
@@ -49,14 +51,19 @@
TEST(TrackingPtr, OwnedSingleValueLifecycle) {
DestructionState ds1, ds2;
+ auto obj1 = std::make_unique<DestructableObject>(&ds1);
+ auto obj2 = std::make_unique<DestructableObject>(&ds2);
+ DestructableObject* obj1_raw = obj1.get();
{
- fidl::tracking_ptr<DestructableObject> ptr1 = std::make_unique<DestructableObject>(&ds1);
- fidl::tracking_ptr<DestructableObject> ptr2 = std::make_unique<DestructableObject>(&ds2);
+ fidl::tracking_ptr<DestructableObject> ptr1 = std::move(obj1);
+ fidl::tracking_ptr<DestructableObject> ptr2 = std::move(obj2);
EXPECT_FALSE(ds1.destructor_called);
EXPECT_FALSE(ds2.destructor_called);
ptr2 = std::move(ptr1);
EXPECT_FALSE(ds1.destructor_called);
EXPECT_TRUE(ds2.destructor_called);
+ EXPECT_EQ(ptr1.get(), nullptr);
+ EXPECT_EQ(ptr2.get(), obj1_raw);
}
EXPECT_TRUE(ds1.destructor_called);
}
@@ -70,6 +77,8 @@
fidl::tracking_ptr<DestructableObject[]> ptr1 = fidl::unowned_ptr_t<DestructableObject>(arr1);
fidl::tracking_ptr<DestructableObject[]> ptr2 = fidl::unowned_ptr_t<DestructableObject>(arr2);
ptr2 = std::move(ptr1);
+ EXPECT_EQ(ptr1.get(), arr1);
+ EXPECT_EQ(ptr2.get(), arr1);
}
EXPECT_FALSE(ds1[0].destructor_called);
EXPECT_FALSE(ds1[1].destructor_called);
@@ -80,15 +89,15 @@
TEST(TrackingPtr, OwnedArrayLifecycle) {
DestructionState ds1[2] = {};
DestructionState ds2[2] = {};
-
+ auto arr1 = std::make_unique<DestructableObject[]>(2);
+ arr1[0].ds = &ds1[0];
+ arr1[1].ds = &ds1[1];
+ auto arr2 = std::make_unique<DestructableObject[]>(2);
+ arr2[0].ds = &ds2[0];
+ arr2[1].ds = &ds2[1];
+ DestructableObject* arr1_raw = arr1.get();
{
- auto arr1 = std::make_unique<DestructableObject[]>(2);
- arr1[0].ds = &ds1[0];
- arr1[1].ds = &ds1[1];
fidl::tracking_ptr<DestructableObject[]> ptr1(std::move(arr1));
- auto arr2 = std::make_unique<DestructableObject[]>(2);
- arr2[0].ds = &ds2[0];
- arr2[1].ds = &ds2[1];
fidl::tracking_ptr<DestructableObject[]> ptr2(std::move(arr2));
EXPECT_FALSE(ds1[0].destructor_called);
EXPECT_FALSE(ds1[1].destructor_called);
@@ -99,6 +108,8 @@
EXPECT_FALSE(ds1[1].destructor_called);
EXPECT_TRUE(ds2[0].destructor_called);
EXPECT_TRUE(ds2[1].destructor_called);
+ EXPECT_EQ(ptr1.get(), nullptr);
+ EXPECT_EQ(ptr2.get(), arr1_raw);
}
EXPECT_TRUE(ds1[0].destructor_called);
EXPECT_TRUE(ds1[1].destructor_called);
diff --git a/src/lib/fidl/llcpp/tests/vector_view_test.cc b/src/lib/fidl/llcpp/tests/vector_view_test.cc
index 4c9c787..97f599b 100644
--- a/src/lib/fidl/llcpp/tests/vector_view_test.cc
+++ b/src/lib/fidl/llcpp/tests/vector_view_test.cc
@@ -39,25 +39,56 @@
EXPECT_FALSE(ds[1].destructor_called);
}
-TEST(VectorView, MoveConstructor) {
+TEST(VectorView, MoveConstructorUnowned) {
std::vector<int32_t> vec{1, 2, 3};
fidl::VectorView<int32_t> vv(fidl::unowned_ptr_t<int32_t>(vec.data()), vec.size());
fidl::VectorView<int32_t> moved_vv(std::move(vv));
- EXPECT_EQ(vv.count(), 0ULL);
- EXPECT_EQ(vv.data(), nullptr);
+ EXPECT_EQ(vv.count(), 3ULL);
+ EXPECT_EQ(vv.data(), vec.data());
EXPECT_EQ(moved_vv.count(), 3ULL);
EXPECT_EQ(moved_vv.data(), vec.data());
}
-TEST(VectorView, MoveAssigment) {
+TEST(VectorView, MoveConstructorOwned) {
+ constexpr size_t size = 3;
+ auto arr = std::make_unique<int32_t[]>(3);
+ arr[0] = 1;
+ arr[1] = 2;
+ arr[2] = 3;
+ int32_t* arr_raw = arr.get();
+ fidl::VectorView<int32_t> vv(std::move(arr), size);
+ fidl::VectorView<int32_t> moved_vv(std::move(vv));
+ EXPECT_EQ(vv.count(), 0ULL);
+ EXPECT_EQ(vv.data(), nullptr);
+ EXPECT_EQ(moved_vv.count(), 3ULL);
+ EXPECT_EQ(moved_vv.data(), arr_raw);
+}
+
+TEST(VectorView, MoveAssigmentUnowned) {
std::vector<int32_t> vec{1, 2, 3};
fidl::VectorView<int32_t> vv(fidl::unowned_ptr_t<int32_t>(vec.data()), vec.size());
fidl::VectorView<int32_t> moved_vv;
moved_vv = std::move(vv);
+ EXPECT_EQ(vv.count(), 3ULL);
+ EXPECT_EQ(vv.data(), vec.data());
+ EXPECT_EQ(moved_vv.count(), 3ULL);
+ EXPECT_EQ(moved_vv.data(), vec.data());
+}
+
+TEST(VectorView, MoveAssigmentOwned) {
+ constexpr size_t size = 3;
+ auto arr = std::make_unique<int32_t[]>(3);
+ arr[0] = 1;
+ arr[1] = 2;
+ arr[2] = 3;
+ int32_t* arr_raw = arr.get();
+ fidl::VectorView<int32_t> vv(std::move(arr), size);
+ fidl::VectorView<int32_t> moved_vv;
+ moved_vv = std::move(vv);
EXPECT_EQ(vv.count(), 0ULL);
EXPECT_EQ(vv.data(), nullptr);
EXPECT_EQ(moved_vv.count(), 3ULL);
- EXPECT_EQ(moved_vv.data(), vec.data());
+ EXPECT_EQ(moved_vv.data(), arr_raw);
}
TEST(VectorView, Iteration) {
diff --git a/zircon/system/ulib/fidl/include/lib/fidl/llcpp/tracking_ptr.h b/zircon/system/ulib/fidl/include/lib/fidl/llcpp/tracking_ptr.h
index 0c983a3..4e77b758 100644
--- a/zircon/system/ulib/fidl/include/lib/fidl/llcpp/tracking_ptr.h
+++ b/zircon/system/ulib/fidl/include/lib/fidl/llcpp/tracking_ptr.h
@@ -179,7 +179,10 @@
marked_ptr release_marked_ptr() noexcept {
marked_ptr temp = mptr_;
- mptr_ = kNullMarkedPtr;
+ if (is_owned()) {
+ // Unowned pointers keep their value like raw pointers, but owned pointers should zero.
+ mptr_ = kNullMarkedPtr;
+ }
return temp;
}
@@ -209,14 +212,12 @@
tracking_ptr(tracking_ptr&& other) noexcept {
reset(other.is_owned_, other.ptr_);
- other.is_owned_ = false;
- other.ptr_ = nullptr;
+ other.release();
}
template <typename U = T, typename = std::enable_if_t<std::is_const<U>::value>>
tracking_ptr(tracking_ptr<std::remove_const_t<U>[]>&& other) noexcept {
reset(other.is_owned_, other.ptr_);
- other.is_owned_ = false;
- other.ptr_ = nullptr;
+ other.release();
}
template <typename U = T, typename = std::enable_if_t<!std::is_void<U>::value>>
tracking_ptr(std::unique_ptr<U>&& other) {
@@ -233,8 +234,7 @@
tracking_ptr& operator=(tracking_ptr&& other) noexcept {
reset(other.is_owned_, other.ptr_);
- other.is_owned_ = false;
- other.ptr_ = nullptr;
+ other.release();
return *this;
}
tracking_ptr& operator=(const tracking_ptr&) = delete;
@@ -248,8 +248,11 @@
// Hand off responsibility of ownership to the caller.
// The internal data can be retrieved through get() and is_owned() before calling release().
void release() {
- ptr_ = nullptr;
- is_owned_ = false;
+ if (is_owned()) {
+ // Unowned pointers keep their value like raw pointers, but owned pointers should zero.
+ ptr_ = nullptr;
+ is_owned_ = false;
+ }
}
private:
diff --git a/zircon/system/ulib/fidl/include/lib/fidl/llcpp/vector_view.h b/zircon/system/ulib/fidl/include/lib/fidl/llcpp/vector_view.h
index abcae87..d578c7c 100644
--- a/zircon/system/ulib/fidl/include/lib/fidl/llcpp/vector_view.h
+++ b/zircon/system/ulib/fidl/include/lib/fidl/llcpp/vector_view.h
@@ -72,8 +72,7 @@
"VectorView<T> can only be move-constructed from VectorView<T> or VectorView<const T>");
count_ = other.count_;
data_ = other.data_;
- other.count_ = 0;
- other.data_ = nullptr;
+ other.release();
}
~VectorView() {
@@ -91,8 +90,7 @@
}
count_ = other.count_;
data_ = other.data_;
- other.count_ = 0;
- other.data_ = nullptr;
+ other.release();
return *this;
}
@@ -146,6 +144,15 @@
bool is_owned() const noexcept { return count_ & kOwnershipMask; }
+ void release() {
+ if (is_owned()) {
+ // Match unique_ptr std::move behavior in owned and unowned case
+ // (unowned case doesn't reset source).
+ count_ = 0;
+ data_ = nullptr;
+ }
+ }
+
friend ::LayoutChecker;
// The lower 63 bits of count_ are reserved to store the number of elements.
