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.
-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 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);
