> Date: Tue, 29 Dec 2020 16:07:19 +0100
> From: Otto Moerbeek <[email protected]>
>
> 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 <[email protected]>
> > > >
> > > > 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.
That looks better. Minus the obvious debug leftovers ;). But those
shouldn't hurt any testing.
> 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;
> }
>
