From patchwork Fri Oct 6 16:35:18 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Carlos O'Donell X-Patchwork-Id: 23382 Received: (qmail 127610 invoked by alias); 6 Oct 2017 16:35:28 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 127600 invoked by uid 89); 6 Oct 2017 16:35:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy=(unknown) X-HELO: mail-qk0-f172.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language; bh=M4I3qpsnV9isgSkXRda2H8Q9ycw3eUYdLugpp1/bjZA=; b=Bg6iL8aPI2RmRYsLd0rZIhTQM/ZHzpgZmESxKJ6IgBJUPKuoT4GQEHgMF7G2PzOt+s rLMb/4H3po7Svd6V1q5ZQEcQ9Yk29iHAHJGO7zO8crmr2vPjdzegKfE4AQzU1lPeifF8 FVVeytARjdHYjcsn9P4Ct019AAQnxfJqYjMwlCr7Uc65bZLWQBWj6EANBbaYBMwQl5ON 8S9uNKrrsl2+XfFdJ/+R7OGA4+UF82+UbaYFeEJ7OZIRFdf22TO+oliTMTmg7JIzQe2n 058bIUcg4trTSZ5npztuHDJikdmXIxfAguEWNOiQ/horx/k/pqgvi9Vptq1mc/E4Caqq YikQ== X-Gm-Message-State: AMCzsaW5Vs2h9t309vDosBbEYosPJvdOW/6jnpA83Sb0rF5898HF8116 vsmY9kU1KqeqoJf72f56xwg7JJMexZQ= X-Google-Smtp-Source: AOwi7QD5wcx5uZ7KHkUgBPDeCpCkvDtVMOC9qNOZaYiWWmBEZTvoWwMp/+t7omDYNn810XeZcv2r1A== X-Received: by 10.55.82.214 with SMTP id g205mr36454299qkb.191.1507307722435; Fri, 06 Oct 2017 09:35:22 -0700 (PDT) Subject: Re: [PATCH v2] malloc: Fix tcache leak on thread destruction [BZ #22111] To: Andrew Pinski , DJ Delorie Cc: Florian Weimer , GNU C Library References: <53491cd3-a4fa-0af5-96b8-8fb1b61d37b1@redhat.com> From: Carlos O'Donell Message-ID: <48c2d0e6-fc60-55f6-2477-21c0ac08f409@redhat.com> Date: Fri, 6 Oct 2017 09:35:18 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: On 10/02/2017 06:43 PM, Andrew Pinski wrote: > On Mon, Oct 2, 2017 at 6:36 PM, DJ Delorie wrote: >> >> Carlos O'Donell writes: >>> diff --git a/malloc/tst-malloc-tcache-leak.c b/malloc/tst-malloc-tcache-leak.c >>> +void * >>> +worker (void *data) >>> +{ >>> + /* 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 have the allocated >>> + memory escape back to the parent to be freed to avoid any compiler >>> + optimizations. */ >>> + return (void *) xmalloc (32); >>> +} >> >> This would be slightly more future-proof if it did an alloc/free/alloc >> cycle, in case in the future malloc doesn't init the tcache "just >> because". The free would force the init, since the chunk would be >> stored in the tcache. >> >> Actually, a malloc/free might be a better test, since it tests that the >> free'd chunks in the tcache are freed as well as the tcache >> infrastructure itself. Or maybe as a second test. > > And if you are going to do a malloc/free pair, make sure you add a > compiler barrier in the code so the compiler does not delete the > malloc/free pairs (which it does already). Thanks DJ, that's a good idea about alloc/free/alloc. Andrew, Yup, compiler barrier `__asm__ volatile ("" ::: "memory");` v3 patch attached. How does this look? Signed-off-by: Carlos O'Donell Would appreciate any Reviewed-by entries from either of you. From 1e26d35193efbb29239c710a4c46a64708643320 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. --- 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 ac0f188..bbd80b1 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-06 Rajalakshmi Srinivasaraghavan * sysdeps/powerpc/powerpc64/multiarch/memrchr-ppc64.c: Revert diff --git a/malloc/Makefile b/malloc/Makefile index 50b487e..6cf78e1 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 1c2a0b0..d3fcadd 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -2916,7 +2916,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 @@ -2953,8 +2953,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]) @@ -2966,8 +2970,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 0000000..22c679b --- /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 -- 2.9.5