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

Do not capture this in on_new_flx_subscription_set callback #6129

Merged
merged 5 commits into from
Jan 4, 2023

Conversation

jbreams
Copy link
Contributor

@jbreams jbreams commented Dec 15, 2022

What, How & Why?

We need to make sure we extend the lifetime of the SessionWrapper into the callback of the on_new_flx_subscription_set callback or else we can end up working with already freed memory.

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@jbreams jbreams marked this pull request as ready for review January 4, 2023 14:03
CHANGELOG.md Outdated
@@ -15,6 +15,7 @@
* Compare actual users (`SyncUser::operator!=`), not pointers (`shared_ptr<SyncUser>::operator!=`). ([#realm/realm-dart#1055](https://github.com/realm/realm-dart/issues/1055), since v10.2.0)
* Core should not alter the order of the properties for additive schemas. ([#6134](https://github.com/realm/realm-core/issues/6134))
* Core should not use the version passed in `Realm` constructor to load the schema at some particular version, but only for setting `m_frozen_version`.
* Fixed possible segfault in sync client where async callback was not extending lifetime of `this` ([#6053](https://github.com/realm/realm-core/issues/6053), since v11.7.0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is not very user friendly; could rephrase into: "Fixed possible segfault in sync client where async callback was using object after being deallocated"

Copy link
Contributor

@michael-wb michael-wb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - just one merge issue when you pulled the latest master.

@@ -1058,20 +1058,20 @@ void SessionWrapper::on_new_flx_subscription_set(int64_t new_version)
return;
}

m_client.post([new_version, this](Status status) {
auto self = util::bind_ptr<SessionWrapper>(this);
m_client.get_service().post([new_version, self = std::move(self)](Status status) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this as m_client.post(...), since m_client.get_service() is deprecated.

@jbreams jbreams merged commit 06ba2b8 into master Jan 4, 2023
@jbreams jbreams deleted the jbr/lifetime_issues branch January 4, 2023 18:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read access violation in SessionWrapper::on_new_flx_subscription_set
3 participants