On Sun, May 02, 2021 at 02:07:21PM +0200, Mark Kettenis wrote:
> > 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.
Agreed. This is a one-time leak, since once_control must not be on
the stack. So not a big issue. I would love to see malloc-free mutexes
as well.
-Otto
>
> > 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;
> > }
> >
> >
> >
>
