Fix a race condition in notifier initialization #4234
Conversation
Newly created notifiers copied their Query from the source Realm on the worker thread without locking the source Realm, which could race with things that invalidated the source Realm. To fix this, switch to copying the required data to a temporary Transaction owned by the notifier while still on the source thread. This increases the amount of work done on the source thread to create a notifier (as it has to create a new Transaction each time), but eliminates the need to hold the lock while parsing the transaction logs for new notifiers, so it may turn out to be a net reduction in main-thread blockage.
tgoyne
force-pushed
the
tg/notifier-invalid-table-ref
branch
from
7fb5488 to
16add52
Compare
|
Likely also fixes realm/realm-swift#6767 and realm/realm-swift#6916 |
|
@tgoyne Any possible connection to realm/realm-java#7100 in your opinion ? Same question for realm/realm-java#7145 ? (I note it has "run_async_notifiers" in the stacktrace) |
| auto& self = Realm::Internal::get_coordinator(*notifier->get_realm()); | ||
| { | ||
| util::CheckedLockGuard lock(self.m_notifier_mutex); | ||
| self.pin_version(version); | ||
| if (!self.m_async_error) | ||
| notifier->attach_to(notifier->get_realm()->duplicate()); |
There was a problem hiding this comment.
So this is the core of the fix.
There was a problem hiding this comment.
Yes, this ended up being the only change actually needed to fix the bug. The changes in run_async_notifiers() are left over from a different attempt at fixing the bug that failed to fix it but did bring a functional benefit (of not running new notifiers with the lock held).
|
realm/realm-java#7145 probably isn't related to this; a data race leading to the Query on the background thread having some invalid data could explain the crash, but AFAICT the racey read doesn't access anything mutated after the last synchronization point and I don't think this bug could lead to a use-after-free. realm/realm-java#7100 may be the same bug in a different place. Calling freeze() from a thread other than the object's thread is invalid, but Results::freeze() doesn't check the thread. Cocoa indirectly checks the thread before calling it and Java appears not to (although it's very possible I'm just failing to find where it does) and so is letting the user do something unsafe. |
Newly created notifiers copied their Query from the source Realm on the worker thread without locking the source Realm, which could race with things that invalidated the source Realm.
To fix this, switch to copying the required data to a temporary Transaction owned by the notifier while still on the source thread. This increases the amount of work done on the source thread to create a notifier (as it has to create a new Transaction each time), but eliminates the need to hold the lock while parsing the transaction logs for new notifiers, so it may turn out to be a net reduction in main-thread blockage.
Fixes #3761.