|
Message-ID: <20221011175046.GH29905@brightrain.aerifal.cx> Date: Tue, 11 Oct 2022 13:50:46 -0400 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Subject: Re: MT fork and key_lock in pthread_key_create.c On Sat, Oct 08, 2022 at 08:03:24PM +0300, Alexey Izbyshev wrote: > On 2022-10-08 04:36, Rich Felker wrote: > >On Thu, Oct 06, 2022 at 02:21:51PM -0400, Rich Felker wrote: > >>On Thu, Oct 06, 2022 at 09:37:50AM +0300, Alexey Izbyshev wrote: > >>> Hi, > >>> > >>> I noticed that fork() doesn't take key_lock that is used to protect > >>> the global table of thread-specific keys. I couldn't find mentions > >>> of this lock in the MT fork discussion in the mailing list archive. > >>> Was this lock overlooked? > >> > >>I think what happened was that we made the main list of locks to > >>review and take care of via grep for LOCK, and then manually added > >>known instances of locks using other locking primitives. This one must > >>have been missed. > >> > >>Having special-case lock types like this is kinda a pain, but as long > >>as there aren't too many I guess it's not a big deal. > > > >Proposed patch attached. > > diff --git a/src/internal/fork_impl.h b/src/internal/fork_impl.h > index ae3a79e5..354e733b 100644 > --- a/src/internal/fork_impl.h > +++ b/src/internal/fork_impl.h > @@ -16,3 +16,4 @@ extern hidden volatile int *const __vmlock_lockptr; > > hidden void __malloc_atfork(int); > hidden void __ldso_atfork(int); > +hidden void __pthread_key_atfork(int); > diff --git a/src/process/fork.c b/src/process/fork.c > index 80e804b1..56f19313 100644 > --- a/src/process/fork.c > +++ b/src/process/fork.c > @@ -37,6 +37,7 @@ static void dummy(int x) { } > weak_alias(dummy, __fork_handler); > weak_alias(dummy, __malloc_atfork); > weak_alias(dummy, __aio_atfork); > +weak_alias(dummy, __pthread_key_atfork); > weak_alias(dummy, __ldso_atfork); > > static void dummy_0(void) { } > @@ -51,6 +52,7 @@ pid_t fork(void) > int need_locks = libc.need_locks > 0; > if (need_locks) { > __ldso_atfork(-1); > + __pthread_key_atfork(-1); > __aio_atfork(-1); > __inhibit_ptc(); > for (int i=0; i<sizeof atfork_locks/sizeof *atfork_locks; i++) > @@ -78,6 +80,7 @@ pid_t fork(void) > else **atfork_locks[i] = 0; > __release_ptc(); > if (ret) __aio_atfork(0); > + __pthread_key_atfork(!ret); > __ldso_atfork(!ret); > } > __restore_sigs(&set); > diff --git a/src/thread/pthread_key_create.c > b/src/thread/pthread_key_create.c > index d1120941..39770c7a 100644 > --- a/src/thread/pthread_key_create.c > +++ b/src/thread/pthread_key_create.c > @@ -1,4 +1,5 @@ > #include "pthread_impl.h" > +#include "fork_impl.h" > > volatile size_t __pthread_tsd_size = sizeof(void *) * PTHREAD_KEYS_MAX; > void *__pthread_tsd_main[PTHREAD_KEYS_MAX] = { 0 }; > @@ -20,6 +21,13 @@ static void dummy_0(void) > weak_alias(dummy_0, __tl_lock); > weak_alias(dummy_0, __tl_unlock); > > +void __pthread_key_atfork(int who) > +{ > + if (who<0) __pthread_rwlock_rdlock(&key_lock); > + else if (!who) __pthread_rwlock_unlock(&key_lock); > + else key_lock = (pthread_rwlock_t)PTHREAD_RWLOCK_INITIALIZER; > > Are you using PTHREAD_RWLOCK_INITIALIZER to avoid dependency on > pthread_rwlock_init (which is used in the aio_atfork patch[1])? Yes. See commit 639bcf251e549f634da9a3e7ef8528eb2ec12505. We could add an additional namespace-safe version of pthread_rwlock_init, but it didn't seem worthwhile when we can just do it inline like this. This would not be acceptable portable POSIX code in general, but within musl, I think it's perfectly reasonable to assume that there's no magic going on in the initializers and that the assignment works as intended. It's not like pthread_rwlock_init is portable here itself, anyway; it's possible as an implementation choice for example that all live rwlocks are entered into some list, and that clobbering one this way corrupts the list. > +} > + > int __pthread_key_create(pthread_key_t *k, void (*dtor)(void *)) > { > pthread_t self = __pthread_self(); > > Looks good to me otherwise. Thanks for looking it over. Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.