cfg: Fix double free/double close crashes

[swstephenson]

Fix the double free of level->yy_buff and double close of
level->file.include_file that can occur during a config reload if a config
file is deleted.

This was observed on a system with frequent dynamic config change. The issue
occurs when a config file is deleted between the glob expansion/directory
scanning in cfg_lexer_include_file_*() functions and files being opened in
cfg_lexer_start_next_include.

Fixes #1720

Signed-off-by: Sam Stephenson sam.stephenson@alliedtelesis.co.nz

[kira-syslogng]

This user does not have permission to start the build. Can one of the admins verify this patch and start the build? (admin please type: ok to test)

[Kokan]

@kira-syslogng ok to test

[kira-syslogng]

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

[gaborznagy]

@swstephenson I have reviewed the PR and found that it indeed solves the double free problem and no crash occurs, but syslog-ng is not handling correctly the error case in cfg_lexer_start_next_include().
When a file read error occurs what happens is that we terminate the lexer and syslog-ng starts without any configuration, doing anything:

[2017-11-07T12:59:49.801231] Error opening include file; filename='/home/gabornagy/install/OSE-Issue1720/share/syslog-ng/include/scl/cisco/plugin.conf', depth='2'
[2017-11-07T12:59:49.801341] Unable to detect fully qualified hostname for localhost, use_fqdn() will use the short hostname;
[2017-11-07T12:59:49.801395] Running application hooks; hook='1'
[2017-11-07T12:59:49.801400] Running application hooks; hook='3'
[2017-11-07T12:59:49.801415] syslog-ng starting up; version='3.12.1'

I use the same reproduction method which furiel mentioned in #1720:
chmod 000 cisco/plugin.conf

This is not a good way of error handling, syslog-ng should be stopped in such cases, otherwise the user would not spot the issue.

The reason that the return FALSE is not sufficient error reporting is that we also return with FALSE when we finished with including every file:

  if (self->include_depth == 0)
    {
      return FALSE;
    }

and this is used in lib/cfg-lex.l:

if (!cfg_lexer_start_next_include(yyextra))
  {
    *yylloc = yyextra->include_stack[0].lloc;
    yyterminate();
  }

Could you resolve this termination part in this PR as well, please?

[swstephenson]

Hi @gaborznagy,

If I understand you correctly, I see the following:

If syslog-ng is already running with a config, and one of the files is made unreadable (with the @furiel reproduction method), the reload fails and syslog reverts to it's old config.

If syslog-ng is starting up for the first time and the config load fails (for the same reason) I see it exiting with a non-zero status and an error message that parsing the configuration failed, as below.

# syslog-ng -f /etc/syslog-ng/syslog-ng.conf -F
[2017-11-08T16:04:33.331737] Error opening include file; filename='/etc/syslog-ng/conf.d/stream.conf', depth='1'
Error parsing pragma, Error including /etc/syslog-ng/conf.d in /etc/syslog-ng/syslog-ng.conf at line 4, column 10:

@include "/etc/syslog-ng/conf.d"
         ^^^^^^^^^^^^^^^^^^^^^^^

syslog-ng documentation: https://www.balabit.com/support/documentation?product=syslog-ng-ose
mailing list: https://lists.balabit.hu/mailman/listinfo/syslog-ng
Error parsing config, syntax error, unexpected LL_ERROR, expecting $end in /etc/syslog-ng/syslog-ng.conf at line 4, column 1:

@include "/etc/syslog-ng/conf.d"
^

syslog-ng documentation: https://www.balabit.com/support/documentation?product=syslog-ng-ose
mailing list: https://lists.balabit.hu/mailman/listinfo/syslog-ng
# echo $?
1

Are you getting something different?

(For getting notification of the failure I would have thought the messages and non-zero exit status should be sufficient. Process supervisors should be able to deal with this as well. And #1739 should make it possible to directly detect the reload failure case)

[gaborznagy]

Yes I have different scenario than you.
The reason is that I have reproduced the issue with an invalid scl file which is included through globbing in scl.conf
(repr. steps like before: chmod 000 scl/cisco/plugin.conf).

In your case it is an invalid config file, which is included directly from the config file.
If I reproduce the issue the same way as you did, I have the same result.

[gaborznagy]

HI @swstephenson !

We have found the source why syslog-ng is not failing when an include fails inside a glob (thanks too Kokan! ).
This error was not discovered before, thank you for your contribution, without your PR it would be still undiscovered!

So what we have found is that when the include procedure returns False (i.e. stop including more files) and the include depth shows that we were not finished we report this as an error state.
I have pasted the diff of the patch.

diff --git a/lib/cfg-lex.l b/lib/cfg-lex.l
index c16155b..243837e 100644
--- a/lib/cfg-lex.l
+++ b/lib/cfg-lex.l
@@ -306,8 +306,13 @@ word       [^ \#'"\(\)\{\}\\;\r\n\t,|\.@:]
 <INITIAL><<EOF>>           {
                              if (!cfg_lexer_start_next_include(yyextra))
                                {
-                                 *yylloc = yyextra->include_stack[0].lloc;
-                                 yyterminate();
+                                 if (yyextra->include_depth == 0)
+                                   {
+                                     *yylloc = yyextra->include_stack[0].lloc;
+                                     yyterminate();
+                                   }
+                                 else
+                                    return LL_ERROR;

What do you think about this solution?
I think if this patch would be integrated then all error cases would be covered and your PR could be merged too.

[swstephenson]

Right, I'm with you now. I was able to reproduce as you describe. Your patch seems to do the trick. Glad this prompted the discovery of another issue!

[kira-syslogng]

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

[kira-syslogng]

Build SUCCESS, the tests were executed on test branch: master and test suite: functions