PROC_PTHREAD_SERIALIZEThe current PROC_PTHREAD_SERIALIZE implementation mmap()s a shared pthread_mutex_t (PTHREAD_PROCESS_SHARED), and the cleanup calls both pthread_mutex_destroy() and munmap() when the pool used with _create() is cleared or _destroy() is called explicitly.
In the (common) case where a parent process creates the shared mutex and then spawns/fork()s children processes, the parent can/should not destroy the mutex until all its children have finished. This is because pthread_mutex_destroy() can mark the mmap()ed pthread_mutex_t as invalid (at least on Linux and probably Solaris too according to PR 49504), so any child still using the mutex might either deadlock or face an unexpected lock/unlock error (we are in the undefined behaviour area...). This does not happen with eg. "sysvsem" where the IPC mechanism handles the destruction just fine when processes are still attached, same with file-backed mechanisms. So for PROC_PTHREAD_SERIALIZE, I propose to use a refcounter (atomic) so that the last process only destroys the shared mutex, the creator takes a reference as well as any child via apr_proc_mutex_child_init(). A cleanup is registered on apr_proc_mutex_child_init()'s pool, so that any leaving child will unref the mutex, and the last one will destroy it if the parent process has dropped its reference already. I first thought we could simply omit the pthread_mutex_destroy() call since anyway it is unlikely to free system resources (pthread_mutex_t is a simple placeholder AFAICT, with no resource associated), so munmap() would have done it (until the last child exits). But I can't talk for all pthread_mutex implementations... the system may maintain an overall list of PTHREAD_PROCESS_SHARED mutexes which would then be broken. So I find this solution better because it's still up to the user to call apr_proc_mutex_create() and apr_proc_mutex_child_init() appropriately or [s]he will be warned/errored/deadlocked when the logic fails (which is good to know). Proposed patch attached (can add tests later if this is acceptable), thoughts? Regards, Yann.
Index: locks/unix/proc_mutex.c =================================================================== --- locks/unix/proc_mutex.c (revision 1722098) +++ locks/unix/proc_mutex.c (working copy) @@ -19,6 +19,7 @@ #include "apr_arch_proc_mutex.h" #include "apr_arch_file_io.h" /* for apr_mkstemp() */ #include "apr_md5.h" /* for apr_md5() */ +#include "apr_atomic.h" APR_DECLARE(apr_status_t) apr_proc_mutex_destroy(apr_proc_mutex_t *mutex) { @@ -417,7 +418,24 @@ static const apr_proc_mutex_unix_lock_methods_t mu #if APR_HAS_PROC_PTHREAD_SERIALIZE -static apr_status_t proc_mutex_proc_pthread_cleanup(void *mutex_) +/* The mmap()ed pthread_interproc is the native pthread_mutex_t followed + * by a refcounter to track children using it. We want to avoid calling + * pthread_mutex_destroy() on the shared mutex area while it is in use by + * another process, because this may mark the shared pthread_mutex_t as + * invalid for everyone, including forked children (unlike "sysvsem" for + * example), causing unexpected errors or deadlocks (PR 49504). So the + * last process (parent or child) referencing the mutex will effectively + * destroy it. + */ +typedef struct { + pthread_mutex_t mutex; + apr_uint32_t refcount; +} proc_pthread_mutex_t; + +#define proc_pthread_mutex_refcount(m) \ + (((proc_pthread_mutex_t *)(m)->pthread_interproc)->refcount) + +static apr_status_t proc_pthread_mutex_unref(void *mutex_) { apr_proc_mutex_t *mutex=mutex_; apr_status_t rv; @@ -430,8 +448,7 @@ static const apr_proc_mutex_unix_lock_methods_t mu return rv; } } - /* curr_locked is set to -1 until the mutex has been created */ - if (mutex->curr_locked != -1) { + if (!apr_atomic_dec32(&proc_pthread_mutex_refcount(mutex))) { if ((rv = pthread_mutex_destroy(mutex->pthread_interproc))) { #ifdef HAVE_ZOS_PTHREADS rv = errno; @@ -439,6 +456,20 @@ static const apr_proc_mutex_unix_lock_methods_t mu return rv; } } + return APR_SUCCESS; +} + +static apr_status_t proc_mutex_proc_pthread_cleanup(void *mutex_) +{ + apr_proc_mutex_t *mutex=mutex_; + apr_status_t rv; + + /* curr_locked is set to -1 until the mutex has been created */ + if (mutex->curr_locked != -1) { + if ((rv = proc_pthread_mutex_unref(mutex))) { + return rv; + } + } if (munmap((caddr_t)mutex->pthread_interproc, sizeof(pthread_mutex_t))) { return errno; } @@ -459,7 +490,7 @@ static apr_status_t proc_mutex_proc_pthread_create new_mutex->pthread_interproc = (pthread_mutex_t *)mmap( (caddr_t) 0, - sizeof(pthread_mutex_t), + sizeof(proc_pthread_mutex_t), PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); if (new_mutex->pthread_interproc == (pthread_mutex_t *) (caddr_t) -1) { @@ -515,6 +546,7 @@ static apr_status_t proc_mutex_proc_pthread_create return rv; } + proc_pthread_mutex_refcount(new_mutex) = 1; /* first/parent reference */ new_mutex->curr_locked = 0; /* mutex created now */ if ((rv = pthread_mutexattr_destroy(&mattr))) { @@ -532,6 +564,17 @@ static apr_status_t proc_mutex_proc_pthread_create return APR_SUCCESS; } +static apr_status_t proc_mutex_proc_pthread_child_init(apr_proc_mutex_t **mutex, + apr_pool_t *pool, + const char *fname) +{ + (*mutex)->curr_locked = 0; + apr_atomic_inc32(&proc_pthread_mutex_refcount(*mutex)); + apr_pool_cleanup_register(pool, *mutex, proc_pthread_mutex_unref, + apr_pool_cleanup_null); + return APR_SUCCESS; +} + static apr_status_t proc_mutex_proc_pthread_acquire(apr_proc_mutex_t *mutex) { apr_status_t rv; @@ -648,7 +691,7 @@ static const apr_proc_mutex_unix_lock_methods_t mu proc_mutex_proc_pthread_timedacquire, proc_mutex_proc_pthread_release, proc_mutex_proc_pthread_cleanup, - proc_mutex_no_child_init, + proc_mutex_proc_pthread_child_init, proc_mutex_no_perms_set, "pthread" };