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

[dimas at dataart.com]

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



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

    To be honest I haven't read C specs so I do not know how << behaves for large  (exceeding number of bits in the value) or negative shifts. So maybe it is not an issue at all. But if I were you, I would first check that seqno delta is meaningful (positive and not exceeding bit width).



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

    What is the sense in having seqno parameter here? Both invocations have it set to the same value (1) also it is name is a bit confusing because for previous two functions it is real seqno while here it is actually a seqno _increment_.



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

    Ins't the (p->ocseq != seqno) check already done by in_ocseq_history?


- dimas


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
> 
>