[asterisk-dev] [Code Review] New bug causes reception of 487 response to INVITE not to be ACKed.

[Joshua Colp]

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

Ship it!



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/308/#comment2290>

    While I'm not totally thrilled over doing this... since this will hopefully never happen and there is no other way then it is fine.


- Joshua


On 2009-07-09 09:45:21, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/308/
> -----------------------------------------------------------
> 
> (Updated 2009-07-09 09:45:21)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The fix from review 298 has exposed a new bug in chan_sip.
> 
> When we hang up an outgoing call, we first will dump all the outstanding packets on the sip_pvt using __sip_pretend_ack. Then, if we can, we send a CANCEL. The problem with this is that since destroyed all the outstanding packets on the dialog, we cannot match the incoming 487 response to our INVITE. Because we cannot match the response, we do not send an ACK.
> 
> To correct this, instead of using __sip_pretend_ack, I have changed the code to loop through the list of packets and call __sip_semi_ack on each one instead. This causes us to stop retransmitting the requests, but we still have them around in case we get responses for them.
> 
> When discussing this earlier today with Josh Colp, we both agreed that in the majority of cases, this would be enough of a fix. However, we also agreed that we should have a safety net in place in case we never receive a response to our initial INVITE. To handle this, I have modified __sip_autodestruct to behave similar to the way it does in Asterisk 1.4. If there are outstanding packets on the sip_pvt, the needdestroy flag is not set, and the last request on the dialog was either a CANCEL or BYE, then we set the needdestroy flag and reschedule destruction for 10 seconds in the future. If, though, the needdestroy flag is set, then we use __sip_pretend_ack to kill the remaining outstanding packets so that the monitor thread can destroy the sip_pvt.
> 
> This review is for Asterisk trunk. The same fixes will need to be applied to all the other branches, including Asterisk 1.4. Even though there is similar code in __sip_autodestruct in 1.4 already, I have confirmed that its logic is not 100% correct, the result being that we can leak memory if communicating with an endpoint that does not send a response to our INVITE if we cancel it.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 205349 
> 
> Diff: https://reviewboard.asterisk.org/r/308/diff
> 
> 
> Testing
> -------
> 
> I ran two separate tests. First, I placed a call from my Aastra phone to my Polycom phone. I hung up the Aastra before the Polycom answered. I verified through sip debug output that Asterisk properly ACKed the 487 received from the Polycom.
> 
> For my second test, I set up a SIPp UAS scenario so that it would send a 200 OK in response to a CANCEL but would not send a 487 for the original INVITE. I verified that after about 40 seconds, Asterisk properly cleans up the outgoing sip_pvt for the call.
> 
> 
> Thanks,
> 
> Mark
> 
>