[syslog-ng] [PATCH 2/7] [affile] fixed memory leak using macros in template

[Balazs Scheidler]

Hi,

I generally agree with this patch, though I'd like to make one further
step. AFFileDestWriter doesn't use the fact that filename is a GString,
thus use a simple character pointer.

I've also removed a stale comment, which was left in by the patch.

NOTE: there's a possibility for optimization, as a new GString instance
is allocated in the fast path for all macro expansion. This could
probably be spared (using a per-thread variable, or using the value
under the protection of the already acquired lock). It'd be interesting
to see a profile with a macro in the filename, this allocation is almost
certainly makes a difference CPU-wise (although will quite possibly
perform better than a single destination if several CPUs/cores are
available).

Here's the updated patch:

commit b9251c2603d3ffba557425387de86aa9093e5429
Author: Balazs Scheidler <bazsi at balabit.hu>
Date:   Sat May 28 14:14:25 2011 +0200

    fixed memory leak using macros in template
    
    Always allocate memory for self->filename, so that it can always be
    freed properly. The former method of trying to figure out when to free
    it and when to ignore it since it's owned by something else didn't
    quite work, and led to memory leaks.
    
    This patch also changes AFFileDestWriter->filename to be a simple char
    pointer instead of a GString.
    
    Signed-off-by: Peter Gyorko <gyorkop at balabit.hu>
    Signed-off-by: Viktor Juhasz <jviktor at balabit.hu>
    Signed-off-by: Balazs Scheidler <bazsi at balabit.hu>


On Mon, 2011-05-23 at 11:56 +0200, Gergely Nagy wrote:
> From: Peter Gyorko <gyorkop at balabit.hu>
> 
> Always allocate memory for self->filename, so that it can always be
> freed properly. The former method of trying to figure out when to free
> it and when to ignore it since it's owned by something else didn't
> quite work, and led to memory leaks.
> 
> Signed-off-by: Peter Gyorko <gyorkop at balabit.hu>
> Signed-off-by: Viktor Juhasz <jviktor at balabit.hu>
> ---
>  modules/affile/affile.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/modules/affile/affile.c b/modules/affile/affile.c
> index 6e4f5f0..c5e89f5 100644
> --- a/modules/affile/affile.c
> +++ b/modules/affile/affile.c
> @@ -690,7 +690,7 @@ affile_dw_new(AFFileDestDriver *owner, GString *filename)
>  
>    /* we have to take care about freeing filename later. 
>       This avoids a move of the filename. */
> -  self->filename = filename;
> +  self->filename = g_string_new(filename->str);
>    g_static_mutex_init(&self->lock);
>    return self;
>  }
> @@ -1037,7 +1037,6 @@ affile_dd_open_writer(gpointer args[])
>        if (!next)
>  	{
>  	  next = affile_dw_new(self, filename);
> -	  args[1] = NULL;
>            if (!log_pipe_init(&next->super, cfg))
>  	    {
>  	      log_pipe_unref(&next->super);
> @@ -1051,8 +1050,6 @@ affile_dd_open_writer(gpointer args[])
>                g_static_mutex_unlock(&self->lock);
>              }
>  	}
> -      else
> -        g_string_free(filename, TRUE);
>      }
>    next->queue_pending = TRUE;
>    /* we're returning a reference */
> @@ -1115,8 +1112,7 @@ affile_dd_queue(LogPipe *s, LogMessage *msg, const LogPathOptions *path_options,
>  	  next = main_loop_call((void *(*)(void *)) affile_dd_open_writer, args, TRUE);
>  	}
>        /* args[1] is set to NULL whenever the affile_dd_open_writer uses it for the newly constructed AFFileDestWriter instance */
> -      if (args[1])
> -        g_string_free(filename, TRUE);
> +      g_string_free(filename, TRUE);
>      }
>    if (next)
>      {

-- 
Bazsi