On Fri, 2009-05-08 at 15:16 +0200, Balazs Scheidler wrote:

Hi,

Thanks for your contribution. Although some of these might be genuine leaks, they mostly occur of you use the same option multiple times in the configuration file.

... deletions ...

...

diff --git a/src/dgroup.c b/src/dgroup.c index 7919a54..be02899 100644 --- a/src/dgroup.c +++ b/src/dgroup.c @@ -76,6 +76,16 @@ log_dest_group_deinit(LogPipe *s) NULL); success = FALSE; } + if (p->group) + { + g_free(p->group); + p->group = NULL; + } + if (p->id) + { + g_free(p->id); + p->id = NULL; + } } return success; }

these should be freed when the driver in question gets freed in the log_drv_free() function

This was a definite leak with our configuration and use. I believed the problem to be due to log_dest_group_deinit() being called, and then log_dest_group_init() being called, without log_drv_free() ever being called inbetween. I will have a look at our configuration files. We do change configuration between logging to a file and logging to/from network on the fly (and while many log messages are being generated).

...

diff --git a/src/logsource.c b/src/logsource.c index 311e61f..93a8747 100644 --- a/src/logsource.c +++ b/src/logsource.c @@ -181,6 +181,10 @@ log_source_set_options(LogSource *self, LogSourceOptions *options, gint stats_le g_atomic_counter_set(&self->options->window_size, current_window); self->stats_level = stats_level; self->stats_source = stats_source; + if (self->stats_id) + g_free(self->stats_id); + if (self->stats_instance) + g_free(self->stats_instance); self->stats_id = stats_id ? g_strdup(stats_id) : NULL; self->stats_instance = stats_instance ? g_strdup(stats_instance): NULL; }

...

diff --git a/src/logwriter.c b/src/logwriter.c index 4c895cc..1d1e00e 100644 --- a/src/logwriter.c +++ b/src/logwriter.c @@ -792,6 +792,10 @@ log_writer_set_options(LogWriter *self, LogPipe *control, LogWriterOptions *opti

self->stats_level = stats_level; self->stats_source = stats_source; + if (self->stats_id) + g_free(self->stats_id); + if (self->stats_instance) + g_free(self->stats_instance); self->stats_id = stats_id ? g_strdup(stats_id) : NULL; self->stats_instance = stats_instance ? g_strdup(stats_instance) : NULL;

these again should only be called once, but better if the function is idempotent.

Again, I was seeing deinit()/init() happening, without any calls to free the driver from af_socket_dd.

...

diff --git a/src/sgroup.c b/src/sgroup.c index 904237c..0fdf8fb 100644 --- a/src/sgroup.c +++ b/src/sgroup.c @@ -79,7 +79,18 @@ log_source_group_deinit(LogPipe *s) evt_tag_str("id", p->id), NULL); success = FALSE; - } + } + if (p->group) + { + g_free(p->group); + p->group = NULL; + } + if (p->id) + { + g_free(p->id); + p->id = NULL; + } + log_pipe_append(&p->super, NULL);

hmm... can you show me the backtrace of the crash that this is supposed to fix? when the configuration is deinitialized the old LogSource instances should never be referenced again, thus this dangling pointer will never be referenced again.

Also, group/id is freed whenever the driver instance is freed, thus no need to open code it. I see the possibility of a leak if syslog-ng falls back to the old configuration as we coulnd't initialize the new one (in which case log_sgroup_init is called multiple times on the same config), but otherwise it should not be a leak.

The memory leaks again occured due to seeing deinit/init pairs being called. The fix for the crash is the log_pipe_append(), which prevents the driver using the sgroup again after it has been freed. I don't know the full reason for this happening, only that it did (with HUPs, changing configs, and lots of log messages all at the same time...) The stack trace was as follows: Thread Backtraces: Thread 1 (process 862): #0 log_source_group_queue (s=0x100795d8, msg=0x100aa598, path_options=0x7fa70ba4) at sgroup.c:98 #1 0x1000a238 in log_pipe_forward_msg (self=<value optimized out>, msg=0x1, path_options=0x3025134c) at logpipe.h:121 #2 0x1000a238 in log_pipe_forward_msg (self=<value optimized out>, msg=0x1, path_options=0x3025134c) at logpipe.h:121 #3 0x10026148 in log_source_queue (s=0x100bacf0, msg=0x100aa598, path_options=<value optimized out>) at logpipe.h:121 #4 0x1001b43c in log_reader_fd_dispatch (source=<value optimized out>, callback=<value optimized out>, user_data=<value optimized out>) at logpipe.h:121 #5 0x30070f9c in IA__g_main_context_dispatch (context=0x100507f0) at gmain.c:1916 #6 0x300732c4 in g_main_context_iterate (context=0x0, block=0, dispatch=1, self=<value optimized out>) at gmain.c:2547 #7 0x300733c8 in IA__g_main_context_iteration (context=0x0, may_block=268765168) at gmain.c:2606 #8 0x10005324 in main_loop_run (cfg=0x0) at main.c:175 #9 0x1000584c in main (argc=1, argv=0x7fa70de4) at main.c:428 The point it crashed at (sgroup.c:98) was the line: (*self->processed_messages)++; in log_source_group_queue(). But self has already been freed, so the memory address it was incrementing was bogus, causing the SEGV. The fix above means that the log_pipe_forward_msg() call will drop the message in this case.