Hi, I've changed this patch almost completely, but the diagnosis of the possible bug was right, I just think I've found a simpler implementation. Here's the patch: commit 694164b1ea6f575424413b3b1f359af09457db16 Author: Balazs Scheidler <bazsi@balabit.hu> Date: Thu Jun 9 14:54:07 2011 +0200 LogReader: Proper wakeup function handling for deinitialized LogReaders The wakeup() callback can be called even if a LogReader is deinitialized (when the Writer is deinitialized _after_ the reader, and some messages are flushed out in the deinit path), in which case we shouldn't trigger the schedule_wakeup event. The original patch by Viktor Juhasz, but was completely reworked by Balazs Scheidler. 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: Juhasz Viktor <jviktor@balabit.hu>
Rearrange how self->super.wakeup is handled, in order to stop a race condition where it might be called earlier than it is set, or after it should have been cleared.
If the reader is uninitialized, but the writer tries to flush the queue, and call log_msg_refcache_stop(), that in turn will call the reader's wakeup function. Since the reader is uninitialized, this can easily corrupt the ivykis event list.
To avoid this scenario, the wakeup callback should be set differently, and cleared early.
Signed-off-by: Viktor Juhasz <jviktor@balabit.hu> --- lib/logreader.c | 16 ++++++++++++---- 1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/lib/logreader.c b/lib/logreader.c index 268be6e..4e56d05 100644 --- a/lib/logreader.c +++ b/lib/logreader.c @@ -577,7 +577,8 @@ static gboolean log_reader_init(LogPipe *s) { LogReader *self = (LogReader *) s; - + /* If reader is initialized than log_msg_ref_cache_stop can call the wakeup function */ + self->super.wakeup = log_reader_wakeup; if (!log_source_init(s)) return FALSE; /* check for new data */ @@ -612,7 +613,6 @@ log_reader_init(LogPipe *s) if (!log_reader_start_watches(self)) return FALSE; iv_event_register(&self->schedule_wakeup); - return TRUE; }
@@ -622,7 +622,14 @@ log_reader_deinit(LogPipe *s) LogReader *self = (LogReader *) s;
main_loop_assert_main_thread(); - + /* + the wakeup function must be set to NULL, because it can be occured than + the reader is uninitialized but the writer flush the queue and call the log_msg_refcache_stop and + log_msg_refcache_stop call the wakeup function if it is not null, but in this case the reader + is uninitialized and the call of iv_event_post for the unregistered event can cause an ivykis list corruption. + IF logreader is uninitialized than the wakeup function is NULL so the iv_event_post isn't called. + */ + self->super.wakeup = NULL; iv_event_unregister(&self->schedule_wakeup); log_reader_stop_watches(self); if (!log_source_deinit(s)) @@ -684,7 +691,8 @@ log_reader_new(LogProto *proto) self->super.super.init = log_reader_init; self->super.super.deinit = log_reader_deinit; self->super.super.free_fn = log_reader_free; - self->super.wakeup = log_reader_wakeup; + /* Set wakeup function to NULL because this is set in the log_reader_init and unset in the log_reader_deinit */ + self->super.wakeup = NULL; self->proto = proto; self->immediate_check = FALSE; self->pollable_state = -1;
-- Bazsi