[asterisk-dev] [Code Review] A potential fix for issue 11231

[Sean Bright]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/295/#review885
-----------------------------------------------------------


Awesome!  Just a minor suggestion that you are free to ignore.


/branches/1.4/channels/chan_sip.c
<http://reviewboard.digium.com/r/295/#comment2140>

    Probably unnecessary, but couldn't this be:
    
       p->ocseqhistory = (p->ocseqhistory << seqno) | 1;


- Sean


On 2009-06-25 19:15:46, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/295/
> -----------------------------------------------------------
> 
> (Updated 2009-06-25 19:15:46)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> First go read issue 11231. I'll wait.
> 
> A problem that has plagued Asterisk's SIP implementation for quite a while is the fact that we only keep track of the latest outgoing CSeq number that we used in a dialog. So if we send out several requests, we will ignore responses for all but the last of the requests we sent. I believe I have found a simple solution to the problem.
> 
> With this change, there is a new field added to the sip_pvt struct called ocseqhistory. It is a bitfield. Bit 0 represents the current ocseq. Each bit beyond represents an old CSeq we have used in outgoing responses on the dialog. Set bits represent outstanding responses.
> 
> The easiest way to explain is with an example, let's pretend that the ocseqhistory is an 8-bit field. Initially, it is all 0's
> 00000000
> When we send our first outgoing request, we set bit 0.
> 00000001
> When we send out each new request beyond the first, we shift the ocseqhistory by one and set bit 0 again. So if we were to send out two more requests, ocseqhistory would look like this:
> 00000111
> 
> Now, let's imagine that due to network oddities, we get the response to our second request first. We examine the CSeq of the response we just received and subtract it from our saved ocseq. It is 1. Since bit 1 of ocseqhistory is set, we do not ignore the response. Further, since we have now received a response, we clear the bit in ocseqhistory representing our outstanding request. Now ocseqhistory looks like this:
> 00000101
> 
> By doing this, we can keep track of old outgoing sequence numbers in a simple, lightweight manner. The only way this can be a problem is if someone has set up Asterisk to send out more than the number of bits in ocseqhistory at once, which is very unlikely to happen.
> 
> Potential bug:
> In the current version of the patch, I unconditionally clear the bit in the ocseqhistory when receiving a response. This should really only be done if the response is a final response since we can receive multiple provisional responses potentially.
> 
> 
> This addresses bug 11231.
>     https://issues.asterisk.org/view.php?id=11231
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_sip.c 202966 
> 
> Diff: http://reviewboard.digium.com/r/295/diff
> 
> 
> Testing
> -------
> 
> I used the following dialplan to make Asterisk rapidly send MESSAGE requests to my phone:
> 
> exten => 15,1,Answer
> exten => 15,n,SendText(ABC)
> exten => 15,n,SendText(ABC)
> exten => 15,n,SendText(ABC)
> exten => 15,n,SendText(ABC)
> exten => 15,n,Wait(20)
> exten => 15,n,Hangup
> 
> Unpatched, this caused a storm of retransmits. With this patch applied, Asterisk sent four MESSAGE requests, received four responses, and did not send any unnecessary retransmissions.
> 
> 
> Thanks,
> 
> Mark
> 
>