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@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@balabit.hu> Signed-off-by: Viktor Juhasz <jviktor@balabit.hu> Signed-off-by: Balazs Scheidler <bazsi@balabit.hu> On Mon, 2011-05-23 at 11:56 +0200, Gergely Nagy wrote:
From: Peter Gyorko <gyorkop@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@balabit.hu> Signed-off-by: Viktor Juhasz <jviktor@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