Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cfg parse error behaviour on a mismatch of #!ifdef/#!endif statements #2057

Closed
henningw opened this issue Sep 6, 2019 · 9 comments
Closed

Comments

@henningw
Copy link
Contributor

henningw commented Sep 6, 2019

The #!ifdef/#endif statement parsing of the kamailio.cfg does not work correctly on a mismatch of #!ifdef/#!define statements. In some cases will just ignore the #!defines and parses the cfg file without the statements.

version: kamailio 5.3.0-pre0 (x86_64/linux) ed10d7

henning@linux:~/repositories/kamailio/src> ./kamailio -c -f ../etc/test-define.cfg
0(9501) CRITICAL: [core/cfg.y:3539]: yyerror_at(): parse error in config file /home/henning/repositories/kamailio/src/../etc/test-define.cfg, line 12, column 8: syntax error
0(9501) CRITICAL: [core/cfg.y:3539]: yyerror_at(): parse error in config file /home/henning/repositories/kamailio/src/../etc/test-define.cfg, line 12, column 8: unknown config variable
0(9501) CRITICAL: [core/cfg.y:3536]: yyerror_at(): parse error in config file /home/henning/repositories/kamailio/src/../etc/test-define.cfg, line 12, column 9-11:
ERROR: bad config file (3 errors)

Test cfg to show the problem:

henning@linux:~/repositories/kamailio/etc> cat test-define.cfg
#!KAMAILIO

#!define foo

#!ifdef foobar1
#!ifdef foobar2
#!endif
#!endif
#!endif

#!ifdef bar
invalid=123
#else 
children=8
#!endif

request_route {
        ;
}

The "bar" condition is parsed, even if the #!define was not set. The WARN message is also not shown in this error case.

Probably the easist fix would be to stop kamailio startup on a mismatch of #!ifdef/#endif statements and output ERROR instead of the current WARN.

@miconda
Copy link
Member

miconda commented Sep 6, 2019

While looking at the code I discovered that the index of preprocessor conditions could become negative and even be used for a wrongly placed #!else - I pushed a fix for it: 8f6e826 -- this probably needs to be backported.

Otherwise, it is fine for me to stop starting kamailio on a mismatch of ifdef/ifndef-endif pairs. But backporting to old stable branches might cause troubles, with kamailio not starting after upgrade, so I would keep it for 5.3 and the future.

@henningw
Copy link
Contributor Author

henningw commented Sep 9, 2019

Thank you Daniel, the example from above works now. Unfortunately the opposite condition still not work:

$./kamailio -c -f ../etc/test-define.cfg -Y /root
0(10801) CRITICAL: [core/cfg.y:3532]: yyerror_at(): parse error in config file /home/henning/repositories/kamailio/src/../etc/test-define.cfg, from line 19, column 2 to line 20, column 0: syntax error
ERROR: bad config file (1 errors)
0(10801) WARNING: [core/ppcfg.c:223]: pp_ifdef_level_check(): different number of preprocessor directives: 1 more #!if[n]def as #!endif

$ cat test-define.cfg
#!KAMAILIO

#!define foo

#!ifdef foobar1
#!ifdef foobar2
#!ifdef foobar3
#!endif
#!endif

#!ifdef bar
invalid=123
#else 
children=8
#!endif

request_route {
        ;
}

@miconda
Copy link
Member

miconda commented Sep 10, 2019

What exactly do you mean by the opposite condition? For the example above I see it fails due to bad config file.

@henningw
Copy link
Contributor Author

The cfg is invalid, to reproduce it easier. My expectation would be that the config would load fine, as the "bar" condition is not defined at all.

@henningw
Copy link
Contributor Author

But maybe it is easier to just stop loading the cfg in this mismatch conditions.

@miconda
Copy link
Member

miconda commented Sep 10, 2019

In that example, the config is invalid because it is empty, you have 3 ifdefs, then two endifs, so that ifdef bar is all together skipped to the end of the file. The error line is 19, which is the end of the file. This case is ok imo.

@henningw
Copy link
Contributor Author

Ah, I misread the line number. You are right. The backport of 8f6e826 is then only open, I think.

@miconda
Copy link
Member

miconda commented Sep 10, 2019

You can backport, if you want. Otherwise I will do it before the next release in the 5.2 series.

@henningw
Copy link
Contributor Author

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants