[Home]

Summary:ASTERISK-10274: sip peer with missing close bracket causes all subsequent sip peers not to load
Reporter:dtyoo (dtyoo)Labels:
Date Opened:2007-09-10 17:34:20Date Closed:2008-09-12 18:20:36
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 10690-reload.diff
( 1) 10690-reload2.diff
( 2) 10690-reload3.diff
( 3) 20080716__bug10690.diff.txt
Description:We have found that if you leave the trailing square bracket off a given sip peer name in sip.conf, then you reload chan_sip.so, all peers that would be loaded after the broken peer do not load.  It looks like the config parser understands what is wrong as I see the following in the messages file:

config.c: parse error: no closing ']', line 25 of xxxx.sip.conf

But then all the rest of your sip peers disappear.  Obviously the error is the lack of a closing brace.  But I would think that the impact of this config error could be minimized if just this sip peer were tossed, and the next sip peer were loaded normally.  With frequent manual editing of a large sip.conf this is bound to happen sometime.  This has happened to us a couple times, each time causing a fairly substantial outage for us as several thousand sip peers disappear.

****** ADDITIONAL INFORMATION ******

Dell 1950, Centos 4.5, 2 x 1.6GHz Xeon, 2GB RAM

Approx 5000 sip peers in sip.conf, #include extensively used to pull in peers from other files.
Comments:By: Jason Parker (jparker) 2007-09-10 18:07:19

In my opinion, the config file shouldn't loaded at all if there is a problem like that.  Consider the following, where the accidental addition of a [ line can cause a peer to not have a password.


[5554]
....

[5555]
type=friend
context=default
host=dynamic
dtmfmode=rfc2833
username=5555
[
secret=5555
disallow=all
allow=ulaw

[5556]
...
By: dtyoo (dtyoo) 2007-09-10 21:15:43

qwell-

I do see your point.  If there is some way you could abort the reload and just continue with the same sip peers loaded that were previously there, I think that could be reasonable.  The existing message "config.c: parse error: no closing ']'" is currently a log warning.  I would suggest making this an error and somehow prominently stating that the reload was aborted.
By: Jason Parker (jparker) 2007-09-11 10:51:29

I discussed this with file, and he agrees with you.  In trunk, this behavior should already be there, but it appears that includes act a little differently with regards to returning a NULL config.

I'll work on this a bit today, and see if I can't get something working.
By: Jason Parker (jparker) 2007-09-11 12:24:38

Try this patch.  It's only for chan_sip right now, but it should do the right thing.  Let me know if it works, and I'll change the other modules that read config files too.
By: dtyoo (dtyoo) 2007-09-11 16:37:23

qwell-

The patch is working fine.  Its even better than I thought.  The peer settings for the broken peer are kept as they were.  I thought they would be wiped out.  Many thanks...
By: Jason Parker (jparker) 2007-09-12 17:00:59

I just uploaded another patch which should also work for chan_iax2 and chan_h323, if you're interested.

As far as I could tell, everything else that reads config files already does the right thing.
By: dtyoo (dtyoo) 2007-10-20 13:37:39

qwell-

Any chance that the sip fix could be committed?  I haven't tested the iax and h323 patch as we don't use these.  But if you need me to test them I can do so.  Let me know.
By: Jason Parker (jparker) 2007-11-02 17:35:01

Updated patch for 1.4
By: dtyoo (dtyoo) 2007-11-13 12:56:04.000-0600

qwell-

I tested the latest patch (10690-reload3.diff) and it is working fine.  On reload the broken peer is tossed out, and the existing peer state preserved.  On restart you get:

[Nov 13 13:49:12] WARNING[20496]: config.c:607 process_text_line: parse error: no closing ']', line 25 of xxx.sip.conf
[Nov 13 13:49:12] NOTICE[20496]: chan_sip.c:16530 reload_config: Unable to load config sip.conf

No sip peers are loaded at all in this case.  Ideally I would say that restart should behave the same way that reload does.  There won't be any existing state so I would expect this peer to just not be present, but for the rest of the sip peers to load normally.
By: Jason Parker (jparker) 2007-11-13 15:22:46.000-0600

That leads us back to my original question, of how do you know where the peer is supposed to end?  If there is a typo, all bets are off.

In the case of the initial load, I don't think any peers should be loaded.
By: dtyoo (dtyoo) 2007-11-13 15:41:30.000-0600

qwell-

I see your point, and I'm ok with the behavior as it is.  The only thing I would suggest is that being unable to load any sip peers due to parsing errors would warrant an "ERROR" in the log, not just a "NOTICE".
By: Digium Subversion (svnbot) 2007-12-12 18:08:27.000-0600

Repository: asterisk
Revision: 92696

U   branches/1.4/channels/chan_h323.c
U   branches/1.4/channels/chan_iax2.c
U   branches/1.4/channels/chan_sip.c
U   branches/1.4/main/config.c

------------------------------------------------------------------------
r92696 | qwell | 2007-12-12 18:08:26 -0600 (Wed, 12 Dec 2007) | 7 lines

If a typo is found in a config file, we previous continued on with what was already loaded.
We do not want to do this (see bug below for details).

This makes it so that if a [ is found without a ], the entire config will fail, and nothing in it will be loaded.

Isue ASTERISK-10274.

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=92696
By: Digium Subversion (svnbot) 2007-12-12 18:15:04.000-0600

Repository: asterisk
Revision: 92697

_U  trunk/
U   trunk/channels/chan_h323.c
U   trunk/channels/chan_sip.c
U   trunk/main/config.c

------------------------------------------------------------------------
r92697 | qwell | 2007-12-12 18:15:04 -0600 (Wed, 12 Dec 2007) | 16 lines

Merged revisions 92696 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

(closes issue ASTERISK-10274)
........
r92696 | qwell | 2007-12-12 18:11:09 -0600 (Wed, 12 Dec 2007) | 7 lines

If a typo is found in a config file, we previous continued on with what was already loaded.
We do not want to do this (see bug below for details).

This makes it so that if a [ is found without a ], the entire config will fail, and nothing in it will be loaded.

Issue 10690.

........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=92697
By: Tzafrir Cohen (tzafrir) 2007-12-15 04:12:33.000-0600

If a typo is found in users.conf, users.conf will fail to load. However everbody just silently ignore the fact that users.conf fails to load, as it is an optional file.

test configuration:

users.conf:
[general]
.. ; from sample config
[user1]
hasiax=yes
type=friend



$ asterisk -rx 'iax2 show peers'
./live_ast run -rx 'iax2 show peers'
Name/Username    Host                 Mask             Port          Status    
demo/asterisk    216.207.245.47  (S)  255.255.255.255  4569          Unmonitored
user1            (Unspecified)   (S)  0.0.0.0          4569          Unmonitored
2 iax2 peers [0 online, 0 offline, 2 unmonitored]

$ echo [ >>/etc/asterisk/users.conf
$ asterisk -rx 'iax2 reload'
$ ./live_ast run -rx 'iax2 show peers'
Name/Username    Host                 Mask             Port          Status    
demo/asterisk    216.207.245.47  (S)  255.255.255.255  4569          Unmonitored
1 iax2 peers [0 online, 0 offline, 1 unmonitored]

Note: iax.conf was succesfully parsed. the peer from users.conf is silently gone.
By: Tzafrir Cohen (tzafrir) 2007-12-15 04:16:16.000-0600

Please see my previous comment: http://bugs.digium.com/10690#75458 - with users.conf this does not work as planned, and thus you end up with a partial configuration. And there, all bets are off.
By: Tilghman Lesher (tilghman) 2008-07-16 19:24:44

Candidate patch uploaded, to add yet another return value to ast_config_load, such that we can detect when a secondary file (such as users.conf) is of an invalid format.
By: Mark Michelson (mmichelson) 2008-08-26 15:51:17

Is this latest patch waiting on testing? Does it just need peer review before being committed? What needs to be done to move this along?
By: Tilghman Lesher (tilghman) 2008-08-26 16:03:15

Needs testing
By: Digium Subversion (svnbot) 2008-09-12 18:20:33

Repository: asterisk
Revision: 142992

U   trunk/apps/app_alarmreceiver.c
U   trunk/apps/app_amd.c
U   trunk/apps/app_directory.c
U   trunk/apps/app_festival.c
U   trunk/apps/app_followme.c
U   trunk/apps/app_meetme.c
U   trunk/apps/app_minivm.c
U   trunk/apps/app_osplookup.c
U   trunk/apps/app_playback.c
U   trunk/apps/app_queue.c
U   trunk/apps/app_rpt.c
U   trunk/apps/app_voicemail.c
U   trunk/channels/chan_agent.c
U   trunk/channels/chan_alsa.c
U   trunk/channels/chan_console.c
U   trunk/channels/chan_dahdi.c
U   trunk/channels/chan_gtalk.c
U   trunk/channels/chan_h323.c
U   trunk/channels/chan_iax2.c
U   trunk/channels/chan_jingle.c
U   trunk/channels/chan_mgcp.c
U   trunk/channels/chan_oss.c
U   trunk/channels/chan_phone.c
U   trunk/channels/chan_sip.c
U   trunk/channels/chan_skinny.c
U   trunk/channels/chan_unistim.c
U   trunk/codecs/codec_adpcm.c
U   trunk/codecs/codec_alaw.c
U   trunk/codecs/codec_dahdi.c
U   trunk/codecs/codec_g722.c
U   trunk/codecs/codec_g726.c
U   trunk/codecs/codec_gsm.c
U   trunk/codecs/codec_lpc10.c
U   trunk/codecs/codec_speex.c
U   trunk/codecs/codec_ulaw.c
U   trunk/funcs/func_config.c
U   trunk/funcs/func_odbc.c
U   trunk/include/asterisk/config.h
U   trunk/main/asterisk.c
U   trunk/main/cdr.c
U   trunk/main/config.c
U   trunk/main/dnsmgr.c
U   trunk/main/dsp.c
U   trunk/main/enum.c
U   trunk/main/features.c
U   trunk/main/http.c
U   trunk/main/loader.c
U   trunk/main/manager.c
U   trunk/main/rtp.c
U   trunk/main/udptl.c
U   trunk/res/res_adsi.c
U   trunk/res/res_config_ldap.c
U   trunk/res/res_config_pgsql.c
U   trunk/res/res_config_sqlite.c
U   trunk/res/res_http_post.c
U   trunk/res/res_indications.c
U   trunk/res/res_jabber.c
U   trunk/res/res_musiconhold.c
U   trunk/res/res_odbc.c
U   trunk/res/res_phoneprov.c
U   trunk/res/res_smdi.c
U   trunk/res/res_snmp.c

------------------------------------------------------------------------
r142992 | tilghman | 2008-09-12 18:20:32 -0500 (Fri, 12 Sep 2008) | 27 lines

Create a new config file status, CONFIG_STATUS_FILEINVALID for differentiating
when a file is invalid from when a file is missing.  This is most important when
we have two configuration files.  Consider the following example:

Old system:
sip.conf     users.conf     Old result               New result
========     ==========     ==========               ==========
Missing      Missing        SIP doesn't load         SIP doesn't load
Missing      OK             SIP doesn't load         SIP doesn't load
Missing      Invalid        SIP doesn't load         SIP doesn't load
OK           Missing        SIP loads                SIP loads
OK           OK             SIP loads                SIP loads
OK           Invalid        SIP loads incompletely   SIP doesn't load
Invalid      Missing        SIP doesn't load         SIP doesn't load
Invalid      OK             SIP doesn't load         SIP doesn't load
Invalid      Invalid        SIP doesn't load         SIP doesn't load

So in the case when users.conf doesn't load because there's a typo that
disrupts the syntax, we may only partially load users, instead of failing with
an error, which may cause some calls not to get processed.  Worse yet, the old
system would do this with no indication that anything was even wrong.

(closes issue ASTERISK-10274)
Reported by: dtyoo
Patches:
      20080716__bug10690.diff.txt uploaded by Corydon76 (license 14)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=142992