Summary:	ASTERISK-05240: [patch] brackets inside a comment in voicemail.conf can cause an uninteded context change when changing password
Reporter:	Michael Anderson (michaela)	Labels:
Date Opened:	2005-10-04 20:43:55	Date Closed:	2008-01-15 15:53:09.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_voicemail.c.fixed_format.patch ( 1) app_voicemail.c.fixed_memcpy.patch ( 2) app_voicemail.c.HEAD.patch ( 3) app_voicemail.c.patch
Description:	If a comment inside a context contains a square bracket character ('[') the parser in vm_change_password will interpret the remainder of the line as a new context. If the mail box definition follows the comment the voicemail.conf file will not be changed as the comparison of contexts will fail. For instance with the following file portion the voicemail password change will fail: [default] ;MBOX => PASSWORD[,Firstname Lastname[,email[,[pager][,options]]]] 1234 => 1111,Mr Smith,msmith@somewhere.com The attached patch changes the parsing of voicemail.conf to make it a bit more robust and tolerant of odd comments.  It still fails if the last line of data does not have a newline. ****** ADDITIONAL INFORMATION ****** The problem can be avoided by the end user by moving the comment above the context definition or by avoiding the use of square brackets in comments.
Comments:	By: Clod Patry (junky) 2005-10-04 20:47:36 Do you have disclaimer on file? This is the first step for a patch review. By: Michael Anderson (michaela) 2005-10-04 21:10:15 I just sent the disclaimer in now. By: BJ Weschke (bweschke) 2005-10-14 19:11:36 michaela: Thanks for this catch and the patch to correct it! Please don't use c++ style comments (//) in the patch, and review identing in the file. See doc/CODING-GUIDELINES for more guidelines. If you correct these issues and resubmit, we'll move this along to try and get your change merged in with CVS-HEAD. By: Michael Anderson (michaela) 2005-10-14 19:30:08 Ok, I fixed the formatting issues (c++ style comments and indenting) and uploaded the new patch.  Mantis won't let me remove the old file so I've named the new one app_voicemail.c.fixed_format.patch. By: BJ Weschke (bweschke) 2005-10-17 10:55:24 Formatting looks good. I apologize, and I should have told you this one at the first go around, but I'm only just noticing it now... Please consider the use of ast_copy_string() instead of memcpy in your patch. Aside from that, I think you're good to go from everything I can see. By: Michael Anderson (michaela) 2005-10-17 12:26:49 Ok, memcpy was changed to ast_string_copy.  Note that the original code used memcpy. By: Mark Spencer (markster) 2005-10-17 18:43:03 I cannot see how this would occur with the code in cvs head already. Can you provide a sample config (extensions.conf, voicemail.conf, and a brief description of what you did to cause it)? By: BJ Weschke (bweschke) 2005-10-17 20:12:12 markster: I think the problem here in this function of app_voicemail is really in line 650. True, a strchr is being done for the presence of ';' three lines above, but then in 650, regardless of whether it's there or not, the user pointer is getting reset with the original value of the line read from voicemail.conf minus the line feed. I also could be totally misreading these C pointer operations too. That's a distinct possibility :-), but the issue did seem like it could happen in a quick review of the existing code. By: Michael Anderson (michaela) 2005-10-17 20:25:30 Well the relevant extensions.conf section is: exten => *97,1,Answer exten => *97,2,Wait(1) exten => *97,3,VoicemailMain(${CALLERIDNUM}@default) exten => *97,4,Macro(hangupcall) and the voicemail.conf section is similar to: [default] ;MBOX => PASSWORD[,Firstname Lastname[,email[,[pager][,options]]]] 123 => 1111,John Smith,jsmith@company.com,,attach=yes With this setup a change to voicemail password for 123 fails. From my read of the original code it should have ignored the comment, but I added logging statements to see what was happening to the context and found that the comment was still being parsed as a context.  I did not try adding white space in front of the comment, that may have fixed it as well. Unfortunatly I have lost the original comment as the first thing I did when I discovered the problem was to remove the comment. By: BJ Weschke (bweschke) 2005-10-17 20:41:52 markster: with regard to your earlier comment, this isn't an issue with the ast_config_* framework, but rather, the code inside the function that rewrites voicemail.conf when a vm password is attempting to be changed. By: BJ Weschke (bweschke) 2005-10-31 08:03:25.000-0600 Housekeeping By: Kevin P. Fleming (kpfleming) 2005-10-31 16:48:43.000-0600 This patch does not apply to CVS HEAD, and I don't see how any of the recent changes in app_voicemail.c are causing that... Can you try to get it updated for current CVS HEAD? Thanks. By: Michael Anderson (michaela) 2005-10-31 18:24:03.000-0600 kpfleming, the app_voicemail.c.HEAD.patch file should address your concerns.  It is diffed against cvs HEAD as of now. By: Kevin P. Fleming (kpfleming) 2005-10-31 18:49:40.000-0600 Committed to CVS HEAD, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:53:09.000-0600 Repository: asterisk Revision: 6917 U   trunk/apps/app_voicemail.c ------------------------------------------------------------------------ r6917 | kpfleming | 2008-01-15 15:53:08 -0600 (Tue, 15 Jan 2008) | 2 lines handle comments containing what appear to be context names during voicemail.conf updates better (issue ASTERISK-5240) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=6917