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@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@toxcorp.com> Signed-off-by: Gergely Nagy <algernon@balabit.hu> Signed-off-by: Balazs Scheidler <bazsi@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@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=c7070e2a6f1c... 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@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@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