From 28b62af55542a1c1df4cc880fea38d25f7563b81 Mon Sep 17 00:00:00 2001
From: Thomas Goyne <tg@realm.io>
Date: Thu, 27 Apr 2023 09:30:31 -0700
Subject: [PATCH] Fix a crash when beginning an async write triggers
 notifications which close the Realm

Beginning a write transaction can result in the Realm being closed as it can
trigger notifications, and the async write loop was missing a check for this.
---
 CHANGELOG.md                            |  1 +
 src/realm/object-store/shared_realm.cpp |  6 +++
 test/object-store/realm.cpp             | 57 +++++++++++++++++++------
 3 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0868e71d31e..1efb9ccad85 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@
 * If you freeze a Results based on a collection of objects, the result will be invalid if you delete the collection ([#6635](https://github.com/realm/realm-core/issues/6635), since V13.11.0)
 * Geospatial polygons now have built in normalization and validation in line with the MongoDB server side behaviour and the geoJSON standard. ([#6607](https://github.com/realm/realm-core/pull/6607), since v13.11.0)
 * Dictionary::get_any() would expose unresolved links rather than mapping them to null. In addition to allowing invalid objects to be read from Dictionaries, this resulted in queries on Dictionaries sometimes having incorrect results ([#6644](https://github.com/realm/realm-core/pull/6644)).
+* Fix a null pointer dereference if beginning an async write transaction refreshed the Realm and one of the notification handlers closed the Realm ([PR #6548](https://github.com/realm/realm-core/pull/6548), since v11.8.0).
 
 ### Breaking changes
 * `platform` and `cpu_arch` fields in the `device_info` structure in App::Config can no longer be provided by the SDK's, they are inferred by the library ([PR #6612](https://github.com/realm/realm-core/pull/6612))
diff --git a/src/realm/object-store/shared_realm.cpp b/src/realm/object-store/shared_realm.cpp
index c348161863e..3fcd5fe3860 100644
--- a/src/realm/object-store/shared_realm.cpp
+++ b/src/realm/object-store/shared_realm.cpp
@@ -803,6 +803,12 @@ void Realm::run_writes()
 
         do_begin_transaction();
 
+        // Beginning the transaction may have delivered notifications, which
+        // then may have closed the Realm.
+        if (!m_transaction) {
+            return;
+        }
+
         auto write_desc = std::move(m_async_write_q.front());
         m_async_write_q.pop_front();
 
diff --git a/test/object-store/realm.cpp b/test/object-store/realm.cpp
index 9c945808c71..9a0a511a542 100644
--- a/test/object-store/realm.cpp
+++ b/test/object-store/realm.cpp
@@ -1759,24 +1759,31 @@ TEST_CASE("SharedRealm: async writes") {
                 REQUIRE(complete_count == 1);
                 verify_persisted_count(1);
             }
-            SECTION("within did_change()") {
-                struct Context : public BindingContext {
-                    int i;
-                    Context(int i)
-                        : i(i)
-                    {
-                    }
-                    void did_change(std::vector<ObserverState> const&, std::vector<void*> const&, bool) override
-                    {
-                        std::invoke(close_functions[i], *realm.lock());
-                    }
-                };
-                realm->m_binding_context.reset(new Context(i));
+
+            struct Context : public BindingContext {
+                int i;
+                bool& called;
+                Context(int i, bool& called)
+                    : i(i)
+                    , called(called)
+                {
+                }
+                void did_change(std::vector<ObserverState> const&, std::vector<void*> const&, bool) override
+                {
+                    called = true;
+                    std::invoke(close_functions[i], *realm.lock());
+                }
+            };
+            SECTION("within did_change() after committing") {
+                bool called = false;
+                realm->m_binding_context.reset(new Context(i, called));
                 realm->m_binding_context->realm = realm;
 
                 realm->async_begin_transaction([&] {
                     table->create_object().set(col, 45);
+                    CHECK_FALSE(called);
                     realm->async_commit_transaction([&](std::exception_ptr) {
+                        CHECK(called);
                         done = true;
                     });
                 });
@@ -1784,6 +1791,30 @@ TEST_CASE("SharedRealm: async writes") {
                 wait_for_done();
                 verify_persisted_count(1);
             }
+
+            SECTION("within did_change() when beginning") {
+                realm->m_binding_context.reset(new Context(i, done));
+                realm->m_binding_context->realm = realm;
+
+                // Make a write on a different instance while autorefresh is
+                // off to ensure that beginning the transaction advances the
+                // read version and thus sends notifications
+                realm->set_auto_refresh(false);
+                auto realm2 = Realm::get_shared_realm(config);
+                realm2->begin_transaction();
+                realm2->commit_transaction();
+
+                bool called = false;
+                realm->async_begin_transaction([&] {
+                    called = true;
+                });
+                wait_for_done();
+
+                // close() inside a notification closes the Realm, but invalidate()
+                // is a no-op. This means the write callback should be invoked
+                // if we're testing invalidate() but not if we're testing close().
+                REQUIRE(called == i);
+            }
         }
     }