diff --git a/CHANGELOG.md b/CHANGELOG.md index c786c2a977f..bfc1615f1d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) * Fix a data race reported by thread sanitizer when preparing to deliver change notifications. This probably did not cause observable problems in practice ([PR #5892](https://github.com/realm/realm-core/pull/5892) since 12.7.0). * Changed the behaviour of creating a collection of non-nullable Mixed type to throw a descriptive error message rather than asserting in debug mode. ([#5894](https://github.com/realm/realm-core/issues/5894), since the introduction of the Mixed type). +* Fix a use-after-free when a sync session is closed and the app is destroyed at the same time ([#5752](https://github.com/realm/realm-core/issues/5752), since v11.5.2). ### Breaking changes * None. diff --git a/src/realm/object-store/impl/realm_coordinator.cpp b/src/realm/object-store/impl/realm_coordinator.cpp index 3dfe9f1ede1..97dca55def7 100644 --- a/src/realm/object-store/impl/realm_coordinator.cpp +++ b/src/realm/object-store/impl/realm_coordinator.cpp @@ -329,14 +329,6 @@ std::shared_ptr RealmCoordinator::get_synchronized_realm(Realm::C return std::make_shared(shared_from_this(), m_sync_session); } -void RealmCoordinator::create_session(const Realm::Config& config) -{ - REALM_ASSERT(config.sync_config); - util::CheckedLockGuard lock(m_realm_mutex); - set_config(config); - open_db(); -} - #endif REALM_NOINLINE void realm::_impl::translate_file_exception(StringData path, bool immutable) diff --git a/src/realm/object-store/impl/realm_coordinator.hpp b/src/realm/object-store/impl/realm_coordinator.hpp index e2aa97b0b80..f76813fec6f 100644 --- a/src/realm/object-store/impl/realm_coordinator.hpp +++ b/src/realm/object-store/impl/realm_coordinator.hpp @@ -68,11 +68,6 @@ class RealmCoordinator : public std::enable_shared_from_this { std::shared_ptr get_synchronized_realm(Realm::Config config) REQUIRES(!m_realm_mutex, !m_schema_cache_mutex); - // Creates the underlying sync session if it doesn't already exists. - // This is also created as part of opening a Realm, so only use this - // method if the session needs to exist before the Realm does. - void create_session(const Realm::Config& config) REQUIRES(!m_realm_mutex, !m_schema_cache_mutex); - std::shared_ptr sync_session() REQUIRES(!m_realm_mutex) { util::CheckedLockGuard lock(m_realm_mutex); diff --git a/src/realm/object-store/sync/sync_manager.cpp b/src/realm/object-store/sync/sync_manager.cpp index 0095a449baf..a9f0a1c7f82 100644 --- a/src/realm/object-store/sync/sync_manager.cpp +++ b/src/realm/object-store/sync/sync_manager.cpp @@ -490,7 +490,7 @@ void SyncManager::delete_user(const std::string& user_id) } } -SyncManager::~SyncManager() +SyncManager::~SyncManager() NO_THREAD_SAFETY_ANALYSIS { // Grab a vector of the current sessions under a lock so we can shut them down. We have to make a copy because // session->shutdown_and_wait() will modify the m_sessions map. @@ -508,14 +508,6 @@ SyncManager::~SyncManager() session->detach_from_sync_manager(); } - // At this point we should have drained all the sessions. -#ifdef REALM_DEBUG - { - util::CheckedLockGuard lk(m_session_mutex); - REALM_ASSERT(m_sessions.empty()); - } -#endif - { util::CheckedLockGuard lk(m_user_mutex); for (auto& user : m_users) { diff --git a/src/realm/object-store/sync/sync_session.cpp b/src/realm/object-store/sync/sync_session.cpp index 6bd1caaeb9c..c11b1876211 100644 --- a/src/realm/object-store/sync/sync_session.cpp +++ b/src/realm/object-store/sync/sync_session.cpp @@ -143,9 +143,16 @@ void SyncSession::become_inactive(util::CheckedUniqueLock lock, std::error_code std::swap(waits, m_completion_callbacks); m_session = nullptr; - auto& sync_manager = *m_sync_manager; + std::shared_ptr sync_manager; + if (m_sync_manager) { + // This method may be called from the SyncManager's destructor. In this case, using 'shared_from_this()' will + // throw 'std::bad_weak_ptr' so we use a weak_ptr as intermediate. + sync_manager = m_sync_manager->weak_from_this().lock(); + } m_state_mutex.unlock(lock); - sync_manager.unregister_session(m_db->get_path()); + if (sync_manager) { + sync_manager->unregister_session(m_db->get_path()); + } // Send notifications after releasing the lock to prevent deadlocks in the callback. if (old_state != new_state) { @@ -963,9 +970,14 @@ void SyncSession::close(util::CheckedUniqueLock lock) m_state_mutex.unlock(lock); break; case State::Inactive: { - auto& sync_manager = *m_sync_manager; + std::shared_ptr sync_manager; + if (m_sync_manager) { + sync_manager = m_sync_manager->weak_from_this().lock(); + } m_state_mutex.unlock(lock); - sync_manager.unregister_session(m_db->get_path()); + if (sync_manager) { + sync_manager->unregister_session(m_db->get_path()); + } break; } case State::WaitingForAccessToken: diff --git a/src/realm/object-store/sync/sync_session.hpp b/src/realm/object-store/sync/sync_session.hpp index ad5ae6b8c4b..6d12ab0928d 100644 --- a/src/realm/object-store/sync/sync_session.hpp +++ b/src/realm/object-store/sync/sync_session.hpp @@ -191,6 +191,9 @@ class SyncSession : public std::enable_shared_from_this { // longer be open on behalf of it. void shutdown_and_wait() REQUIRES(!m_state_mutex, !m_connection_state_mutex); + // DO NOT CALL OUTSIDE OF TESTING CODE. + void detach_from_sync_manager() REQUIRES(!m_state_mutex, !m_connection_state_mutex); + // The access token needs to periodically be refreshed and this is how to // let the sync session know to update it's internal copy. void update_access_token(const std::string& signed_token) REQUIRES(!m_state_mutex, !m_config_mutex); @@ -344,7 +347,6 @@ class SyncSession : public std::enable_shared_from_this { void create_sync_session() REQUIRES(m_state_mutex, !m_config_mutex); void did_drop_external_reference() REQUIRES(!m_state_mutex, !m_config_mutex, !m_external_reference_mutex, !m_connection_state_mutex); - void detach_from_sync_manager() REQUIRES(!m_state_mutex, !m_connection_state_mutex); void close(util::CheckedUniqueLock) RELEASE(m_state_mutex) REQUIRES(!m_config_mutex, !m_connection_state_mutex); void become_active() REQUIRES(m_state_mutex, !m_config_mutex); diff --git a/test/object-store/sync/session/session.cpp b/test/object-store/sync/session/session.cpp index 9fe624c8057..96ca3acb73c 100644 --- a/test/object-store/sync/session/session.cpp +++ b/test/object-store/sync/session/session.cpp @@ -248,6 +248,13 @@ TEST_CASE("SyncSession: close() API", "[sync]") { session->close(); REQUIRE(sessions_are_inactive(*session)); } + + SECTION("Close session after it was detached from the SyncManager") { + auto session = sync_session( + user, "/test-close-after-detach", [](auto, auto) {}, SyncSessionStopPolicy::Immediately); + session->detach_from_sync_manager(); + REQUIRE_NOTHROW(session->close()); + } } TEST_CASE("SyncSession: shutdown_and_wait() API", "[sync]") {