Seg fault with include file contains an error
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; }
Hi, The patch looks good and I could successfully reproduce the problem without the patch and it seems to be gone with the patch. Thanks for tracking this down, and sorry that it took such a long time to review and integrate your patch. Here's the official git patch entry, now visible on git.balabit.hu commit 45566d957609026abe41a3292003909b23615124 Author: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz> Date: Tue Nov 3 20:44:59 2009 +0100 cfg-lex: fixed a possible segmentation fault in HUP processing if included files have syntax errors This patch fixes several use-after-free problems if a file with a syntax error is included and syslog-ng is reloaded multiple times. Thanks for Anthony Lineham for tracking this down. On Mon, 2009-09-14 at 16:50 +1200, 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; }
______________________________________________________________________________ Member info: https://lists.balabit.hu/mailman/listinfo/syslog-ng Documentation: http://www.balabit.com/support/documentation/?product=syslog-ng FAQ: http://www.campin.net/syslog-ng/faq.html
-- Bazsi
participants (2)
-
anthony lineham
-
Balazs Scheidler