On Fri, Dec 25, 2020 at 02:04:03PM +0100, Otto Moerbeek wrote:
> On Fri, Dec 25, 2020 at 12:59:10PM +0100, Otto Moerbeek wrote:
>
> > 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 <o...@drijf.net>
> > > >
> > > > 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?
>
> Using procmap I'm seeing single page anon regions being added to the process.
> A suspect would be uvm_uarea_alloc() in thread_fork.
Added some debug code and uvm_uarea_free() is called on the thread (in
the repaer) shortly after the thread exists. So that part seems to be
ok. Puzzled what other mapping could show up in the process when
creating/destroying threads.
-Otto
