OpenSSLInitializer isn't threadsafe #1739
Comments
|
Additional information: This issue was reported in issue #1498, which was closed due to the workaround solving the issue. |
The init/uninit methods can be called from multiple threads, and thus need synchronization with a mutex.
|
I do not understand: Atomic counter should be "atomic". I don't understand the problem here: I use it in some multi threaded environments and I thought AtomicCounter was atomic. why is wrong? |
|
Atomic itself is indeed atomic; performing |
|
mmm I don't get it. |
|
The problem is with increments and decrements in a multithreaded environment? because if we just increment I do not see problem |
|
And if so we have the same problem in all Poco::Notification because RefCountedObject is used |
|
if there was only one place where |
|
Ok now it's quite clear, probably we have the problem even in other cases I talk about, because we have AtomicCounter incremented decremented and checked (==) in multithread environment. Probably we usually do all the increments and after all the decrements and probably that way should not be a problem. But it's anyway some kind of condition we have to check carefully because we can get into trouble otherwise |
|
It's a bit more complicated than that. This is a specific problem of OpenSSLInitializer, because of the leading to a race condition. It is only an issue if you have multiple threads calling The specific problem is that in a multithreaded scenario a thread (that had The actual comparison is not an issue, because you are comparing with the result of the atomic operation, not with the actual value at the time of comparison (which could be different already). With in a well-formed program, if |
|
All correct, except that the thread about to use the object is accessing a dead object. That thread has obtained the pointer and may access a live object, while the other one just happens to be done with it and is about to delete it - that is precisely what the shared pointer should prevent from happening. It is likely a rare scenario, but I think it's a legitimate concern. Generally speaking, comparison of an incremented or decremented atomic variable is thread-safe, as long as it is (a) done in one place only or (b) same operation is done in multiple places and compared with same value. When the opposite operations (probably the most frequent scenario), or comparisons with different values are done in multiple places, it is not thread-safe. |
|
What you want would be more like std::experimental::atomic_shared_ptr that std::shared_ptr. In any case, having two threads access the same SharedPtr instance (as opposed to different SharedPtr instances pointing to the same object) without external synchronisation is a really bad idea. |
|
And, the comparison after the atomic increment/decrement is not the issue, as you're comparing the result of the atomic operation, not the current (by the time the comparison happens) value. |
|
And regarding usage of the same SharedPtr (or AutoPtr) instance from multiple threads. Good luck if one thread assigns a new pointer (causing deletion of the object previously pointed to) while the other thread currently calls a method of that object through the same SharedPtr. |
|
I think the problem is still there even if you have two SharedPtr instances to the same object in two different thread. you can occur in the rare case above. The problem is if you have your reference counter incremented end decremented by different thread. Is not the use case you usually use SharedPtr because u usually create all the instances you need, so incrementing, and than decrementing on destroying. But probably the safest way to do the check is using a Mutex like a OpenSSLInitializer (or using some atomic_shard_ptr). |
|
Of course, the rule should be to copy SharedPtr when used by multiple threads and we should not concern ourselves with what users do. However, there is only one instance of the reference counter, shared by all SharedPtr copies. So, when one thread has obtained 0 reference count here, the other may subsequently increment it to 1 here. But I agree that such scenario is a bug because it means copying the last SharedPtr that is being destroyed, which will typically happen in the owning object destructor. As for atomics, yes I was wrong - after atomic increment/decrement, there is a local copy of the value and, regardless of what other threads do, the equality criteria will be satisfied in one thread only. |
|
Multithreading is hard ;-) Back then when I did the change to use AtomicCounter I spend quite some time thinking things through. Still, things like OpenSSLInitializer can happen :-( |
* Fix OpenSSLInitialized thread safety (#1739) The init/uninit methods can be called from multiple threads, and thus need synchronization with a mutex. * Renamed mutex variable and use ScopedLock. * Change reference count variable to be an integer, since it’s protected by a mutex and no longer needs to be atomic.
The initialize and uninitialize methods in OpenSSLInitializer uses a construct like this:
if (++_rc == 1) { ... }_rc is a Poco::AtomicCounter.
This is insufficient since you can easily get into a situation where both uninitialize and initialize is called at the same time from multiple threads. These methods should be protected by a mutex lock to ensure proper execution.
The workaround to keep a persistent instance of this object works of course, but that just avoids triggering the race condition.
The text was updated successfully, but these errors were encountered: