[syslog-ng] PATCH: Fix some small memory leaks and a SEGV

mark tomlinson mark.tomlinson at alliedtelesis.co.nz
Mon May 11 00:52:23 CEST 2009


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.





More information about the syslog-ng mailing list