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

[Balazs Scheidler]

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.

On Wed, 2009-05-06 at 08:26 +1200, mark tomlinson wrote:
> Fix various memory leaks caused by strings not being freed.
> Also, in log_source_group_deinit(), ensure no drivers are still
> pointing to this possibly soon-to-be-freed structure. Without this
> a SIGSEGV was possible when a SIGHUP occured while syslog was busy.
> 
> diff --git a/src/afinet.c b/src/afinet.c
> index 28b9d50..144a08a 100644
> --- a/src/afinet.c
> +++ b/src/afinet.c
> @@ -191,6 +191,8 @@ afinet_sd_set_transport(LogDriver *s, const gchar *transport)
>  {
>    AFInetSourceDriver *self = (AFInetSourceDriver *) s;
>    
> +  if (self->super.transport)
> +    g_free(self->super.transport);
>    self->super.transport = g_strdup(transport);
>    if (strcasecmp(transport, "udp") == 0)
>      {

this leak only occurs, if you use multiple transport() options.

> diff --git a/src/afstreams.c b/src/afstreams.c
> index df4e758..35d4812 100644
> --- a/src/afstreams.c
> +++ b/src/afstreams.c
> @@ -104,6 +104,8 @@ afstreams_sd_set_sundoor(LogDriver *s, gchar *filename)
>  {
>    AFStreamsSourceDriver *self = (AFStreamsSourceDriver *) s;
>    
> +  if (self->door_filename)
> +    g_string_free(self->door_filename, TRUE);
>    self->door_filename = g_string_new(filename);
>  }

would be better to use g_string_assign() in this case, but I'll take
care about it.

>  
> 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

> 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.

>  
> 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.

-- 
Bazsi