Possible data race in composite_subscription? #475
Description
composite_subscription_inner looks like it might have an incorrect implementation of double-checked locking ?
RxCpp/Rx/v2/src/rxcpp/rx-subscription.hpp
Line 245 in 7930ccc
inline weak_subscription add(subscription s) {
if (!issubscribed) {
s.unsubscribe();
} else if (s.is_subscribed()) {
std::unique_lock<decltype(lock)> guard(lock); // <<<<<<<<<<<<<
subscriptions.insert(s);
}
return s.get_weak();
}After acquiring the guard lock, issubscribed could already be false. Subsequently this races against either clear or unsubscribe.
Thread 1 Thread 2
-----------------------------------------------------------
add(s)
clear() or unsubscribe()
lock
call unsubscribe for all
unlock
block-for-lock
--> insert into set
unlock
Likely correct code:
inline weak_subscription add(subscription s) {
if (!issubscribed) {
s.unsubscribe();
} else if (s.is_subscribed()) {
std::unique_lock<decltype(lock)> guard(lock);
/*+*/ if (!issubscribed.load()) {
/*+*/ s.unsubscribe();
/*+*/ } else {
subscriptions.insert(s);
}
}
return s.get_weak();
}This potential issue seems repeated in most lock usages (add, remove, clear) except unsubscribe which is ok because of the exchange.
remove() also seems to have an additional data race with w:
inline void remove(const weak_subscription& w) {
if (issubscribed && !w.expired()) {
auto s = subscription::lock(w); /// <<< (2) this could already be expired
std::unique_lock<decltype(lock)> guard(lock); /// <<< (1) issubscribed could be false
subscriptions.erase(s); /// calls erase on moved-from set, could be benign
}
}and calling 'subscription::lock' on an expired subscription would then call std::terminate (which btw, seems like a strange API decision) but at least the program would crash immediately instead of having data corruption.
Another layer of possibly unspecified behaior:
The data race in expressed here in that std::set 'subscriptions' could be used (erase, insert, etc) even after it is moved-from in erase.
https://en.cppreference.com/w/cpp/container/set/set overload (4)
This pattern is repeated by clear/unsubscribe [even in single threaded code], in that they both call the move-constructor on subscriptions.