Summary:	ASTERISK-05222: [patch] Tone definitions in indications.conf not properly validated
Reporter:	clint (clint)	Labels:
Date Opened:	2005-10-03 10:27:35	Date Closed:	2005-11-08 21:18:06.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/Configuration
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) indications-102505-2030.patch
Description:	Tone definitions in indications.conf containing more than two tones to be played simultaneously are accepted, but cause incorrect behavior. Tone definitions containing invalid data should be rejected at load time, not result in Asterisk playing unpredictable tones indefinitely.
Comments:	By: clint (clint) 2005-10-03 12:43:43 Actually, that's not quite correct.  This should be validated at play time.  The current validation is insufficient to catch an improperly formatted tone string, defaulting to trying to play the first part of the string as a tone.  If a definition falls through the string matching in the tone parser, it should be errored out, not munged and played. By: Olle Johansson (oej) 2005-10-24 03:07:58 Any other opinions from someone else in this matter? Any coders that want to fix this? /Housekeeping By: Jason Parker (jparker) 2005-10-24 16:39:20 Now, I don't know exactly how sscanf works, but I imagine that unless something is explicitly matched in char *format, it will simply not match. I assume that passing 250+500+1000 to (sscanf(s, "%d+%d", &freq1, &freq2) == 2) would do one of three things. a) simply not match - this is what I would expect b) match "500+1000", and ignore the "250+" c) match "250+500", and ignore the "+1000" I can test this when I get home tonight, if anybody would like. If the result is a, then this is probably a non issue. By: Jason Parker (jparker) 2005-10-25 01:05:37 Make that tomorrow... By: Jason Parker (jparker) 2005-10-25 22:33:13 It was c...sorta. okay, I suck.  Anybody have any suggestions on how this can be done better? (patch is disclaimed...etc, etc) By: Kevin P. Fleming (kpfleming) 2005-10-31 19:08:50.000-0600 This patch can't be the right solution, because it assumes that the input format is always exactly the same as what snprintf() would generate, which does not necessarily need to be the case. Can you give some examples of what is failing to be caught? By: Jason Parker (jparker) 2005-10-31 19:37:28.000-0600 Kevin: Personally, I don't agree with the bug - I just submitted a (crappy at best) patch. Basically, the problem is that when there is a configuration issue, asterisk fails to parse it properly. Example1: 440+480+520 - sscanf will match on 440+480, so freq1 becomes 440, freq2 becomes 480, and "+520" is simply discarded. Example2: 440+480/2000/4000 - I haven't tested this scenario, but I imagine it would make freq1=440, freq2=480, time=2000, and discard the "/4000". In my opinion, this could probably be considered a configuration issue.  There is no reason that somebody would/should have "invalid" tone strings.  Did that help any? By: Kevin P. Fleming (kpfleming) 2005-11-08 21:17:55.000-0600 I'm going to close this one for now... without writing our own parser to replace sscanf() with, there is no straightforward way to solve this problem. Given that this particular configuration file is rarely edited by users, this is truly a pretty minor issue that doesn't warrant a great deal of effort in that area... If someone wants to contribute a well-tested patch that handles all the known cases, we can consider it.