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

[jrose]

> On Oct. 26, 2011, 4:17 p.m., rmudgett wrote:
> > /branches/1.8/res/res_rtp_multicast.c, lines 235-236
> > <https://reviewboard.asterisk.org/r/1542/diff/1/?file=21405#file21405line235>
> >
> >     You could just bitwise and it:
> >     x = x & 0xFFFF
> >     since it is used as a bit value anyway.
> >     It may even be faster than the modulo division.
> 
> wdoekes wrote:
>     I'm thinking the compiler will optimize things here.
>     
>     But &0xFFFF is better for two reasons:
>     (1) You immediately see that you want exactly 16bits, instead of looking at some random decimal number.
>     (2) The modulo should've been 65536, not 65535.

Right, I knew it was due to overflow and as for the 65536 bit, yeah, that was just another screwup on my part.  I wasn't especially thinking that this patch was the best way to do it anyway and just wanted to get a little feedback before going ahead with it.  That being the case, thanks.


- jrose


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


On Oct. 26, 2011, 3:35 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1542/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2011, 3:35 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/4a094e82/attachment-0001.htm>