From 65a748de0ca16e150ca71fdf32e980707a6bb82d Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Tue, 31 May 2016 13:11:45 -0700 Subject: [PATCH 1/4] Fix move-assigning to a `Results` that has a notifier to not leak the `Realm`, and moving from a `Results` to not result in a use-after-free. The compiler generated move-assignment operator resulted in `m_notifier` being assigned to without first calling `CollectionNotifier::unregister`. This left a retain cycle in place, causing the `Realm` and other objects to leak. `ResultsNotifier` keeps track of which `Results` it should update when a new `TableView` becomes available. When `Results` move-assignment operator and move-constructor transfer ownership of the `ResultsNotifier` to a new instance they also need to update its target so it won't attempt to update the moved-from `Results`. --- src/impl/collection_notifier.hpp | 40 ++++++++++++++++ src/impl/results_notifier.cpp | 10 +++- src/impl/results_notifier.hpp | 2 + src/list.cpp | 7 +-- src/list.hpp | 8 +++- src/results.cpp | 82 +++++++++++++++++++++++++++++++- src/results.hpp | 13 ++--- tests/results.cpp | 51 ++++++++++++++++++++ 8 files changed, 196 insertions(+), 17 deletions(-) diff --git a/src/impl/collection_notifier.hpp b/src/impl/collection_notifier.hpp index 211d5ced..34a133fa 100644 --- a/src/impl/collection_notifier.hpp +++ b/src/impl/collection_notifier.hpp @@ -150,6 +150,9 @@ public: void prepare_handover(); bool deliver(Realm&, SharedGroup&, std::exception_ptr); + template + class Handle; + protected: bool have_callbacks() const noexcept { return m_have_callbacks; } void add_changes(CollectionChangeBuilder change) { m_accumulated_changes.merge(std::move(change)); } @@ -201,6 +204,43 @@ private: CollectionChangeCallback next_callback(); }; +// A smart pointer to a CollectionNotifier that unregisters the notifier when +// the pointer is destroyed. Movable. Copying will produce a null Handle. +template +class CollectionNotifier::Handle : public std::shared_ptr { +public: + using std::shared_ptr::shared_ptr; + + Handle() = default; + ~Handle() { reset(); } + + // Copying a Handle produces a null Handle. + Handle(const Handle&) : Handle() { } + Handle& operator=(const Handle& other) + { + if (this != &other) { + reset(); + } + return *this; + } + + Handle(Handle&&) = default; + Handle& operator=(Handle&& other) + { + reset(); + std::shared_ptr::shared_ptr::operator=(std::move(other)); + return *this; + } + + void reset() + { + if (*this) { + this->get()->unregister(); + std::shared_ptr::reset(); + } + } +}; + } // namespace _impl } // namespace realm diff --git a/src/impl/results_notifier.cpp b/src/impl/results_notifier.cpp index 6e91f3d0..eea290c2 100644 --- a/src/impl/results_notifier.cpp +++ b/src/impl/results_notifier.cpp @@ -18,8 +18,6 @@ #include "impl/results_notifier.hpp" -#include "results.hpp" - using namespace realm; using namespace realm::_impl; @@ -34,6 +32,14 @@ ResultsNotifier::ResultsNotifier(Results& target) m_query_handover = Realm::Internal::get_shared_group(*get_realm()).export_for_handover(q, MutableSourcePayload::Move); } +void ResultsNotifier::target_results_moved(Results& old_target, Results& new_target) +{ + auto lock = lock_target(); + + REALM_ASSERT(m_target_results == &old_target); + m_target_results = &new_target; +} + void ResultsNotifier::release_data() noexcept { m_query = nullptr; diff --git a/src/impl/results_notifier.hpp b/src/impl/results_notifier.hpp index 8028fbd0..23212922 100644 --- a/src/impl/results_notifier.hpp +++ b/src/impl/results_notifier.hpp @@ -30,6 +30,8 @@ class ResultsNotifier : public CollectionNotifier { public: ResultsNotifier(Results& target); + void target_results_moved(Results& old_target, Results& new_target); + private: // Target Results to update // Can only be used with lock_target() held diff --git a/src/list.cpp b/src/list.cpp index fec20772..113f735d 100644 --- a/src/list.cpp +++ b/src/list.cpp @@ -30,12 +30,7 @@ using namespace realm; using namespace realm::_impl; List::List() noexcept = default; -List::~List() -{ - if (m_notifier) { - m_notifier->unregister(); - } -} +List::~List() = default; List::List(std::shared_ptr r, const ObjectSchema& s, LinkViewRef l) noexcept : m_realm(std::move(r)) diff --git a/src/list.hpp b/src/list.hpp index 8abd601d..895e4a09 100644 --- a/src/list.hpp +++ b/src/list.hpp @@ -20,6 +20,7 @@ #define REALM_LIST_HPP #include "collection_notifications.hpp" +#include "impl/collection_notifier.hpp" #include #include @@ -46,6 +47,11 @@ public: List(std::shared_ptr r, const ObjectSchema& s, LinkViewRef l) noexcept; ~List(); + List(const List&) = default; + List& operator=(const List&) = default; + List(List&&) = default; + List& operator=(List&&) = default; + const std::shared_ptr& get_realm() const { return m_realm; } Query get_query() const; const ObjectSchema& get_object_schema() const { return *m_object_schema; } @@ -89,7 +95,7 @@ private: std::shared_ptr m_realm; const ObjectSchema* m_object_schema; LinkViewRef m_link_view; - std::shared_ptr<_impl::CollectionNotifier> m_notifier; + _impl::CollectionNotifier::Handle<_impl::CollectionNotifier> m_notifier; void verify_valid_row(size_t row_ndx, bool insertion = false) const; diff --git a/src/results.cpp b/src/results.cpp index 5e9064f9..c71a4fff 100644 --- a/src/results.cpp +++ b/src/results.cpp @@ -38,6 +38,9 @@ using namespace realm; #define REALM_FALLTHROUGH #endif +Results::Results() = default; +Results::~Results() = default; + Results::Results(SharedRealm r, const ObjectSchema &o, Query q, SortOrder s) : m_realm(std::move(r)) , m_object_schema(&o) @@ -83,13 +86,88 @@ Results::Results(SharedRealm r, const ObjectSchema& o, TableView tv, SortOrder s REALM_ASSERT(m_sort.column_indices.size() == m_sort.ascending.size()); } -Results::~Results() +Results::Results(const Results& other) +: m_realm(other.m_realm) +, m_object_schema(other.m_object_schema) +, m_query(other.m_query) +, m_table_view(other.m_table_view) +, m_link_view(other.m_link_view) +, m_table(other.m_table) +, m_sort(other.m_sort) +, m_live(other.m_live) +, m_notifier() +, m_mode(other.m_mode) +, m_has_used_table_view(false) +, m_wants_background_updates(true) +{ +} + +#if 0 +// FIXME: TableViewBase::operator= is missing from the core static library. +Results& Results::operator=(const Results& other) +{ + if (this == &other) { + return *this; + } + + m_realm = other.m_realm; + m_object_schema = other.m_object_schema; + m_query = other.m_query; + m_table_view = other.m_table_view; + m_link_view = other.m_link_view; + m_table = other.m_table; + m_sort = other.m_sort; + m_live = other.m_live; + m_notifier.reset(); + m_mode = other.m_mode; + m_has_used_table_view = false; + m_wants_background_updates = true; + + return *this; +} +#endif + +Results::Results(Results&& other) +: m_realm(std::move(other.m_realm)) +, m_object_schema(std::move(other.m_object_schema)) +, m_query(std::move(other.m_query)) +, m_table_view(std::move(other.m_table_view)) +, m_link_view(std::move(other.m_link_view)) +, m_table(other.m_table) +, m_sort(std::move(other.m_sort)) +, m_live(other.m_live) +, m_notifier(std::move(other.m_notifier)) +, m_mode(other.m_mode) +, m_has_used_table_view(other.m_has_used_table_view) +, m_wants_background_updates(other.m_wants_background_updates) { if (m_notifier) { - m_notifier->unregister(); + m_notifier->target_results_moved(other, *this); } } +Results& Results::operator=(Results&& other) +{ + m_realm = std::move(other.m_realm); + m_object_schema = std::move(other.m_object_schema); + m_query = std::move(other.m_query); + m_table_view = std::move(other.m_table_view); + m_link_view = std::move(other.m_link_view); + m_table = other.m_table; + m_sort = std::move(other.m_sort); + m_live = other.m_live; + m_notifier = std::move(other.m_notifier); + m_mode = other.m_mode; + m_has_used_table_view = other.m_has_used_table_view; + m_wants_background_updates = other.m_wants_background_updates; + + if (m_notifier) { + m_notifier->target_results_moved(other, *this); + } + + return *this; +} + void Results::validate_read() const { if (m_realm) diff --git a/src/results.hpp b/src/results.hpp index 6b25982a..cfef07a1 100644 --- a/src/results.hpp +++ b/src/results.hpp @@ -21,6 +21,7 @@ #include "collection_notifications.hpp" #include "shared_realm.hpp" +#include "impl/collection_notifier.hpp" #include #include @@ -48,7 +49,7 @@ public: // Results can be either be backed by nothing, a thin wrapper around a table, // or a wrapper around a query and a sort order which creates and updates // the tableview as needed - Results() = default; + Results(); Results(SharedRealm r, const ObjectSchema& o, Table& table); Results(SharedRealm r, const ObjectSchema& o, Query q, SortOrder s = {}); Results(SharedRealm r, const ObjectSchema& o, TableView tv, SortOrder s); @@ -56,10 +57,10 @@ public: ~Results(); // Results is copyable and moveable - Results(Results const&) = default; - Results(Results&&) = default; - Results& operator=(Results&&) = default; - Results& operator=(Results const&) = default; + Results(Results&&); + Results& operator=(Results&&); + Results(const Results&); + Results& operator=(const Results&); // Get the Realm SharedRealm get_realm() const { return m_realm; } @@ -203,7 +204,7 @@ private: SortOrder m_sort; bool m_live = true; - std::shared_ptr<_impl::ResultsNotifier> m_notifier; + _impl::CollectionNotifier::Handle<_impl::ResultsNotifier> m_notifier; Mode m_mode = Mode::Empty; bool m_has_used_table_view = false; diff --git a/tests/results.cpp b/tests/results.cpp index 9113d76e..cd18d206 100644 --- a/tests/results.cpp +++ b/tests/results.cpp @@ -533,3 +533,54 @@ TEST_CASE("Async Results error handling") { } } } + +TEST_CASE("Notifications on moved Results") { + InMemoryTestFile config; + config.cache = false; + config.automatic_change_notifications = false; + config.schema = std::make_unique(Schema{ + {"object", "", { + {"value", PropertyType::Int}, + }}, + }); + + auto r = Realm::get_shared_realm(config); + auto table = r->read_group()->get_table("class_object"); + auto results = std::make_unique(r, *config.schema->find("object"), *table); + + int notification_calls = 0; + auto token = results->add_notification_callback([&](CollectionChangeSet c, std::exception_ptr err) { + REQUIRE_FALSE(err); + ++notification_calls; + }); + + advance_and_notify(*r); + + auto write = [&](auto&& f) { + r->begin_transaction(); + f(); + r->commit_transaction(); + advance_and_notify(*r); + }; + + SECTION("notifications continue to work after Results is moved (move-constructor)") { + Results r(std::move(*results)); + results.reset(); + + write([&] { + table->set_int(0, table->add_empty_row(), 1); + }); + REQUIRE(notification_calls == 2); + } + + SECTION("notifications continue to work after Results is moved (move-assignment)") { + Results r; + r = std::move(*results); + results.reset(); + + write([&] { + table->set_int(0, table->add_empty_row(), 1); + }); + REQUIRE(notification_calls == 2); + } +} From 2a75edde8e1de0bedecdd256b9371c61cc01299c Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Wed, 1 Jun 2016 11:29:32 -0700 Subject: [PATCH 2/4] Move `List`s defaulted constructors and assignment operators out of line. --- src/list.cpp | 5 +++++ src/list.hpp | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/list.cpp b/src/list.cpp index 113f735d..e8e72ae4 100644 --- a/src/list.cpp +++ b/src/list.cpp @@ -32,6 +32,11 @@ using namespace realm::_impl; List::List() noexcept = default; List::~List() = default; +List::List(const List&) = default; +List& List::operator=(const List&) = default; +List::List(List&&) = default; +List& List::operator=(List&&) = default; + List::List(std::shared_ptr r, const ObjectSchema& s, LinkViewRef l) noexcept : m_realm(std::move(r)) , m_object_schema(&s) diff --git a/src/list.hpp b/src/list.hpp index 895e4a09..4b1e178d 100644 --- a/src/list.hpp +++ b/src/list.hpp @@ -47,10 +47,10 @@ public: List(std::shared_ptr r, const ObjectSchema& s, LinkViewRef l) noexcept; ~List(); - List(const List&) = default; - List& operator=(const List&) = default; - List(List&&) = default; - List& operator=(List&&) = default; + List(const List&); + List& operator=(const List&); + List(List&&); + List& operator=(List&&); const std::shared_ptr& get_realm() const { return m_realm; } Query get_query() const; From 3e952269dad9505e5d65c8279254b67f49eb9b62 Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Wed, 1 Jun 2016 11:48:35 -0700 Subject: [PATCH 3/4] Have `Results` default its copy constructor. The custom implementation was an attempt to ensure that `m_has_used_table_view` and `m_wants_background_updates` had appropriate intial values. Thomas pointed out that we can remove the reliance on the initial values by ensure that `prepare_async` sets `m_wants_background_updates`, removing the need for a custom copy constructor. --- src/results.cpp | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/results.cpp b/src/results.cpp index c71a4fff..70d35972 100644 --- a/src/results.cpp +++ b/src/results.cpp @@ -86,21 +86,7 @@ Results::Results(SharedRealm r, const ObjectSchema& o, TableView tv, SortOrder s REALM_ASSERT(m_sort.column_indices.size() == m_sort.ascending.size()); } -Results::Results(const Results& other) -: m_realm(other.m_realm) -, m_object_schema(other.m_object_schema) -, m_query(other.m_query) -, m_table_view(other.m_table_view) -, m_link_view(other.m_link_view) -, m_table(other.m_table) -, m_sort(other.m_sort) -, m_live(other.m_live) -, m_notifier() -, m_mode(other.m_mode) -, m_has_used_table_view(false) -, m_wants_background_updates(true) -{ -} +Results::Results(const Results& other) = default; #if 0 // FIXME: TableViewBase::operator= is missing from the core static library. @@ -538,6 +524,7 @@ void Results::prepare_async() } if (!m_notifier) { + m_wants_background_updates = true; m_notifier = std::make_shared<_impl::ResultsNotifier>(*this); _impl::RealmCoordinator::register_notifier(m_notifier); } From 08404d709811345e50a837e2f6bc49e8fd8e45b9 Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Wed, 1 Jun 2016 11:50:25 -0700 Subject: [PATCH 4/4] Express `Results`' asignment operators in terms of their equivalent constructors. This avoids having to repeat the move constructor's logic in the move assignment operator, and allows the copy assignment operator to compile despite `TableViewBase`'s missing copy assignment implementation. the copy assignment implementation can be defaulted once `TableViewBase` is fixed. --- src/results.cpp | 41 ++++++----------------------------------- 1 file changed, 6 insertions(+), 35 deletions(-) diff --git a/src/results.cpp b/src/results.cpp index 70d35972..9bb5a218 100644 --- a/src/results.cpp +++ b/src/results.cpp @@ -88,30 +88,16 @@ Results::Results(SharedRealm r, const ObjectSchema& o, TableView tv, SortOrder s Results::Results(const Results& other) = default; -#if 0 -// FIXME: TableViewBase::operator= is missing from the core static library. +// Cannot be defaulted as TableViewBase::operator= is missing from the core static library. +// Delegate to the copy constructor and move-assignment operators instead. Results& Results::operator=(const Results& other) { - if (this == &other) { - return *this; + if (this != &other) { + *this = Results(other); } - m_realm = other.m_realm; - m_object_schema = other.m_object_schema; - m_query = other.m_query; - m_table_view = other.m_table_view; - m_link_view = other.m_link_view; - m_table = other.m_table; - m_sort = other.m_sort; - m_live = other.m_live; - m_notifier.reset(); - m_mode = other.m_mode; - m_has_used_table_view = false; - m_wants_background_updates = true; - return *this; } -#endif Results::Results(Results&& other) : m_realm(std::move(other.m_realm)) @@ -134,23 +120,8 @@ Results::Results(Results&& other) Results& Results::operator=(Results&& other) { - m_realm = std::move(other.m_realm); - m_object_schema = std::move(other.m_object_schema); - m_query = std::move(other.m_query); - m_table_view = std::move(other.m_table_view); - m_link_view = std::move(other.m_link_view); - m_table = other.m_table; - m_sort = std::move(other.m_sort); - m_live = other.m_live; - m_notifier = std::move(other.m_notifier); - m_mode = other.m_mode; - m_has_used_table_view = other.m_has_used_table_view; - m_wants_background_updates = other.m_wants_background_updates; - - if (m_notifier) { - m_notifier->target_results_moved(other, *this); - } - + this->~Results(); + new (this) Results(std::move(other)); return *this; }