On Tue, Dec 29, 2020 at 01:42:57PM +0100, Otto Moerbeek wrote: > On Tue, Dec 29, 2020 at 12:46:54PM +0100, Mark Kettenis wrote: > > > > Date: Tue, 29 Dec 2020 12:21:25 +0100 > > > From: Otto Moerbeek <o...@drijf.net> > > > > > > Hi, > > > > > > this is a continuation from the threads on bugs@ > > > > > > This version makes it explicit to *only* setup "TLS" (which actually > > > is just a pointer to static data) with the data provided if we're > > > running single threaded (ie.. no -pthread or -pthread but no pthread > > > function that triggers multi-threaded init called yet). In all other > > > cases the real allocated TLS is zero'ed. This avoids the setup issue > > > asr is having. This diff also makes sure asr uses a specialized > > > destructor for it's TLS to kill a mem leak that occurs on thread > > > destruction. > > > > > > All in all I'm still a bit fond of the constructor approach to > > > librthread init: it makes the distinction between having a single > > > thread and being the main thread in a multi-threaded setup moot. > > > > > > Anyway, we can consider that later, please test this. > > > > This isn't right. The memcpy() call you're removing is there such > > that the contents of the per-thread storage for the initial (main) > > thread remains the same. > > > > With your change it reverts back to zero when the first thread is > > created. In the particular case at hand that means the initial thread > > would get a fresh resolver context at that point. > > Yes, but if I do not that crashes happen on destruct. Note that your > initial suggestion (in asr.c) > > if (priv != &_asr && *priv == _asr) > *priv = NULL; > > has the same property: it clears the contents if they happen to be the > same as _asr. > > The crash happens if the pre-mt (static) and the TLS of the main > thread share the same data. I did not find a way to make that work, so > I decided to use the big hammer. I might be overlooking something > obvious though...
This workds better, checking the flags does not work if the thread is already on the road to desctruction. -Otto Index: asr/asr.c =================================================================== RCS file: /cvs/src/lib/libc/asr/asr.c,v retrieving revision 1.64 diff -u -p -r1.64 asr.c --- asr/asr.c 6 Jul 2020 13:33:05 -0000 1.64 +++ asr/asr.c 29 Dec 2020 15:05:45 -0000 @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg) _asr_ctx_unref(ac); return; } else { - priv = _THREAD_PRIVATE(_asr, _asr, &_asr); + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr); if (*priv == NULL) return; asr = *priv; @@ -128,6 +128,23 @@ _asr_resolver_done(void *arg) free(asr); } +static void +_asr_resolver_done_tp(void *arg) +{ + char buf[100]; + int len; + struct asr **priv = arg; + struct asr *asr; + + if (*priv == NULL) + return; + asr = *priv; + + _asr_ctx_unref(asr->a_ctx); + free(asr); + free(priv); +} + void * asr_resolver_from_string(const char *str) { @@ -349,7 +366,8 @@ _asr_use_resolver(void *arg) } else { DPRINT("using thread-local resolver\n"); - priv = _THREAD_PRIVATE(_asr, _asr, &_asr); + priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp, + &_asr); if (*priv == NULL) { DPRINT("setting up thread-local resolver\n"); *priv = _asr_resolver(); Index: include/thread_private.h =================================================================== RCS file: /cvs/src/lib/libc/include/thread_private.h,v retrieving revision 1.35 diff -u -p -r1.35 thread_private.h --- include/thread_private.h 13 Feb 2019 13:22:14 -0000 1.35 +++ include/thread_private.h 29 Dec 2020 15:05:45 -0000 @@ -98,7 +98,8 @@ struct thread_callbacks { void (*tc_mutex_destroy)(void **); void (*tc_tag_lock)(void **); void (*tc_tag_unlock)(void **); - void *(*tc_tag_storage)(void **, void *, size_t, void *); + void *(*tc_tag_storage)(void **, void *, size_t, void (*)(void *), + void *); __pid_t (*tc_fork)(void); __pid_t (*tc_vfork)(void); void (*tc_thread_release)(struct pthread *); @@ -142,6 +143,7 @@ __END_HIDDEN_DECLS #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0) #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0) #define _THREAD_PRIVATE(keyname, storage, error) &(storage) +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error) &(storage) #define _MUTEX_LOCK(mutex) do {} while (0) #define _MUTEX_UNLOCK(mutex) do {} while (0) #define _MUTEX_DESTROY(mutex) do {} while (0) @@ -168,7 +170,12 @@ __END_HIDDEN_DECLS #define _THREAD_PRIVATE(keyname, storage, error) \ (_thread_cb.tc_tag_storage == NULL ? &(storage) : \ _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)), \ - &(storage), sizeof(storage), error)) + &(storage), sizeof(storage), NULL, (error))) + +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error) \ + (_thread_cb.tc_tag_storage == NULL ? &(storage) : \ + _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)), \ + &(storage), sizeof(storage), (dt), (error))) /* * Macros used in libc to access mutexes. Index: thread/rthread_cb.h =================================================================== RCS file: /cvs/src/lib/libc/thread/rthread_cb.h,v retrieving revision 1.2 diff -u -p -r1.2 rthread_cb.h --- thread/rthread_cb.h 5 Sep 2017 02:40:54 -0000 1.2 +++ thread/rthread_cb.h 29 Dec 2020 15:05:45 -0000 @@ -35,5 +35,5 @@ void _thread_mutex_unlock(void **); void _thread_mutex_destroy(void **); void _thread_tag_lock(void **); void _thread_tag_unlock(void **); -void *_thread_tag_storage(void **, void *, size_t, void *); +void *_thread_tag_storage(void **, void *, size_t, void (*)(void*), void *); __END_HIDDEN_DECLS Index: thread/rthread_libc.c =================================================================== RCS file: /cvs/src/lib/libc/thread/rthread_libc.c,v retrieving revision 1.3 diff -u -p -r1.3 rthread_libc.c --- thread/rthread_libc.c 10 Jan 2019 18:45:33 -0000 1.3 +++ thread/rthread_libc.c 29 Dec 2020 15:05:45 -0000 @@ -5,6 +5,8 @@ #include <pthread.h> #include <stdlib.h> #include <string.h> +#include <tib.h> +#include <unistd.h> #include "rthread.h" #include "rthread_cb.h" @@ -31,7 +33,7 @@ static pthread_mutex_t _thread_tag_mutex * This function will never return NULL. */ static void -_thread_tag_init(void **tag) +_thread_tag_init(void **tag, void (*dt)(void *)) { struct _thread_tag *tt; int result; @@ -42,7 +44,8 @@ _thread_tag_init(void **tag) tt = malloc(sizeof *tt); if (tt != NULL) { result = pthread_mutex_init(&tt->m, NULL); - result |= pthread_key_create(&tt->k, free); + result |= pthread_key_create(&tt->k, dt ? dt : + free); *tag = tt; } } @@ -62,7 +65,7 @@ _thread_tag_lock(void **tag) if (__isthreaded) { if (*tag == NULL) - _thread_tag_init(tag); + _thread_tag_init(tag, NULL); tt = *tag; if (pthread_mutex_lock(&tt->m) != 0) _rthread_debug(1, "tag mutex lock failure"); @@ -79,7 +82,7 @@ _thread_tag_unlock(void **tag) if (__isthreaded) { if (*tag == NULL) - _thread_tag_init(tag); + _thread_tag_init(tag, NULL); tt = *tag; if (pthread_mutex_unlock(&tt->m) != 0) _rthread_debug(1, "tag mutex unlock failure"); @@ -88,28 +91,33 @@ _thread_tag_unlock(void **tag) /* * return the thread specific data for the given tag. If there - * is no data for this thread initialize it from 'storage'. + * is no data for this thread allocate and initialize it from 'storage' + * or clear it for non-main threads. * On any error return 'err'. */ void * -_thread_tag_storage(void **tag, void *storage, size_t sz, void *err) +_thread_tag_storage(void **tag, void * storage, size_t sz, void (*dt)(void *), + void *err) { struct _thread_tag *tt; void *ret; if (*tag == NULL) - _thread_tag_init(tag); + _thread_tag_init(tag, dt); tt = *tag; ret = pthread_getspecific(tt->k); if (ret == NULL) { - ret = malloc(sz); + ret = calloc(1, sz); if (ret == NULL) ret = err; else { - if (pthread_setspecific(tt->k, ret) == 0) - memcpy(ret, storage, sz); - else { + int len; + char buf[100]; + if (pthread_setspecific(tt->k, ret) == 0) { + if (pthread_self() == &_initial_thread) + memcpy(ret, storage, sz); + } else { free(ret); ret = err; }