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"
};
