Bug #654
Proposed patch to mutex.c
Description
The code in src/libstrongswan/threading/mutex.c performs (without lock):
pthread_t self = pthread_self();
if (pthread_equal(this->thread, self))
when this->thread is concurrently mutated.
This can break even on single CPU due to compiler optimizations.
Compilers assume that programs are race-free.
On top of that pthread_t is not guaranteed to be a single word,
something that this code assumes.
The proposed patch fixes this by adding an additional mutex that protects the pthread_t.
History
#1 Updated by Tobias Brunner about 11 years ago
- Tracker changed from Issue to Bug
- Category set to libstrongswan
- Status changed from New to Feedback
- Assignee set to Tobias Brunner
- Target version set to 5.2.1
Thanks for the report and the patch.
Introducing a global mutex is not optimal, all threads that want to access any recursive mutex would have to compete for it. They wouldn't hold it very long, but it still might create a bottleneck.
The associated commit uses atomic compare-and-swap operations and a pointer to our thread abstraction (which is stored in thread local storage) to determine whether the mutex is currently held. On some platforms this might still require a global mutex, but on most CAS is implemented efficiently.
#2 Updated by Noam Lampert about 11 years ago
I didn't realized strongswan was already using compare-and-swap. Your patch is definitely better.
For reference only, the newly introduced mutex in the original was not global, but rather part of the recursive-mutex, so two threads locking two different mutexes don't compete.
Also, the implementation of cas_xxx when !defined(HAVE_GCC_ATOMIC_OPERATIONS) can be improved my making the cas_mutex and array of mutexes and using the pointer to chose which mutexto lock. Not sure this is relevant, if there even are platforms in which HAVE_GCC_ATOMIC_OPERATIONS is not defined.
#3 Updated by Tobias Brunner about 11 years ago
For reference only, the newly introduced mutex in the original was not global, but rather part of the recursive-mutex, so two threads locking two different mutexes don't compete.
You're right, sorry. Should have looked more closely at the patch. If you send other patches could you please send them in the unified diff format (git diff, or with diff -u). Thanks!
Also, the implementation of cas_xxx when !defined(HAVE_GCC_ATOMIC_OPERATIONS) can be improved my making the cas_mutex and array of mutexes and using the pointer to chose which mutexto lock. Not sure this is relevant, if there even are platforms in which HAVE_GCC_ATOMIC_OPERATIONS is not defined.
Agreed, probably not worth the effort.
#4 Updated by Tobias Brunner almost 11 years ago
- Status changed from Feedback to Closed
- Resolution set to Fixed