From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Fri, 10 Nov 2017 14:44:58 +0000 (+0100)
Subject: basic/hashmap: add cleanup of memory pools (#7164)
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?a=commitdiff_plain;h=cd30037fde29c36430cce29c8c82c70bb7c7abd2;p=elogind.git

basic/hashmap: add cleanup of memory pools (#7164)

It was dropped in 89439d4fc0d29f04ac68432fd06ab84bc4e36e20. As a result, every
process that uses a hashmap allocates and then leaks the hashmap mempools.
The mempools are only allocated in the main thread, but we don't know where
the memory is used.

So let's check if we are the last thread and free the mempools then. This is
fairly heavy, because /proc/self/status has to be opened and parsed, but we do
it only when compiled for valgrind, i.e. not by default, and compared to running
under valgrind or asan, the extra cost is acceptable. The big advantage is that
we don't have to think or filter out this false positive.

As a micro-opt, cleanup is attempted only in the main thread. We could allow
any thread to check if it is the last one and perform cleanup, but that'd mean
that we'd have to _do_ the check in every thread. We don't use threads like
that, our non-main threads are always short-lived, so let's just accept the
possibility that we'll leak memory if a thread survives. The check is also
non-atomic, but it's called in a destructor of the main thread _and_ we do
cleanup only when there are no other threads, so the risk of some library
suddenly spawning another thread is very low. All in all, this is not perfect,
but should work in 999‰ of cases.

Fixes the following valgrind warning:

==22564== HEAP SUMMARY:
==22564==     in use at exit: 8,192 bytes in 2 blocks
==22564==   total heap usage: 243 allocs, 241 frees, 151,905 bytes allocated
==22564==
==22564== 4,096 bytes in 1 blocks are still reachable in loss record 1 of 2
==22564==    at 0x4C2FB6B: malloc (vg_replace_malloc.c:299)
==22564==    by 0x4F08A8C: mempool_alloc_tile (mempool.c:62)
==22564==    by 0x4F08B16: mempool_alloc0_tile (mempool.c:81)
==22564==    by 0x4EF8DE0: hashmap_base_new (hashmap.c:748)
==22564==    by 0x4EF8ED9: internal_hashmap_new (hashmap.c:782)
==22564==    by 0x11045D: test_hashmap_copy (test-hashmap-plain.c:87)
==22564==    by 0x115722: test_hashmap_funcs (test-hashmap-plain.c:914)
==22564==    by 0x10FC9D: main (test-hashmap.c:60)
==22564==
==22564== 4,096 bytes in 1 blocks are still reachable in loss record 2 of 2
==22564==    at 0x4C2FB6B: malloc (vg_replace_malloc.c:299)
==22564==    by 0x4F08A8C: mempool_alloc_tile (mempool.c:62)
==22564==    by 0x4F08B16: mempool_alloc0_tile (mempool.c:81)
==22564==    by 0x4EF8DE0: hashmap_base_new (hashmap.c:748)
==22564==    by 0x4EF8EF8: internal_ordered_hashmap_new (hashmap.c:786)
==22564==    by 0x10A2A0: test_ordered_hashmap_copy (test-hashmap-ordered.c:89)
==22564==    by 0x10F70F: test_ordered_hashmap_funcs (test-hashmap-ordered.c:916)
==22564==    by 0x10FCA2: main (test-hashmap.c:61)
==22564==
==22564== LEAK SUMMARY:
==22564==    definitely lost: 0 bytes in 0 blocks
==22564==    indirectly lost: 0 bytes in 0 blocks
==22564==      possibly lost: 0 bytes in 0 blocks
==22564==    still reachable: 8,192 bytes in 2 blocks
==22564==         suppressed: 0 bytes in 0 blocks

v2:
- check if we are the main thread

v3:
- check if there are no other threads
---

diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c
index bcef42e11..b303fe74f 100644
--- a/src/basic/hashmap.c
+++ b/src/basic/hashmap.c
@@ -25,12 +25,14 @@
 
 #include "alloc-util.h"
 #include "hashmap.h"
+#include "fileio.h"
 #include "macro.h"
 #include "mempool.h"
 #include "process-util.h"
 #include "random-util.h"
 #include "set.h"
 #include "siphash24.h"
+#include "string-util.h"
 #include "strv.h"
 #include "util.h"
 
@@ -278,6 +280,28 @@ static const struct hashmap_type_info hashmap_type_info[_HASHMAP_TYPE_MAX] = {
         },
 };
 
+#ifdef VALGRIND
+__attribute__((destructor)) static void cleanup_pools(void) {
+        _cleanup_free_ char *t = NULL;
+        int r;
+
+        /* Be nice to valgrind */
+
+        /* The pool is only allocated by the main thread, but the memory can
+         * be passed to other threads. Let's clean up if we are the main thread
+         * and no other threads are live. */
+        if (!is_main_thread())
+                return;
+
+        r = get_proc_field("/proc/self/status", "Threads", WHITESPACE, &t);
+        if (r < 0 || !streq(t, "1"))
+                return;
+
+        mempool_drop(&hashmap_pool);
+        mempool_drop(&ordered_hashmap_pool);
+}
+#endif
+
 static unsigned n_buckets(HashmapBase *h) {
         return h->has_indirect ? h->indirect.n_buckets
                                : hashmap_type_info[h->type].n_direct_buckets;