Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read access violation when refreshing SubscriptionSet #2952

Closed
papafe opened this issue Feb 1, 2022 · 14 comments · Fixed by realm/realm-core#5239 or #3111
Closed

Read access violation when refreshing SubscriptionSet #2952

papafe opened this issue Feb 1, 2022 · 14 comments · Fixed by realm/realm-core#5239 or #3111
Assignees

Comments

@papafe
Copy link
Contributor

papafe commented Feb 1, 2022

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 :

REALM_EXPORT void realm_subscriptionset_wait_for_state(SubscriptionSet* subs, void* task_completion_source, NativeException::Marshallable& ex)
{
handle_errors(ex, [&] {
subs->get_state_change_notification(SubscriptionSet::State::Complete)
.get_async([task_completion_source, subs](StatusWith<SubscriptionSet::State> status) mutable noexcept {
try {
subs->refresh();
if (status.is_ok()) {
s_state_wait_callback(task_completion_source, core_to_csharp_state(status.get_value()), realm_value_t{});
}
else {
s_state_wait_callback(task_completion_source, CSharpState::Error, to_capi_value(status.get_status().reason()));
}
}
catch (...) {
auto inner_ex = convert_exception();
auto error_message = util::format("%1: %2", inner_ex.message, inner_ex.detail);
s_state_wait_callback(task_completion_source, CSharpState::Error, to_capi_value(error_message));
}
});
});
}

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
Copy link

jbreams commented Feb 7, 2022

Do you have a full stacktrace of this failure?

@sync-by-unito
Copy link

sync-by-unito bot commented Feb 7, 2022

➤ Jonathan Reams commented:

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

@sync-by-unito
Copy link

sync-by-unito bot commented Feb 7, 2022

➤ Daniel Tabacaru commented:

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

@papafe
Copy link
Contributor Author

papafe commented Feb 8, 2022

@danieltabacaru I'm not feeling really well, but the failing test is here (#2797). Feel free to take it with someone else of the .NET team, they should be able to run it with you.

@danieltabacaru
Copy link

@papafe No worries

@nirinchev
Copy link
Member

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
Copy link

jsflax commented Feb 8, 2022

Are we safe to close this then?

@jbreams jbreams removed their assignment Feb 8, 2022
@nirinchev
Copy link
Member

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
Copy link
Member

nirinchev commented May 9, 2022

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();

@nirinchev nirinchev reopened this May 9, 2022
@sync-by-unito
Copy link

sync-by-unito bot commented May 10, 2022

➤ 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
Copy link
Member

nirinchev commented May 10, 2022

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
Copy link

sync-by-unito bot commented May 10, 2022

➤ 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
Copy link

sync-by-unito bot commented May 10, 2022

➤ Daniel Tabacaru commented:

[~nikola.irinchev@mongodb.com] do you have a stracktrace?

@nirinchev
Copy link
Member

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.

@nicola-cab nicola-cab transferred this issue from realm/realm-core Jun 8, 2022
@bmunkholm bmunkholm reopened this Jun 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants