[syslog-ng] [PATCH 7/7] [logreader]: Proper wakeup function handling

[Balazs Scheidler]

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 at 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 at balabit.hu>
    Signed-off-by: Balazs Scheidler <bazsi at balabit.hu>


On Mon, 2011-05-23 at 11:56 +0200, Gergely Nagy wrote:
> From: Juhasz Viktor <jviktor at 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 at 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