On Fri, Dec 25, 2020 at 12:35:57PM +0100, Mark Kettenis wrote:
> > Date: Fri, 25 Dec 2020 11:34:47 +0100
> > From: Otto Moerbeek <[email protected]>
> >
> > On Thu, Dec 24, 2020 at 01:29:28PM +0100, Uli Schlachter wrote:
> >
> > > Hi,
> > >
> > > due to that other thread, it occurred to me that getaddrinfo() also has
> > > another bug: It leaks memory. _asr_use_resolver() allocates memory
> > > per-thread (via _asr_resolver()) and saves it via _THREAD_PRIVATE() in
> > > _asr, but nothing frees that memory. A reproducer follows bellow. On
> > > Debian, no memory leak is observed (= RES in top stays constant over
> > > time).
> > >
> > > I have no good suggestion for how to fix this leak, but I feel like this
> > > might also be helpful in fixing the thread unsafety from "that other
> > > thread". Both bugs originate from storing a pointer to an allocation via
> > > _THREAD_PRIVATE(), which is something that does not really work with
> > > that API.
> > >
> > > IMHO this internal API needs to change. At this point, one can also fix
> > > the other problem by having an API that guarantees that each thread gets
> > > zeroed per-thread data instead of memcpy()ing from a global default.
> > >
> > > Other users of _THREAD_PRIVATE() instead seem to only store buffers,
> > > e.g. strerror_l() or localtime(). These buffers do not need extra cleanup.
> > >
> > > Reproducer (sorry for the line wrapping; this is basically just the
> > > previous example, but without calling getaddrinfo() on the main thread:
> > > lots of threads are started and each thread calls getaddrinfo() once):
> > >
> > > #include <pthread.h>
> > > #include <sys/types.h>
> > > #include <sys/socket.h>
> > > #include <netdb.h>
> > > #include <stdio.h>
> > > #include <string.h>
> > >
> > > #define NUM_THREADS 50
> > >
> > > static void do_lookup(const char *host)
> > > {
> > > int s;
> > > struct addrinfo hints;
> > > struct addrinfo *result;
> > >
> > > memset(&hints, 0, sizeof(hints));
> > > hints.ai_family = AF_UNSPEC;
> > > hints.ai_socktype = SOCK_STREAM;
> > > hints.ai_flags = AI_ADDRCONFIG;
> > > hints.ai_protocol = IPPROTO_TCP;
> > >
> > > s = getaddrinfo(host, NULL, &hints, &result);
> > > if (s != 0) {
> > > fprintf(stderr, "Lookup error for %s: %s\n", host,
> > > gai_strerror(s));
> > > } else {
> > > freeaddrinfo(result);
> > > }
> > > }
> > >
> > > static void *
> > > do_things(void *arg)
> > > {
> > > (void) arg;
> > > do_lookup("ipv4.google.com");
> > > return NULL;
> > > }
> > >
> > > int main()
> > > {
> > > pthread_t threads[NUM_THREADS];
> > > int i;
> > > int s;
> > >
> > > for (;;) {
> > > for (i = 0; i < NUM_THREADS; i++) {
> > > s = pthread_create(&threads[i], NULL, do_things, NULL);
> > > if (s != 0)
> > > fprintf(stderr, "Error creating thread");
> > > }
> > > for (i = 0; i < NUM_THREADS; i++) {
> > > pthread_join(threads[i], NULL);
> > > }
> > > }
> > > return 0;
> > > }
> > >
> > > Cheers,
> > > Uli
> > > --
> > > This can be a, a little complicated. Listen, my advice is... ask
> > > somebody else for advice, at least someone who's... got more experience
> > > at... giving advice.
> > >
> >
> > Hoi,
> >
> > the diff (which is certainly wip) below fixes the asr related leaks.
> > There's still a leak on creating/destroying threads, in particular the
> > stacks do not seem to be unmapped.
>
> That is (somewhat) expected. Stacks are recycled if they are
> allocated using "default" parameters; see _rthread_free_stack().
yes, agreed, they do seem to be reused. I was looking with valgrind,
and this seem to inrtroduce a bug where the stack reuse is effectively
disabled. So this observation was a fluke.
Still seeing growing mem usage when I create and destroy threads,
though. Strange thing is that while top show increasing SIZE and RES,
ktrace shows no syscall that do allocations, just repeating
67230/276969 a.out RET __tfork 445287/0x6cb67
67230/614016 a.out RET futex 0
67230/521766 a.out CALL __threxit(0xfe46b76aa30)
67230/276969 a.out CALL __tfork(0x7f7ffffc5e80,24)
67230/614016 a.out CALL __threxit(0xfe424f44230)
67230/276969 a.out STRU struct __tfork { tcb=0xfe424f44400,
tid=0xfe424f44430, stack=0xfe3eed35630 }
67230/445287 a.out RET __tfork 0
67230/276969 a.out RET __tfork 118126/0x1cd6e
67230/445287 a.out CALL futex(0xfe3fa8d7448,0x2<FUTEX_WAKE>,1,0,0)
67230/276969 a.out CALL __tfork(0x7f7ffffc5e80,24)
67230/118126 a.out RET __tfork 0
67230/445287 a.out RET futex 0
67230/276969 a.out STRU struct __tfork { tcb=0xfe3fa8d7200,
tid=0xfe3fa8d7230, stack=0xfe403a719b0 }
67230/118126 a.out CALL futex(0xfe424f44448,0x2<FUTEX_WAKE>,1,0,0)
67230/445287 a.out CALL __threxit(0xfe3fa8d7430)
67230/276969 a.out RET __tfork 456926/0x6f8de
Could this indicate that one of these syscalls introduces a mem leak
in the process?
-Otto
#include <pthread.h>
#include <sys/types.h>
#include <stdio.h>
#include <string.h>
#define NUM_THREADS 50
static void *
noop(void *a)
{
return NULL;
}
int main()
{
pthread_t threads[NUM_THREADS];
int i, j;
int s;
for (i = 0; i < 10000; i++) {
for (j = 0; j < NUM_THREADS; j++) {
s = pthread_create(&threads[j], NULL, noop, NULL);
if (s != 0)
fprintf(stderr, "Error creating thread");
}
for (j = 0; j < NUM_THREADS; j++) {
s = pthread_join(threads[j], NULL);
if (s != 0)
fprintf(stderr, "Error joining thread");
}
}
return 0;
}
>
> I think changing the tc_tag_storage signature is safe, since it is
> always provided by libc and only used only used internally in libc.
>
> > 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 25 Dec 2020 09:09:26 -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,21 @@ _asr_resolver_done(void *arg)
> > free(asr);
> > }
> >
> > +static void
> > +_asr_resolver_done_tp(void *arg)
> > +{
> > + 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 +364,10 @@ _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 != &_asr && *priv == _asr)
> > + *priv = NULL;
> > if (*priv == NULL) {
> > DPRINT("setting up thread-local resolver\n");
> > *priv = _asr_resolver();
> > Index: asr/asr_private.h
> > ===================================================================
> > RCS file: /cvs/src/lib/libc/asr/asr_private.h,v
> > retrieving revision 1.47
> > diff -u -p -r1.47 asr_private.h
> > --- asr/asr_private.h 28 Apr 2018 15:16:49 -0000 1.47
> > +++ asr/asr_private.h 25 Dec 2020 09:09:26 -0000
> > @@ -319,6 +319,8 @@ struct asr_query *_gethostbyaddr_async_c
> >
> > int _asr_iter_domain(struct asr_query *, const char *, char *, size_t);
> >
> > +#define DEBUG
> > +
> > #ifdef DEBUG
> >
> > #define DPRINT(...) do { if(_asr_debug) { \
> > 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 25 Dec 2020 09:09:26 -0000
> > @@ -98,7 +98,7 @@ 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 +142,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 +169,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 25 Dec 2020 09:09:26 -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 25 Dec 2020 09:09:26 -0000
> > @@ -31,7 +31,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 +42,7 @@ _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 +62,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 +79,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");
> > @@ -92,13 +92,13 @@ _thread_tag_unlock(void **tag)
> > * 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);
> >
> >
>
