[Home]

Summary:ASTERISK-16304: [patch] [regression] RFC 2833 frame out of order detection does not properly handle numeric overflow
Reporter:mdeneen (mdeneen)Labels:
Date Opened:2010-06-30 10:24:54Date Closed:2010-07-06 09:33:17
Priority:BlockerRegression?Yes
Status:Closed/CompleteComponents:Core/RTP
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) rtp_seqno_rollover.patch
Description:SVN revision r254482 added code to detect out of sequence RTP frames, and to discard them.  However, at least with my ITSP, the RTP sequence is a 16 bit unsigned number.  It is possible to get into a situation where the check fails because the RTP sequence number rolls back around to 0 as the DTMF frame comes in.

In this situation, the last sequence number (rtp->lastevent) is not updated and further DTMF frames are ignored.

For example, rtp->lastevent might be 65535.  The RTP sequence can never be greater than this, so no further DTMF will be processed.

Instead of doing:

if (rtp->lastevent > seqno) {
   return;
}
Perhaps it should not fall into the "out of order" branch if the difference between the last sequence and the current sequence is greater than a chosen reasonable number.


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

I'm setting this to major because it is possible for callers to get stuck, seemingly at random.  If you feel that this is an over-reaction, feel free to change the severity.
Comments:By: mdeneen (mdeneen) 2010-06-30 11:41:56

This problem also affects the 1.4 branch, introduced in r254452.
By: Paul Belanger (pabelanger) 2010-06-30 12:38:10

Downgrading: Severity only references the amount of people affected
By: mdeneen (mdeneen) 2010-06-30 14:43:17

I have a feeling that this would impact everyone using rfc 2833, given enough time.  I'm not intimate with RTP sequences, so perhaps 32 bit sequences are more common these days.


By: richardf (richardf) 2010-07-01 01:35:44

I can reproduce this issue on trunk r272961. It manifests itself as dtmf not working at all on a call after approx 10min. Mainly affects long Voicemail calls.
By: mdeneen (mdeneen) 2010-07-01 08:45:22

I modified main/rtp.c to add some logging such that it would write out the rtp sequence whenever we receive DTMF.

Interestingly enough, a new call for me does not start at sequence 0.  It appears to be somewhat random.  In this scenario, one could hit the rollover almost immediately without having to wait for the sequence to flow from 0->65535.

If you set up a simple dial plan so that it never hangs up, and sit there pressing a button on the phone, eventually it will stop receiving DTMF for the rest of the call.
By: jmls (jmls) 2010-07-02 01:17:01

I can confirm that we have this problem as well, mainly manifested during using the playback application. It is driving our users mad (1.4 svn trunk)
By: Max_cnu (maxochoa) 2010-07-02 09:46:43

Experienced this same problem using app_queue (issue 17545) - had not tested other conditions, given long calls are rare in our environment.
By: Mark Michelson (mmichelson) 2010-07-02 10:43:11

I've uploaded a simple patch which implements the idea of using a windowed approach to determine if the seqno has rolled over.

richardf and I discussed on IRC a more deterministic method of doing so using the timestamps from the RTP events instead of the seqno. However, this is prone to error due to SSRC changes plus the fact the timestamp itself can roll over (though admittedly much much much less frequently). Thus I have chosen to go with the easier, less error-prone method that will likely work 99.99999% of the time :)
By: Max_cnu (maxochoa) 2010-07-02 12:27:22

This patch resolved the issue in testing using an agent in app_queue with durations just over 20 minutes.
By: Joe Cracchiolo (jjcinaz) 2010-07-02 17:46:57

I built a test with SIPp using a known RTP stream exhibiting the problem.  Confirmed patch solves issue.
By: jmls (jmls) 2010-07-03 00:35:43

is there a patch for 1.4 ?
By: Paul Belanger (pabelanger) 2010-07-03 10:07:56

All branches.
By: richardf (richardf) 2010-07-03 22:27:25

Confirmed that patch resolves issue on latest trunk and doesn't break anything else as far as I can tell.


By: Paul Belanger (pabelanger) 2010-07-04 11:05:47

Promoted to 'Ready for Review'
By: Digium Subversion (svnbot) 2010-07-06 09:29:22

Repository: asterisk
Revision: 274157

U   branches/1.4/main/rtp.c

------------------------------------------------------------------------
r274157 | mmichelson | 2010-07-06 09:29:22 -0500 (Tue, 06 Jul 2010) | 16 lines

Fix problem with RFC 2833 DTMF not being accepted.

A recent check was added to ensure that we did not erroneously
detect duplicate DTMF when we received packets out of order.
The problem was that the check did not account for the fact that
the seqno of an RTP stream will roll over back to 0 after hitting
65535. Now, we have a secondary check that will ensure that the
seqno rolling over will not cause us to stop accepting DTMF.

(closes issue ASTERISK-16304)
Reported by: mdeneen
Patches:
     rtp_seqno_rollover.patch uploaded by mmichelson (license 60)
Tested by: richardf, maxochoa, JJCinAZ


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

http://svn.digium.com/view/asterisk?view=rev&revision=274157
By: Digium Subversion (svnbot) 2010-07-06 09:31:13

Repository: asterisk
Revision: 274164

_U  trunk/
U   trunk/res/res_rtp_asterisk.c

------------------------------------------------------------------------
r274164 | mmichelson | 2010-07-06 09:31:12 -0500 (Tue, 06 Jul 2010) | 22 lines

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

........
 r274157 | mmichelson | 2010-07-06 09:29:23 -0500 (Tue, 06 Jul 2010) | 16 lines
 
 Fix problem with RFC 2833 DTMF not being accepted.
 
 A recent check was added to ensure that we did not erroneously
 detect duplicate DTMF when we received packets out of order.
 The problem was that the check did not account for the fact that
 the seqno of an RTP stream will roll over back to 0 after hitting
 65535. Now, we have a secondary check that will ensure that the
 seqno rolling over will not cause us to stop accepting DTMF.
 
 (closes issue ASTERISK-16304)
 Reported by: mdeneen
 Patches:
       rtp_seqno_rollover.patch uploaded by mmichelson (license 60)
 Tested by: richardf, maxochoa, JJCinAZ
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=274164
By: Digium Subversion (svnbot) 2010-07-06 09:33:15

Repository: asterisk
Revision: 274168

_U  branches/1.6.2/
U   branches/1.6.2/main/rtp.c

------------------------------------------------------------------------
r274168 | mmichelson | 2010-07-06 09:33:15 -0500 (Tue, 06 Jul 2010) | 29 lines

Merged revisions 274164 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r274164 | mmichelson | 2010-07-06 09:31:13 -0500 (Tue, 06 Jul 2010) | 22 lines
 
 Merged revisions 274157 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r274157 | mmichelson | 2010-07-06 09:29:23 -0500 (Tue, 06 Jul 2010) | 16 lines
   
   Fix problem with RFC 2833 DTMF not being accepted.
   
   A recent check was added to ensure that we did not erroneously
   detect duplicate DTMF when we received packets out of order.
   The problem was that the check did not account for the fact that
   the seqno of an RTP stream will roll over back to 0 after hitting
   65535. Now, we have a secondary check that will ensure that the
   seqno rolling over will not cause us to stop accepting DTMF.
   
   (closes issue ASTERISK-16304)
   Reported by: mdeneen
   Patches:
         rtp_seqno_rollover.patch uploaded by mmichelson (license 60)
   Tested by: richardf, maxochoa, JJCinAZ
 ........
................

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

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