[asterisk-bugs] [Asterisk 0012955]: suspected typo in main/rtp.c bridge_p2p_rtp_write() payload type check (can cause RFC2833 DTMF detection issues)

[noreply at bugs.digium.com]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=12955 
====================================================================== 
Reported By:                tonyredstone
Assigned To:                
====================================================================== 
Project:                    Asterisk
Issue ID:                   12955
Category:                   Core/RTP
Reproducibility:            always
Severity:                   minor
Priority:                   normal
Status:                     new
Asterisk Version:           1.4.21 
SVN Branch (only for SVN checkouts, not tarball releases): N/A 
SVN Revision (number only!):  
Disclaimer on File?:        N/A 
Request Review:              
====================================================================== 
Date Submitted:             06-30-2008 10:44 CDT
Last Modified:              07-09-2008 05:06 CDT
====================================================================== 
Summary:                    suspected typo in main/rtp.c bridge_p2p_rtp_write()
payload type check (can cause RFC2833 DTMF detection issues)
Description: 
Hi,

Near the top of bridge_p2p_rtp_write(), there is code that reads as
follows:
      /* If the payload coming in is not one of the negotiated ones
then send it to the core, this will cause formats to change and the
bridge to break */
       if (!bridged->current_RTP_PT[payload].code)
               return -1;

The value of payload is from the inbound leg, however, (struct ast_rtp *)
bridged is the RTP struct for the *outbound* leg.  I believe this code
should read:
       /* If the payload coming in is not one of the negotiated ones
then send it to the core, this will cause formats to change and the
bridge to break */
       if (!rtp->current_RTP_PT[payload].code)
               return -1;

since (struct ast_rtp *)rtp is the structure for the inbound leg so now
the code matches the comment (and it makes sense).

With the code as it currently stands, packet2packet briding sometimes
punts packets to the core unnecessarily and can cause DTMF detection
breakage under certain conditions (see below).

Regards,
-Tony.

patch:
--- main/rtp.c.orig	2008-05-14 22:32:00.000000000 +0100
+++ main/rtp.c	2008-06-20 15:48:44.000000000 +0100
@@ -1063,7 +1063,7 @@
 	rtpPT = ast_rtp_lookup_pt(rtp, payload);

 	/* If the payload coming in is not one of the negotiated ones then
send it to the core, this will cause formats to change and the bridge
to break */
-	if (!bridged->current_RTP_PT[payload].code)
+	if (!rtp->current_RTP_PT[payload].code)
 		return -1;

 	/* If the payload is DTMF, and we are listening for DTMF - then feed
it into the core */
====================================================================== 

---------------------------------------------------------------------- 
 tonyredstone - 07-09-08 05:06  
---------------------------------------------------------------------- 
Hi,

That's not how I read the code...!

Let's step through:
=
        int res = 0, payload = 0, bridged_payload = 0, mark;
        struct rtpPayloadType rtpPT;
        int reconstruct = ntohl(rtpheader[0]);

        /* Get fields from packet */
        payload = (reconstruct & 0x7f0000) >> 16;
        mark = (((reconstruct & 0x800000) >> 23) != 0);

        /* Check what the payload value should be */
        rtpPT = ast_rtp_lookup_pt(rtp, payload);
=

so by this point, "payload" is set to the value of the payload type
received on the inbound leg.  Also, rtpPT is set to an asterisk internal
representation of what the payload type refers to.  Agreed?

the next couple of lines:
=
  /* If the payload coming in is not one of the negotiated ones
then send it to the core, this will cause formats to change and the
bridge to break */
       if (!bridged->current_RTP_PT[payload].code)
               return -1;
=
recall payload is the value of the payload type on the receiving (inbound)
side.  However, here it's used to lookup whether the same payload type
number happens to be in use on the *outbound* leg.  This is nonsensical
because dynamic payload type numbers are only significant within a
particular leg of the call.  To put it another way, there's no reason why
identical RTP media formats (which use dynamic pt numbers) cannot have
completely different payload type numbers on different legs of the call.

let's move on:
=
        /* If the payload is DTMF, and we are listening for DTMF - then
feed it into the core */
        if (ast_test_flag(rtp, FLAG_P2P_NEED_DTMF) && !rtpPT.isAstFormat
&& rtpPT.code == AST_RTP_DTMF)
                return -1;

        /* Otherwise adjust bridged payload to match */
        bridged_payload = ast_rtp_lookup_code(bridged, rtpPT.isAstFormat,
rtpPT.code);
=

The last line here will do the inbound pt -> outbound pt number
translation.

One further point, if transcoding needs to occur, then the two calls legs
will not be packet2packet bridged in the first place (see
ast_rtp_bridge()).

It occurs to me that the if (!bridged->current_RTP_PT[payload].code) test
would be valid if it also checked that payload<96, ie is in the static
payload type range.

Whilst I see more value in changing the check from being against bridged
to rtp, if, instead, we added a payload type range check so that we're only
testing static pt numbers, I suspect it would still solve the DTMF
detection issue caused by asterisk regenerating rfc2833 tones with a
different SSRC id from the RTP media.

so the test could look like this:
if (payload<96 && !bridged->current_RTP_PT[payload].code)
               return -1; 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
07-09-08 05:06  tonyredstone   Note Added: 0089933                          
======================================================================