[syslog-ng] Memory leak reintroduced in 3.3.3.

[Balazs Scheidler]

On Fri, 2012-03-30 at 16:12 +0200, Gergely Nagy wrote:
> Jakub Jankowski <shasta at toxcorp.com> writes:
> 
> > I'm pretty sure this particular memory leak was fixed in 3.3.2,
> > but then got reintroduced in this commit:
> > http://git.balabit.hu/?p=bazsi/syslog-ng-3.3.git;a=commitdiff;h=c7070e2a6f1c3a312260bcecf49d62028fef27ce
> > which was done to fix tcp() destination related crash after 3.3.2 was released.
> >
> > So, Bazsi, yes - it does reintroduce the leak :) (in reply to your comment in
> > https://lists.balabit.hu/pipermail/syslog-ng/2011-November/017697.html )
> 
> I think I found the solution: the reason for the crash in afsocket,
> which Bazsi fixed with killing the log_queue_unref in lib/driver.h was
> that it was missing a log_queue_ref. We didn't unref more than we
> should, we reffed less!
> 
> While I don't entirely understand the call chain, comparing how affile
> and afsocket did their queue magic, I prepared a patch that does not
> crash afsocket on reload, and stops the leak aswell. It also looks
> sane-ish to me, so I'm reasonably sure it's fine.
> 
> The patch is attached, I tested it as much as I could: no crash, no
> leak, no empty files. If you could test it too, it would be most
> appreciated!
> 
> differences between files attachment
> (0001-afsocket-Properly-release-our-queue-at-deinit.patch)
> From f15f8d91a28680092ba2ab0bd3ed64220d0ceba8 Mon Sep 17 00:00:00 2001
> From: Gergely Nagy <algernon at balabit.hu>
> Date: Fri, 30 Mar 2012 16:00:17 +0200
> Subject: [PATCH] afsocket: Properly release our queue at deinit
> 
> At deinit time, release the destination driver's queue
> explicitly. This allows us to re-add the implicit queue unref to
> log_dest_driver_release_queue().
> 
> This fixes the memory leak seen with, for example, file destinations.
> 
> Signed-off-by: Gergely Nagy <algernon at balabit.hu>
> ---
>  lib/driver.h                |    1 +
>  modules/afsocket/afsocket.c |    3 +++
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/driver.h b/lib/driver.h
> index 7f7acbd..c805da5 100644
> --- a/lib/driver.h
> +++ b/lib/driver.h
> @@ -188,6 +188,7 @@ log_dest_driver_release_queue(LogDestDriver *self, LogQueue *q)
>    if (q)
>      {
>        self->queues = g_list_remove(self->queues, q);
> +      log_queue_unref(q);
>  
>        self->release_queue(self, q, self->release_queue_data);
>      }
> diff --git a/modules/afsocket/afsocket.c b/modules/afsocket/afsocket.c
> index ae9c5c2..4e9002f 100644
> --- a/modules/afsocket/afsocket.c
> +++ b/modules/afsocket/afsocket.c
> @@ -1217,6 +1217,9 @@ afsocket_dd_deinit(LogPipe *s)
>    if (self->writer)
>      log_pipe_deinit(self->writer);
>  
> +  log_dest_driver_release_queue(&self->super, log_writer_get_queue(self->writer));

this shouldn't be needed, as this is called automatically by
log_dest_driver_deinit_method(), later in this function.

> +  log_writer_set_queue(self->writer, NULL);
> +

This shouldn't be needed here.

>    if (self->flags & AFSOCKET_KEEP_ALIVE)
>      {
>        cfg_persist_config_add(cfg, afsocket_dd_format_persist_name(self, FALSE), self->writer, (GDestroyNotify) log_pipe_unref, FALSE);

You said, the issue was that we didn't put enough refs on the queue,
that was the reason for the original crash. Can you describe where?

Thanks.


-- 
Bazsi