From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2 autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 26819 invoked from network); 11 Oct 2022 17:51:01 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 11 Oct 2022 17:51:01 -0000 Received: (qmail 26596 invoked by uid 550); 11 Oct 2022 17:50:51 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 26562 invoked from network); 11 Oct 2022 17:50:50 -0000 Date: Tue, 11 Oct 2022 13:50:46 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20221011175046.GH29905@brightrain.aerifal.cx> References: <033092a85a52d9f8e8d73a235e2381ae@ispras.ru> <20221006182151.GV29905@brightrain.aerifal.cx> <20221008013615.GE29905@brightrain.aerifal.cx> <7c781fb1944200ba72ed381b5635fd31@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7c781fb1944200ba72ed381b5635fd31@ispras.ru> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] 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 @@ -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