[asterisk-dev] [Code Review] 4233: res_pjsip_session: Fix crash that would occur when rescheduling a reinvite due to a 491 response.

[Matt Jordan]

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

Ship it!



/branches/12/res/res_pjsip_session.c
<https://reviewboard.asterisk.org/r/4233/#comment24404>

    Completely ancillary to this... but I don't see where this gets used anywhere. :-)


- Matt Jordan


On Dec. 4, 2014, 3:22 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4233/
> -----------------------------------------------------------
> 
> (Updated Dec. 4, 2014, 3:22 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24556
>     https://issues.asterisk.org/jira/browse/ASTERISK-24556
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> A reporter discovered that Asterisk would crash when attempting to retransmit a reinvite that had previously received a 491 response. The crash occurred because a pjsip_tx_data structure was being saved for reuse, but its reference count was not being increased. The result was that the pjsip_tx_data was being freed before we were actually done with it. When we attempted to re-use the structure when re-sending the reinvite, Asterisk would crash.
> 
> The fix implemented here is not to try holding onto the pjsip_tx_data at all. Instead, when we reschedule sending the reinvite, we create a brand new pjsip_tx_data and send that instead. Because of this change, there is no need for an ast_sip_session_delayed_request structure to have a pjsip_tx_data on it any more. So any code referencing its use has been removed.
> 
> When this initial fix was introduced, I encountered a second crash when processing a subsequent 200 OK on a rescheduled reinvite. The reason was that when rescheduling the reinvite, we gave the wrong location for a response callback. This has been fixed in this patch as well.
> 
> (Feel free to stop reading at this point. The rest of this is justification for the way this is being fixed)
> 
> My initial inclination when fixing this was to fix it by bumping the reference count of the pjsip_tx_data structure, and if necessary, decrementing its refcount when we are finished with it. Unfortunately, PJSIP does not allow for this. This is because when we initially send our reinvite, PJSIP creates a transaction and registers the transaction using a key based on the branch parameter. When we receive the 491 response, PJSIP sends an ACK but then keeps the transaction alive for a while longer. This is so that if our outbound ACK is lost, and we receive the 491 again, we can retransmit the ACK. Since the transaction is still present, when we attempt to retransmit the reinvite, PJSIP complains that a transaction with the branch we've given it already exists. Thinking about it a bit further, I realized that in addition to the re-used branch, we also were not increasing the CSeq value as we should when sending out the reinvite again. Thus, holding onto the pjsip_tx_data was a bad solution. Just to be certain about the rules about this, I consulted RFC 3261 to see what it says about re sending the reinvite. It says, "... the UAC SHOULD attempt the re-INVITE once more, if it still desires for that session modification to take place." The wording here is "attempt the re-INVITE once more", which does not mean to retransmit the same reinvite. So sending a new reinvite is perfectly fine.
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/res_pjsip_session.c 428420 
> 
> Diff: https://reviewboard.asterisk.org/r/4233/diff/
> 
> 
> Testing
> -------
> 
> See /r/4234
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141204/2ca20e55/attachment.html>