From 6e1ea21501eac981204c3cc8212d45998f74983c Mon Sep 17 00:00:00 2001 From: Carlos O'Donell Date: Thu, 28 Sep 2017 11:05:18 -0600 Subject: [PATCH] malloc: Fix tcache leak after thread destruction [BZ #22111] The malloc tcache added in 2.26 will leak all of the elements remaining in the cache and the cache structure itself when a thread exits. The defect is that we do not set tcache_shutting_down early enough, and the thread simply recreates the tcache and places the elements back onto a new tcache which is subsequently lost as the thread exits (unfreed memory). The fix is relatively simple, move the setting of tcache_shutting_down earlier in tcache_thread_freeres. We add a test case which uses mallinfo and some heuristics to look for unaccounted for memory usage between the start and end of a thread start/join loop. It is very reliable at detecting that there is a leak given the number of iterations. Without the fix the test will consume 122MiB of leaked memory. (cherry picked from commit 1e26d35193efbb29239c710a4c46a64708643320) --- ChangeLog | 9 +++ malloc/Makefile | 3 + malloc/malloc.c | 8 ++- malloc/tst-malloc-tcache-leak.c | 112 ++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 malloc/tst-malloc-tcache-leak.c diff --git a/ChangeLog b/ChangeLog index d7a185e99d..bfcdc1ebbf 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2017-10-06 Carlos O'Donell + + [BZ #22111] + * malloc/malloc.c (tcache_shutting_down): Use bool type. + (tcache_thread_freeres): Set tcache_shutting_down before + freeing the tcache. + * malloc/Makefile (tests): Add tst-malloc-tcache-leak. + * malloc/tst-malloc-tcache-leak.c: New file. + 2017-10-04 H.J. Lu * math/test-math-iscanonical.cc (error): Replace bool with int. diff --git a/malloc/Makefile b/malloc/Makefile index 3fa395b949..9e23db9343 100644 --- a/malloc/Makefile +++ b/malloc/Makefile @@ -34,6 +34,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \ tst-interpose-nothread \ tst-interpose-thread \ tst-alloc_buffer \ + tst-malloc-tcache-leak \ tests-static := \ tst-interpose-static-nothread \ @@ -242,3 +243,5 @@ tst-dynarray-fail-ENV = MALLOC_TRACE=$(objpfx)tst-dynarray-fail.mtrace $(objpfx)tst-dynarray-fail-mem.out: $(objpfx)tst-dynarray-fail.out $(common-objpfx)malloc/mtrace $(objpfx)tst-dynarray-fail.mtrace > $@; \ $(evaluate-test) + +$(objpfx)tst-malloc-tcache-leak: $(shared-thread-library) diff --git a/malloc/malloc.c b/malloc/malloc.c index e3ff778113..01ec1571b9 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -2952,7 +2952,7 @@ typedef struct tcache_perthread_struct tcache_entry *entries[TCACHE_MAX_BINS]; } tcache_perthread_struct; -static __thread char tcache_shutting_down = 0; +static __thread bool tcache_shutting_down = false; static __thread tcache_perthread_struct *tcache = NULL; /* Caller must ensure that we know tc_idx is valid and there's room @@ -2989,8 +2989,12 @@ tcache_thread_freeres (void) if (!tcache) return; + /* Disable the tcache and prevent it from being reinitialized. */ tcache = NULL; + tcache_shutting_down = true; + /* Free all of the entries and the tcache itself back to the arena + heap for coalescing. */ for (i = 0; i < TCACHE_MAX_BINS; ++i) { while (tcache_tmp->entries[i]) @@ -3002,8 +3006,6 @@ tcache_thread_freeres (void) } __libc_free (tcache_tmp); - - tcache_shutting_down = 1; } text_set_element (__libc_thread_subfreeres, tcache_thread_freeres); diff --git a/malloc/tst-malloc-tcache-leak.c b/malloc/tst-malloc-tcache-leak.c new file mode 100644 index 0000000000..22c679b65b --- /dev/null +++ b/malloc/tst-malloc-tcache-leak.c @@ -0,0 +1,112 @@ +/* Bug 22111: Test that threads do not leak their per thread cache. + Copyright (C) 2015-2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* The point of this test is to start and exit a large number of + threads, while at the same time looking to see if the used + memory grows with each round of threads run. If the memory + grows above some linear bound we declare the test failed and + that the malloc implementation is leaking memory with each + thread. This is a good indicator that the thread local cache + is leaking chunks. */ + +#include +#include +#include +#include +#include + +#include +#include +#include + +void * +worker (void *data) +{ + void *ret; + /* Allocate an arbitrary amount of memory that is known to fit into + the thread local cache (tcache). If we have at least 64 bins + (default e.g. TCACHE_MAX_BINS) we should be able to allocate 32 + bytes and force malloc to fill the tcache. We are assuming tcahce + init happens at the first small alloc, but it might in the future + be deferred to some other point. Therefore to future proof this + test we include a full alloc/free/alloc cycle for the thread. We + need a compiler barrier to avoid the removal of the useless + alloc/free. We send some memory back to main to have the memory + freed after the thread dies, as just another check that the chunks + that were previously in the tcache are still OK to free after + thread death. */ + ret = xmalloc (32); + __asm__ volatile ("" ::: "memory"); + free (ret); + return (void *) xmalloc (32); +} + +static int +do_test (void) +{ + pthread_t *thread; + struct mallinfo info_before, info_after; + void *retval; + + /* This is an arbitrary choice. We choose a total of THREADS + threads created and joined. This gives us enough iterations to + show a leak. */ + int threads = 100000; + + /* Avoid there being 0 malloc'd data at this point by allocating the + pthread_t required to run the test. */ + thread = (pthread_t *) xcalloc (1, sizeof (pthread_t)); + + info_before = mallinfo (); + + assert (info_before.uordblks != 0); + + printf ("INFO: %d (bytes) are in use before starting threads.\n", + info_before.uordblks); + + for (int loop = 0; loop < threads; loop++) + { + *thread = xpthread_create (NULL, worker, NULL); + retval = xpthread_join (*thread); + free (retval); + } + + info_after = mallinfo (); + printf ("INFO: %d (bytes) are in use after all threads joined.\n", + info_after.uordblks); + + /* We need to compare the memory in use before and the memory in use + after starting and joining THREADS threads. We almost always grow + memory slightly, but not much. Consider that if even 1-byte leaked + per thread we'd have THREADS bytes of additional memory, and in + general the in-use at the start of main is quite low. We will + always leak a full malloc chunk, and never just 1-byte, therefore + anything above "+ threads" from the start (constant offset) is a + leak. Obviously this assumes no thread-related malloc'd internal + libc data structures persist beyond the thread death, and any that + did would limit the number of times you could call pthread_create, + which is a QoI we'd want to detect and fix. */ + if (info_after.uordblks > (info_before.uordblks + threads)) + FAIL_EXIT1 ("Memory usage after threads is too high.\n"); + + /* Did not detect excessive memory usage. */ + free (thread); + exit (0); +} + +#include