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

[Mark Michelson]

> On 2009-06-26 11:43:42, dimas wrote:
> >

Thanks for the comments dimas!


> On 2009-06-26 11:43:42, dimas wrote:
> > /branches/1.4/channels/chan_sip.c, line 1791
> > <http://reviewboard.digium.com/r/295/diff/1/?file=5740#file5740line1791>
> >
> >     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_.

You're correct that the name is a bit confusing. I actually wrote this function with the intention that if, for some reason, we incremented our outgoing sequence number by more than 1 somewhere, the function could handle it. When I actually searched the code, it turns out that we only ever increment the CSeq by 1. I could probably just remove the second parameter from this function.


> On 2009-06-26 11:43:42, dimas wrote:
> > /branches/1.4/channels/chan_sip.c, line 16141
> > <http://reviewboard.digium.com/r/295/diff/1/?file=5740#file5740line16141>
> >
> >     Ins't the (p->ocseq != seqno) check already done by in_ocseq_history?

Yes, you are correct.


> On 2009-06-26 11:43:42, dimas wrote:
> > /branches/1.4/channels/chan_sip.c, line 1783
> > <http://reviewboard.digium.com/r/295/diff/1/?file=5740#file5740line1783>
> >
> >     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).

The shift is guaranteed to not be negative since we already know prior to calling shift_ocseq_history that p->ocseq >= seqno. When shifting more than the number of bits in the value, the spec guarantees that 0s will be shifted into the value. This means that if we should attempt to shift more than the bit-width of p->ocseqhistory, then the value we compare to will be 0.

In short, the line you have highlighted is safe.


- Mark


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


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