Read access violation when refreshing SubscriptionSet

[papafe]

We are getting a "Read access violation" in here:

https://github.com/realm/realm-core/blob/6b05da97d0a9bd0bf61522a0342222443b35e196/src/realm/sync/subscriptions.cpp#L708

when refreshing the SubscriptionSet (line 335) in the callback of get_state_change_notification :

https://github.com/realm/realm-dotnet/blob/db0d9023937d06c9df089257a09fd4c16a4ca530/wrappers/src/subscription_set_cs.cpp#L329-L350

This happens only when previously we committed an empty write to the SubscriptionSet. By "empty", I mean that no query was added or removed, so the refresh should probably be a no-op.

[jbreams]

Do you have a full stacktrace of this failure?

[sync-by-unito[bot]]

➤ Jonathan Reams commented:

[~daniel.tabacaru], can you take a look at this with [~ferdinando.papale] ?

[sync-by-unito[bot]]

➤ Daniel Tabacaru commented:

[~jonathan.reams] I'm looking into it.

[papafe]

@danieltabacaru I'm not feeling really well, but the failing test is here (https://github.com/realm/realm-dotnet/pull/2797). Feel free to take it with someone else of the .NET team, they should be able to run it with you.

[danieltabacaru]

@papafe No worries

[nirinchev]

Okay, so this turned out to be a combination of 2 bugs.

1. get_state_change_notification not completing, which has been fixed in latest Core + server.
2. The Read access violation we get when trying to call refresh after the Realm instance has been closed.

The reason for 2. manifesting here is that we were hitting a timeout due to 1., which fails the test, then we would close all Realm instances created during the test, triggering 2. A pseudocode repro case for 2, should be something like:

auto realm = Realm::get_shared_realm(config);
auto subs = realm.get_latest_subscription_set();
realm.close();

// this should crash as `m_mgr` should have been invalidated
subs.refresh();

[jsflax]

Are we safe to close this then?

[nirinchev]

The second bug is a core bug. We should either change refresh() and similar methods to be no-ops when operating on a subscription set with an invalidated SubscriptionStore or they should throw exceptions rather than crash.

[nirinchev]

I'm still able to reproduce the access violation crash. This is the .NET test:

var testGuid = Guid.NewGuid();

Task waitTask = null;
using (var realm = await GetFLXIntegrationRealmAsync())
{
    realm.Subscriptions.Update(() =>
    {
        var query = realm.All<SyncAllTypesObject>().Where(o => o.GuidProperty == testGuid);

        realm.Subscriptions.Add(query);
    });

    waitTask = realm.Subscriptions.WaitForSynchronizationAsync();
}

await waitTask;

This roughly translates into:

auto realm = Realm::get_shared_realm(config);
auto subs = realm.get_latest_subscription_set();

subs.update(...);

subs->get_state_change_notification(SubscriptionSet::State::Complete)
    .get_async([task_completion_source, subs](StatusWith<SubscriptionSet::State> status) mutable noexcept {
        // this crashes as `m_mgr` has been invalidated
        subs->refresh();
    });

realm.close();

[sync-by-unito[bot]]

➤ Jonathan Reams commented:

in your example

auto realm = Realm::get_shared_realm(config);
auto subs = realm.get_latest_subscription_set();
// assume you're doing auto mut_subs = subs.make_mutable_copy(); and then doing your mutations on mut_subs below?
subs.update(...);
// do you commit mut_subs here and re-assign subs to its return value?
// are you getting a state change notification on the return value of commit() or on mut_subs?
subs->get_state_change_notification(SubscriptionSet::State::Complete)
            .get_async([task_completion_source, subs](StatusWith<SubscriptionSet::State> status) mutable noexcept {
// do you check that status.is_ok() here?
                // this crashes as `m_mgr` has been invalidated
                subs->refresh();
            });

realm.close();

[nirinchev]

Apologies, I translated the C# code too sloppily.

// replace subs.update with below

auto mut_subs = subs.make_mutable_copy();
mut_subs.insert_or_assign(query);
subs = std::move(mut_subs).commit();

subs->get_state_change_notification(...);

So yes, I'm reassigning subs to the return value of mut_subs.commit() and I'm getting state change notification on subs that now holds the returned SubscriptionSet from commit.

[sync-by-unito[bot]]

➤ Jonathan Reams commented:

and does the is_ok() of the StatusWith passed to the callback return true and is the crash a segmentation fault or a std::logic_error getting thrown?

[sync-by-unito[bot]]

➤ Daniel Tabacaru commented:

[[email protected]] do you have a stracktrace?

[nirinchev]

Apologies for the delay here - I've been swamped with other stuff, so didn't have time to come back to this. Here's the stacktrace

>	realm-wrappers.dll!std::_Ptr_base<realm::sync::SubscriptionStore const>::_Construct_from_weak<realm::sync::SubscriptionStore const>(const std::weak_ptr<realm::sync::SubscriptionStore const> & _Other) Line 1310	C++
 	realm-wrappers.dll!std::weak_ptr<realm::sync::SubscriptionStore const>::lock() Line 3022	C++
 	realm-wrappers.dll!realm::sync::SubscriptionSet::get_flx_subscription_store() Line 153	C++
 	realm-wrappers.dll!realm::sync::SubscriptionSet::refresh() Line 351	C++
 	realm-wrappers.dll!realm_subscriptionset_wait_for_state::__l2::void <lambda>(void)::__l2::<lambda>(realm::StatusWith<enum realm::sync::SubscriptionSet::State> status) Line 336	C++
 	realm-wrappers.dll!realm::util::future_details::call<void <lambda>(realm::StatusWith<enum realm::sync::SubscriptionSet::State>) &,realm::StatusWith<enum realm::sync::SubscriptionSet::State>>(realm_subscriptionset_wait_for_state::__l2::void <lambda>(void)::__l2::void <lambda>(realm::StatusWith<enum realm::sync::SubscriptionSet::State>) & func, realm::StatusWith<enum realm::sync::SubscriptionSet::State> && arg) Line 112	C++
 	realm-wrappers.dll!realm::util::future_details::Future<enum realm::sync::SubscriptionSet::State>::get_async::__l2::void <lambda>(void)::__l2::<lambda>(realm::util::future_details::SharedStateBase * ssb) Line 697	C++
 	realm-wrappers.dll!realm::util::UniqueFunction<void __cdecl(realm::util::future_details::SharedStateBase *)>::call_regular_void<void <lambda>(realm::util::future_details::SharedStateBase *)>(const std::integral_constant<bool,1> is_void, realm::util::future_details::Future<enum realm::sync::SubscriptionSet::State>::get_async::__l2::void <lambda>(void)::__l2::void <lambda>(realm::util::future_details::SharedStateBase *) & f, realm::util::future_details::SharedStateBase * && <args_0>) Line 149	C++
 	realm-wrappers.dll!realm::util::UniqueFunction<void __cdecl(realm::util::future_details::SharedStateBase *)>::SpecificImpl<void <lambda>(realm::util::future_details::SharedStateBase *)>::call(realm::util::future_details::SharedStateBase * && <args_0>) Line 168	C++
 	realm-wrappers.dll!realm::util::UniqueFunction<void __cdecl(realm::util::future_details::SharedStateBase *)>::operator()(realm::util::future_details::SharedStateBase * <args_0>) Line 94	C++
 	realm-wrappers.dll!realm::util::future_details::SharedStateBase::transition_to_finished() Line 331	C++
 	realm-wrappers.dll!realm::util::future_details::SharedStateBase::set_status(realm::Status status) Line 341	C++
 	realm-wrappers.dll!realm::util::future_details::Promise<enum realm::sync::SubscriptionSet::State>::~Promise<enum realm::sync::SubscriptionSet::State>() Line 462	C++
 	[External Code]	
 	realm-wrappers.dll!realm::sync::SessionWrapper::~SessionWrapper() Line 794	C++
 	[External Code]	
 	realm-wrappers.dll!realm::util::AtomicRefCountBase::unbind_ptr() Line 369	C++
 	realm-wrappers.dll!realm::sync::ClientImpl::actualize_and_finalize_session_wrappers() Line 559	C++
 	realm-wrappers.dll!realm::util::network::Trigger::ExecOper<<lambda_096606a26adbfa11c7cff43681b8ca79>>::recycle_and_execute() Line 3643	C++
 	realm-wrappers.dll!realm::util::network::Service::Impl::execute(std::unique_ptr<realm::util::network::Service::AsyncOper,realm::util::network::Service::LendersOperDeleter> & lenders_ptr) Line 1631	C++
 	realm-wrappers.dll!realm::util::network::Service::Impl::run() Line 1392	C++
 	realm-wrappers.dll!realm::util::network::Service::run() Line 1756	C++
 	realm-wrappers.dll!realm::sync::ClientImpl::run() Line 481	C++
 	realm-wrappers.dll!realm::sync::Client::run() Line 1462	C++
 	realm-wrappers.dll!realm::_impl::SyncClient::<lambda>() Line 81	C++
 	[External Code]	
 	realm-wrappers.dll!thread_start<unsigned int (__stdcall*)(void *),1>(void * const parameter) Line 97	C++

So I think this may be due to the design of the .NET SDK. My overly simplified translation is missing key details. In reality, we're using pointers, not values and when the Realm gets closed, we delete the pointer to the subscriptions. I guess a more realistic translation would be:

auto realm = Realm::get_shared_realm(config);
auto subs = new SubscriptionSet(realm.get_latest_subscription_set());

auto mut_subs = subs.make_mutable_copy();
mut_subs.insert_or_assign(query);
subs = new SubscriptionSet(std::move(mut_subs).commit());

subs->get_state_change_notification(SubscriptionSet::State::Complete)
    .get_async([task_completion_source, subs](StatusWith<SubscriptionSet::State> status) mutable noexcept {
        // subs here seems to be pointing to garbage data
        subs->refresh();
    });

realm.close();
delete subs;

So as far as I can tell, this is a bug in the .NET wrapper code and not in Core. I'll try and investigate more with @fealebenpae and will get back to you if anything from Core is needed.