> From: Martijn van Duren <openbsd+t...@list.imperialat.at>
> Date: Sun, 02 May 2021 13:28:10 +0200
>
> Found this while tracing a memory leak in filter-dkimsign, thanks to
> libcrypto. The mutex in pthread_once_t is never destroyed, so the
> memory allocated inside the mutex is never released.
>
> The diff below was inspired by Ed Schouten and switches form mutex to
> futex to prevent any memory allocation. I've run with it for about a
> week without issues and tb@ has given it some beating on sparc64.
> However I'm no expert in this area and scrutiny from people with more
> experience in this area and testing in general would be appreciated.
>
> This implementation has one shortcoming I can see, namely[0]:
> The pthread_once() function is not a cancellation point. However, if
> init_routine is a cancellation point and is canceled, the effect on
> once_control shall be as if pthread_once() was never called.
> It doesn't handle this situation by waking up the sleeping threads.
> However, the current code doesn't handle this requirement either:
> #include <stdio.h>
> #include <pthread.h>
>
> pthread_once_t once = PTHREAD_ONCE_INIT;
>
> void
> init(void)
> {
> printf("init\n");
> pthread_exit(NULL);
> }
>
> void *
> routine(void *arg)
> {
> pthread_once(&once, init);
> printf("%s\n", __func__);
> return NULL;
> }
>
> int
> main(int argc, char *argv[])
> {
> pthread_t thread;
> pthread_create(&thread, NULL, routine, NULL);
> pthread_once(&once, init);
> printf("%s\n", __func__);
> return 0;
> }
>
> Since our current code shows similar behaviour without real world
> problems and all the solutions that I can come up with are racey I think
> this diff can stand on its own and some other brave soul can fix this
> requirement at a later time. :-)
>
> OK?
Sorry, no, this is an ABI break. And a libpthreads major bump is a
major flag day.
I don't think this is worth fixing on its own. There are other
instances where using a mutex will leak memory. We need to change the
mutex implementation such that it doesn't use malloc. This is needed
for process shared mutexes too.
> Index: include/pthread.h
> ===================================================================
> RCS file: /cvs/src/include/pthread.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 pthread.h
> --- include/pthread.h 5 Mar 2018 01:15:26 -0000 1.4
> +++ include/pthread.h 2 May 2021 11:24:17 -0000
> @@ -136,20 +136,13 @@ typedef void *(*pthread_startroutine_t)(
> * Once definitions.
> */
> struct pthread_once {
> - int state;
> - pthread_mutex_t mutex;
> + volatile unsigned int state;
> };
>
> /*
> - * Flags for once initialization.
> - */
> -#define PTHREAD_NEEDS_INIT 0
> -#define PTHREAD_DONE_INIT 1
> -
> -/*
> * Static once initialization values.
> */
> -#define PTHREAD_ONCE_INIT { PTHREAD_NEEDS_INIT, PTHREAD_MUTEX_INITIALIZER }
> +#define PTHREAD_ONCE_INIT { 0 }
>
> /*
> * Static initialization values.
> Index: lib/libc/thread/rthread_once.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/thread/rthread_once.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 rthread_once.c
> --- lib/libc/thread/rthread_once.c 4 Nov 2017 22:53:57 -0000 1.3
> +++ lib/libc/thread/rthread_once.c 2 May 2021 11:24:17 -0000
> @@ -18,15 +18,25 @@
>
> #include <pthread.h>
>
> +#include "synch.h"
> +
> int
> pthread_once(pthread_once_t *once_control, void (*init_routine)(void))
> {
> - pthread_mutex_lock(&once_control->mutex);
> - if (once_control->state == PTHREAD_NEEDS_INIT) {
> + switch (atomic_cas_uint(&(once_control->state), 0, 1)) {
> + case 0:
> init_routine();
> - once_control->state = PTHREAD_DONE_INIT;
> + atomic_inc_int(&once_control->state);
> + _wake(&once_control->state, INT_MAX);
> + break;
> + case 1:
> + do {
> + _twait(&once_control->state, 1, 0, NULL);
> + } while (once_control->state != 2);
> + break;
> + default:
> + break;
> }
> - pthread_mutex_unlock(&once_control->mutex);
>
> - return (0);
> + return 0;
> }
>
>
>
