[syslog-ng] Fwd: Seg fault with include file contains an error

[anthony lineham]

Hi,

I sent this patch to the list last month, but haven't had any feedback yet. Does anyone have any comments (aside from my poor spelling and grammar)? Am I way off track with this fix or is it ok?

Looking forward to your comments.

best regards,
Anthony

>>> On 9/14/2009 at  4:50 PM, in message
<4ADD7D39.7D71.00E6.0 at alliedtelesis.co.nz>, anthony lineham wrote: 
> Hi Bazsi,
> 
> I've been investigating a seg fault that occurs when using an include 
> statement in the config file. The error 
> occurs when one of the included files contains an syntax error. The first 
> time syslog-ng is restarted with the bad
> config, the error is detected and restart is aborted. However, if a second 
> HUP is sent a seg fault occurs.
> 
> I had a bit of trouble tracking down the cause and I'm still not entirely 
> sure of the mechanism that leads to the
> crash but I found a couple of apparent problems, which when corrected 
> prevent it.
> 
> 1. There is a global variable "include_depth" which normally gets 
> decremented back to zero after successful 
> parsing of included config files. However, if an error is detected it stays 
> at its current value and subsequent 
> restarts increment from that point. This doesn't cause the crash, but given 
> enough restarts would lead to
> overrun or exhaustion of the "include_stack" array.
> 
> 2. In the case of unsuccessful config parsing certain bits of memory are 
> dealloced but their pointers that not
> reset. This seems to be what was causing the seg fault but I've found it a 
> bit hard to pin down.
> 
> 3. There was an off-by-1 error in the included config deinit loop.
> 
> The following patch fixes the problem, but may not necessarily be the best 
> way to do it - particularly issue 1.
> 
> Regards,
> Anthony
> 
> --- syslog-ng_3.0.1-63-g41f77f5-old/src/cfg-lex.l       2009-09-14 
> 16:02:09.000000000 +1200
> +++ syslog-ng_3.0.1-63-g41f77f5/src/cfg-lex.l   2009-09-14 
> 16:15:30.000000000 +1200        
> @@ -626,17 +626,24 @@
>  {
>    gint i;
> 
> -  for (i = 0; i < include_depth; i++)
> +  for (i = 0; i <= include_depth; i++)
>      {
>        CfgIncludeLevel *level = &include_stack[i];
> 
>        if (level->current_file)
> -        g_free(level->current_file);
> +        {
> +          g_free(level->current_file);
> +          level->current_file = NULL;
> +        }
> 
>        g_slist_foreach(level->files, (GFunc) g_free, NULL);
>        g_slist_free(level->files);
>        level->files = NULL;
>        if (level->yybuf)
> -        yy_delete_buffer(level->yybuf);
> +        {
> +          yy_delete_buffer(level->yybuf);
> +          level->yybuf = NULL;
> +        }
>      }
> +  include_depth = 0;
>  }
>