[asterisk-dev] [Code Review]: Resolve Sequence Number Rollover causing codec increase in res_rtp_multicast

[Alec Davis]

> On Oct. 26, 2011, 6:54 p.m., Alec Davis wrote:
> > /branches/1.8/res/res_rtp_multicast.c, line 231
> > <https://reviewboard.asterisk.org/r/1542/diff/1/?file=21405#file21405line231>
> >
> >     unrelated but did you see this.
> >     (0 << 23)
> >     
> >     effectively does nothing right?
> >
> 
> Alec Davis wrote:
>     in the root of asterisk source run this:
>     
>     grep -r '0 <<' *
> 
> wdoekes wrote:
>     Well.. here it serves a documentation purpose:
>     
>     /* DTMF flags - see str2dtmfmode() and dtmfmode2str() */
>     #define SIP_DTMF            (7 << 15) /*!< DP: DTMF Support: five settings, uses three bits */
>     #define SIP_DTMF_RFC2833    (0 << 15) /*!< DP: DTMF Support: RTP DTMF - "rfc2833" */
>     #define SIP_DTMF_INBAND     (1 << 15) /*!< DP: DTMF Support: Inband audio, only for ULAW/ALAW - "inband" */
>     #define SIP_DTMF_INFO       (2 << 15) /*!< DP: DTMF Support: SIP Info messages - "info" */
>     #define SIP_DTMF_AUTO       (3 << 15) /*!< DP: DTMF Support: AUTO switch between rfc2833 and in-band DTMF */
>     #define SIP_DTMF_SHORTINFO  (4 << 15) /*!< DP: DTMF Support: SIP Info messages - "info" - short variant */
>     
>     In put_unaligned_uint32() it's not clarifying anything for me. Perhaps if you change it to:
>     
>     (0/*mark*/ << 23)
>

if (0/*mark*/ << 23) is trying to set bit23, then it's not going to. The result is 0.

It should perhaps be (1 </*mark*/ << 23) where the lsb is bit0.

If it's documentation, then it's still not serving a very usefull purpose.


- Alec


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1542/#review4586
-----------------------------------------------------------


On Oct. 27, 2011, 12:44 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1542/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2011, 12:44 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Basically what was happening was the sequence number would go above 65535, and rather than rolling back to zero, the extra bits would overflow into the codec bits of the RTP packet.  Because of this, if the sequence number goes above 65535, it'll make the codec value change and this can make calls fail.
> 
> This patch simply checks the value of the sequence number each time it is incremented and sets it back to 0 if it's above 65535.  Shouldn't cause any problems.
> 
> 
> This addresses bug ASTERISK-18291.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18291
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/res/res_rtp_multicast.c 341429 
> 
> Diff: https://reviewboard.asterisk.org/r/1542/diff
> 
> 
> Testing
> -------
> 
> I checked the unpatched version with wireshark to make sure the codec shift was in fact happening and it was.  I checked it again after patching and made sure the problem went away and it did.  I also checked to see whether the values stayed as expected if I forcibly changed the codec value to something other than 0, and in all cases it behaved as expected.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111027/5d44a665/attachment-0001.htm>