heap-use-after-free in rx-subject.hpp

[shivashankarp]

When I was testing with Thread Sanitizer, it kept complaining about this piece of code in rx-subject.hpp and it took a while for me to realize the problem here.

    if (b->current_generation != b->state->generation) {
        std::unique_lock<std::mutex> guard(b->state->lock);
        b->current_generation = b->state->generation;
        b->current_completer = b->completer;
    }

    auto current_completer = b->current_completer;

What happens if Thread-A updates b->current_completer just before Thread-B assigns b->current_completer to the local variable? Assuming no one else is holding reference to b->current_completer, the instance will be destroyed by Thread-A and Thread-B may start operating on the destructed heap space. Can you confirm?

The obvious solution is to not depend on generation numbers and always acquire the mutex and assign b->completer to a local variable. I can submit a patch if we agree on the issue and a possible solution.

[kirkshoop]

Yes, this is broken.

I have not seen this failing in practice. probably because generations are relatively rare and the shared_ptr refcounts are accessed atomically, so only the pointer-to the refcount block is handled unsafely.

There will be an atomic_shared_ptr in the near future that will enable this safely. But it is not generally available now.

The generations were added for perf reasons, mutex was so slow. I think that mutex has been improved on most platforms, so I expect a ton of complexity could be removed from subject if it was to begin locking a mutex in each on_next(). The completer and binder structs should disappear.

I would like to see this change, and check to see how the perf compares.

Thanks!

[shivashankarp]

I'd suggest that we retain the completer and binder structs so that it'll be easier to use atomic_shared_ptr once it is available. For now, I'll provide a quick patch for comparing the perf with mutex usage in on_next.