From 65a748de0ca16e150ca71fdf32e980707a6bb82d Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Tue, 31 May 2016 13:11:45 -0700 Subject: [PATCH] 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); + } +}