On Wed, Jul 30, 2003 at 11:21:21AM -0500, William Rowe wrote: > Attached is a patch that appears to be as close as thread-safe > (but altogether not reentrant) as I can accomplish. > > The existing unix/thread_mutex.c code is neither thread-safe > nor reentrant when using the 'nested' (default) thread mutexes.
Why do you say "nested (default)"? The pools code is the only place I can find which actually uses nested mutexes. By default they are not nested. > The bug is most often expressed on SMP boxes, where they > are so massively parallel that the subtle flaws of modifying the > mutex object AFTER we have released the lock become very > apparent. > > Examples include a number of error (45) Deadlock Avoided > alerts using Apache2/mod_ssl on Solaris, where there are both > internal thread locks and global locks which contain a nested > thread lock. This code looks like it is making lots of dubious assumptions: - pthread_t can be a structure, that's why the memset is used I presume, assinging 0 to a pthread_t definitely wins no prizes - who says a memset-to-zero'ed pthread_t isn't a valid pthread_t? No reason why it couldn't be. If it is *not* a valid pthread_t, then passing it to pthread_equal gives undefined behaviour anyway! - let's hope and pray that pthread_equal() is an atomic operation > Many more eyes on this code would be welcomed, I expect to > discover that we repeat this mistake elsewhere in the mutex > code... Like Aaron I'm not sure why the comments about reentrancy are relevant: you can't use pthread mutexes (and hence apr mutexes :) from a signal handler, anyone doing that is SOL. You've made lots of changes in the patch: 1. changing some incr++ to ++incr which makes no differences 2. re-ordered the mutex_unlock code so that the ownership changes occur inside the critical section. 3. added some "protection" against races 1 and 3 look bogus; 2 looks like the important change here. The only effect of your (3) changes would seem to be to hide races if there still are any? (2) does introduce a semantic change in that the caller cannot rely on pthread_mutex_unlock() having defined behaviour if called from a thread which does not own the mutex lock, but I expect that's no big deal. It's worth noting that most modern POSIX implementations offer "recursive" mutexes in which pthread_mutex_lock/unlock do all this work internally. joe
