memleak hunting: cleanup based on valgrind report #1649

furiel · 2017-08-21T15:21:56Z

This patchet intends to fix some entries in valgrind report. All of these issues are minors, i.e non of them accumulate over time. All of these changes intended to be cosmetic, explicitely calling deinits/frees, instead of just adding new exceptions to a valgrind suppress file.

Credits for @bkil-syslogng for #975, which gave much inspiration for this pull request.
Only there are two commits in 975 that were not ported. I list them here for documentation reason.

g_process_set_argv_space. There were a lot of fight on these recently. The proposed change is still incomplete, see g_process_finish: reverting attempt to fix memleak #1528 for details.
g_static_mutex_free. Syslog-ng mutexes are typically statically initialized. The glib documentation states that "[...] You don't have to call this functions for a GStaticMutex with an unbounded lifetime, i.e. objects declared 'static', [...]". In the glib implementation, when someone calls lock in the first time on such a static mutex, glib will allocate using malloc on the fly, but there is no automatic deallocation mechanism provided. As that malloc is not freed, valgrind reports it as still reachable memory. The suggested patch calls explicitely g_static_mutex_free-s on these objects. This should work, though I feel this change somewhat risky, because the documentation states we do not have to call such function. There is a small chance (very low, because we are talking about a deprecated mutex api to begin with ...) that some automatic deallocation would be introduced glib at some point. Especially, this leak comes from the design of glib, I don't want to fix it from syslog-ng side. That's why I did not pick that patch.

Edit:
~~The dns_cache part is reported in: #1400~~
dns_cache will be handled in a separate pull request.

kira-syslogng · 2017-08-21T15:37:37Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

bazsi · 2017-09-04T12:27:04Z

lib/dnscache.c

+
+  if (main_loop_is_terminating(main_loop_get_instance()))
+    dns_cache_free(dns_cache);
+  else


hmm I don't really like this coupling between dnscache and mainloop. Why isn't the dns_caching_global_deinit() enough? That should cover the exit case properly.

As mainloop does not wait threads to terminate, it can happen that unused_dns_caches is freed by the mainloop in dns_caching_global_deinit by the time dns_caching_thread_deinit is called. This results in an invalid read in the thread:

unused_dns_caches = g_list_prepend(unused_dns_caches, dns_cache);

as unused_dns_caches is already freed.

The leak here is only a side effect, due that dns_cache is appended to a non existing list.

ugh, I understand. Still I would leave in this as a bug and then fix the termination issue in mainloop perhaps later.

As far as I understand, the mainloop does have a grab on how many threads are executing, and we could wait for that to drop to 0, perhaps even using a timeout, to ensure that it does not take all eternity to exit.

If the timeout is reached, we would exit drastically (e.g. exit() without calling the shutdown functions), but that should only happen in extreme cases.

Ok, fine for me. I drop this patch and try to create a separate one based on your guidance.

bazsi · 2017-09-04T12:34:16Z

lib/gprocess.c

@@ -1465,6 +1465,7 @@ g_process_finish(void)
 #endif

  g_process_remove_pidfile();
+  reloc_deinit(); /* path_cache needed for calculating pidfile location above */
 }


I hate this one too. I understand that the pidfile location is actually managed by reloc, which then can't be deinitialized in app_shutdown() where it should be deinitialized, or otherwise we'd risk use-after-free, but adding this kind of coupling is worse than that minor, one-time memory leak.

I would either:

leave the leak as is

put reloc_deinit() into main()

use some kind of destructor trick to make sure that hash table gets freed

The easiest is probably 2), which plugs the leak and is acceptable code/coupling wise.

Thanks, I'll go with 2).

bazsi · 2017-09-04T12:34:25Z

lib/mainloop.c

@@ -195,7 +195,7 @@ main_loop_initialize_state(GlobalConfig *cfg, const gchar *persist_filename)
  return success;
 }

-static inline gboolean
+gboolean
 main_loop_is_terminating(MainLoop *self)


should not be needed.

bazsi · 2017-09-04T12:35:20Z

lib/stats/stats-control.c

@@ -49,3 +49,11 @@ stats_register_control_commands(void)
  control_register_command("STATS", NULL, control_connection_send_stats, NULL);
  control_register_command("RESET_STATS", NULL, control_connection_reset_stats, NULL);
 }
+
+#include <stdio.h>
+


why the stdio.h inclusion?

bazsi

I had a few comments that should be changed/dropped but the rest of it is good stuff that should go in.

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

kira-syslogng · 2017-09-05T13:35:14Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Signed-off-by: Antal Nemes <antal.nemes@balabit.com> Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>

This cache can only be freed at a very late stage, because the location of the pidfile is calculated from path_cache. To avoid uneasy coupling of gprocess and reloc, we call reloc_deinit in main(). Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com> Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

kira-syslogng · 2017-09-05T14:31:16Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-09-05T14:34:32Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

lbudai added the in progress label Aug 21, 2017

bazsi reviewed Sep 4, 2017

View reviewed changes

bazsi requested changes Sep 4, 2017

View reviewed changes

stats: control command list memleak

28c4757

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

furiel force-pushed the instead-of-suppr branch from 8c5a990 to 3aae4b1 Compare September 5, 2017 13:22

furiel and others added 3 commits September 5, 2017 16:06

value_pairs: free all_macros in deinit

8110db1

Signed-off-by: Antal Nemes <antal.nemes@balabit.com> Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>

libcrypto: missing free-s

7b22bdf

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

lib/messages.c: g_log_remove_handler() in msg_deinit()

1910db0

Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>

furiel force-pushed the instead-of-suppr branch from 3aae4b1 to 2b0a5ea Compare September 5, 2017 14:16

bkil-syslogng and others added 3 commits September 5, 2017 16:19

lib/afiter: reclaim resources at the end via afinter_global_deinit()

2949b22

Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>

suppression file: cleanup

d8a478c

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

furiel force-pushed the instead-of-suppr branch from 2b0a5ea to d8a478c Compare September 5, 2017 14:19

presidento added the bug label Sep 6, 2017

lbudai merged commit 76c3bb5 into syslog-ng:master Sep 6, 2017

lbudai removed the in progress label Sep 6, 2017

furiel mentioned this pull request Sep 6, 2017

F/improve deinit leaks mutex (add missing global deinitialization, Valgrind fixes) #975

Closed

furiel deleted the instead-of-suppr branch May 8, 2018 04:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

memleak hunting: cleanup based on valgrind report #1649

memleak hunting: cleanup based on valgrind report #1649

furiel commented Aug 21, 2017 •

edited

kira-syslogng commented Aug 21, 2017

bazsi Sep 4, 2017

furiel Sep 4, 2017

bazsi Sep 5, 2017

furiel Sep 5, 2017

bazsi Sep 4, 2017

furiel Sep 5, 2017

bazsi Sep 4, 2017

bazsi Sep 4, 2017

bazsi left a comment

kira-syslogng commented Sep 5, 2017

kira-syslogng commented Sep 5, 2017

kira-syslogng commented Sep 5, 2017

memleak hunting: cleanup based on valgrind report #1649

memleak hunting: cleanup based on valgrind report #1649

Conversation

furiel commented Aug 21, 2017 • edited

kira-syslogng commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazsi left a comment

Choose a reason for hiding this comment

kira-syslogng commented Sep 5, 2017

kira-syslogng commented Sep 5, 2017

kira-syslogng commented Sep 5, 2017

furiel commented Aug 21, 2017 •

edited