Summary:	ASTERISK-16215: [patch] Crash in dsp.c when entering digits from SpeechBackground
Reporter:	Richard Kenner (kenner)	Labels:
Date Opened:	2010-06-05 11:36:21	Date Closed:	2010-06-05 12:56:56
Priority:	Critical	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) dsp_digitlen_fix.patch ( 1) dsp.c.diff
Description:	The field current_len is set to zero and decremented, but never incremented in dsp.c.  But its used as the operand of memmove, so the second time the code in question is executed, memmove is passed an operand of -1, which causes a crash.  I have a patch, which fixes the problem, but I don't understand the code enough to be completely confident that it's correct.
Comments:	By: Lott Caskey (lottc) 2010-06-05 12:11:22 Is this related to 17371? By: Richard Kenner (kenner) 2010-06-05 12:46:51 Yes, that seems like the identical problem.  The patch there seems incorrect to me, but it might be worth seeing if the patch here fixes that issue.  I think there needs to be a code cleanup here: if current_digits and current_len are really to mean different things, there needs to be better documentation saying what each means and a better accounting for their values. By: Lott Caskey (lottc) 2010-06-05 12:47:09 I am pretty sure you are onto something, but you are correct in that your patch isn't quite correct...  And someone may correct me here, but current_len is the total sample length, and thus may exceed MAX_DTMF_DIGITS+1 (the size of datalen). By: Richard Kenner (kenner) 2010-06-05 12:54:00 No, current_len right now (pre any patch) is set to zero and then goes DOWN, so it's certainly not anything like "the total sample length".  Moreover, no more than MAX_DTMF_DIGITS can be saved: see dsp.c:store_digits. The question that I have is: in what ways did the author intend current_digits and current_len to differ?  I'd have expected to see a comment near the declaration of those fields to explain that, but without that comment I have no idea and can only propose the patch I did, which has the effect of making them always have the same value. That's what I mean by it can't be correct: there's no point in having two fields which always have the same value.  But it's CERTAINLY incorrect to have a field being used as the size in a memmove which is never positive! By: Lott Caskey (lottc) 2010-06-05 12:54:57 I have uploaded dsp_digitlen_fix.patch, which I believe is a more correct implementation, however, I have concerns about how current_len is being calculated.  I think there is a larger fundamental problem here... By: Digium Subversion (svnbot) 2010-06-05 12:55:30 Repository: asterisk Revision: 268456 U   trunk/main/dsp.c ------------------------------------------------------------------------ r268456 | tilghman | 2010-06-05 12:55:27 -0500 (Sat, 05 Jun 2010) | 14 lines Fix crash in DTMF detection. What I did not originally see in my previous commit was that even though the next digit could be detected before the previous was considered ended, the detection of the next digit effectively ends the detection of the previous. Therefore, the length moves in lockstep with the digit, and no separate counter is needed for the length alone. (closes issue ASTERISK-16125) Reported by: alecdavis (closes issue ASTERISK-16215) Reported by: kenner ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=268456 By: Digium Subversion (svnbot) 2010-06-05 12:56:56 Repository: asterisk Revision: 268457 _U  branches/1.6.2/ U   branches/1.6.2/main/dsp.c ------------------------------------------------------------------------ r268457 | tilghman | 2010-06-05 12:56:54 -0500 (Sat, 05 Jun 2010) | 21 lines Merged revisions 268456 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........  r268456 | tilghman | 2010-06-05 12:55:28 -0500 (Sat, 05 Jun 2010) | 14 lines    Fix crash in DTMF detection.    What I did not originally see in my previous commit was that even though the  next digit could be detected before the previous was considered ended, the  detection of the next digit effectively ends the detection of the previous.  Therefore, the length moves in lockstep with the digit, and no separate counter  is needed for the length alone.    (closes issue ASTERISK-16125)   Reported by: alecdavis    (closes issue ASTERISK-16215)   Reported by: kenner ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=268457