[syslog-ng] Memory leak reintroduced in 3.3.3.

[Balazs Scheidler]

Hi,

I think this patch solves both problems and addresses the original flaw
in the LogDestDriver queue ref counting. Instead of working around it in
afsocket.

The code in affile is needed to avoid keeping the LogQueue instances of
reaped files around, the same is not needed for the afsocket driver,
which only has a single queue.

commit 9064e909e8aef518ec3c073bccc1bf09da9a2c06
Author: Balazs Scheidler <bazsi at balabit.hu>
Date:   Sun Apr 1 09:42:58 2012 +0200

    driver: fixed possible leak and use-after-free
    
    log_dest_driver_release_queue() was possibly leaking the LogQueue
    instances for file destinations when they got reaped. This was
    caused by an earlier patch that fixed a crash in reloads,
    more specifically this one: c7070e2a6f1c3a312260bcecf49d62028fef27ce
    
    This patch should fix both cases properly, the leak in the file destination
    driver and the original crash in the afsocket destination.
    
    Also, this patch fixes a use-after-free condition, the next member of
    a GList structure was referenced after it was removed from the list.
    
    Kudos to Jakub for the detailed bug report and Algernon for the origianl
    fix.
    
    Reported-By: Jakub Jankowski <shasta at toxcorp.com>
    Signed-off-by: Gergely Nagy <algernon at balabit.hu>
    Signed-off-by: Balazs Scheidler <bazsi at balabit.hu>



On Sat, 2012-03-31 at 22:42 +0200, Balazs Scheidler wrote:
> 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