apr icon indicating copy to clipboard operation
apr copied to clipboard

Fix a rare segfault in apr_global_mutex_child_init()

Open monkburger opened this issue 3 years ago • 7 comments

Under specific circumstances, apr_global_mutex_child_init can be called with NULL mutex. We've seen this behavior under certain modules, specifically mod_lsapi and a few others under high load/IO wait during graceful restarts:

  #0  apr_proc_mutex_child_init (mutex=0x8, fname=0x0, pool=0x5566e972f128) at locks/unix/proc_mutex.c:1570
  #1  0x00002b9a5730cd2c in apr_global_mutex_child_init (mutex=<optimized out>, fname=<optimized out>, pool=<optimized out>) at locks/unix/global_mutex.c:89

With this patch, it shows the following - as well as no more segfaults:

[Fri Feb 25 01:13:58.924341 2022] [:emerg] [pid 92020] (20009)No lock was provided and one was required.: [host test.test] mod_lsapi: apr_global_mutex_child_init error for pipe mutex

monkburger avatar Feb 25 '22 07:02 monkburger

Is ignoring these the right answer? Or should we set ourselves in a 5-retry loop? With sleeps?

wrowe avatar Mar 15 '22 15:03 wrowe

Is ignoring these the right answer? Or should we set ourselves in a 5-retry loop? With sleeps?

I was thinking about that, maybe a retry loop (hybrid-ish spinlock?)

monkburger avatar Mar 15 '22 18:03 monkburger

Why is the *mutex NULL in the first place? And why would retrying work?

ylavic avatar Mar 15 '22 18:03 ylavic

Why is the *mutex NULL in the first place? And why would retrying work?

Short of some ACL problem that the process doesn't have permission to create a mutex, this almost always is going to be a resource contention problem, too many mutexes held. Ultimately, if they are 'forgotten' kernel resources, the process (and perhaps the system) is going down in flames, but sometimes, we are just in an overbooked state, and a retry after a significant (1s) pause will be enough for other threads to release some mutexes back to the pool of resources.

wrowe avatar Mar 31 '22 20:03 wrowe

Is ignoring these the right answer? Or should we set ourselves in a 5-retry loop? With sleeps?

I was thinking about that, maybe a retry loop (hybrid-ish spinlock?)

0006-thread_mutex-APR_THREAD_MUTEX_DEFAULT-should-map-to-.patch

I do not have code for other platforms, this patch is a tested (had it on file for quite a while) linux-glibc only approach.

crrodriguez avatar May 02 '23 21:05 crrodriguez

Is ignoring these the right answer? Or should we set ourselves in a 5-retry loop? With sleeps?

I was thinking about that, maybe a retry loop (hybrid-ish spinlock?)

0006-thread_mutex-APR_THREAD_MUTEX_DEFAULT-should-map-to-.patch

I do not have code for other platforms, this patch is a tested (had it on file for quite a while) linux-glibc only approach.

the adaptative_np mutex spins glibc.pthread.mutex_spin_count (tunable at runtime, default 100) times before calling into the kernel..

crrodriguez avatar May 02 '23 21:05 crrodriguez

apr_global_mutex_child_init() is not supposed to be called with *mutex == NULL (it should have been allocated with apr_global_mutex_create() first), so this is a usage/user error (i.e. don't do that)? Also, if something has to be done with the new APR_ENOLOCK error from this PR, it should be possible to do the same by testing *mutex == NULL before in the caller, so I don't get what this PR is fixing, looks like defensive programming only to me.

ylavic avatar May 04 '23 09:05 ylavic