[asterisk-dev] [Code Review] 3954: pjsip_options.c: Fix race condition stopping periodic out of dialog OPTIONS request.

[Mark Michelson]

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



/branches/13/res/res_pjsip.c
<https://reviewboard.asterisk.org/r/3954/#comment23715>

    The approach taken here is incorrect. send_request_cb() has a pjsip_event passed to it that we currently are assuming is always PJSIP_EVENT_TSX_STATE, with the transaction state change being caused by event PJSIP_EVENT_RX_MSG. However, for the cases you mention where the callback is called during an error, the event is likely PJSIP_EVENT_TRANSPORT_ERROR, or it is PJSIP_EVENT_TSX_STATE with the transaction state change being caused by PJSIP_EVENT_TRANSPORT_ERROR. We should be handling this event in send_request_cb() by logging the error and immediately returning. That way, there is no risk that send_request_cb() has unreffed req_data, and since the user's callback has not been called, there is no risk that user data has been freed either.
    
    By doing this, an error return for pjsip_endpt_send_request() can safely unref req_data and return an error. Callers of send_out_of_dialog_request() can safely unref/free their data on an error return as well.



/branches/13/res/res_pjsip/pjsip_options.c
<https://reviewboard.asterisk.org/r/3954/#comment23716>

    I don't understand the race condition you're referring to here since the scheduler has been altered to deal with an external deletion of the currently running task.
    
    Also, switching from a variable scheduler callback means that if the qualify frequency of a contact is changed, we will not switch to the new qualify frequency.


- Mark Michelson


On Sept. 3, 2014, 2:40 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3954/
> -----------------------------------------------------------
> 
> (Updated Sept. 3, 2014, 2:40 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: AFS-155 and ASTERISK-24295
>     https://issues.asterisk.org/jira/browse/AFS-155
>     https://issues.asterisk.org/jira/browse/ASTERISK-24295
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The crash on the issues is a result of an invalid transport configuration change when asterisk is restarted.  The attempt to send the qualify request fails and we cleaned up.  However, the callback is also called which results in a double unref of the objects involved.
> 
> * Fixed send_out_of_dialog_request() to not return error or cleanup resources if pjsip_endpt_send_request() is not successful.
> 
> * Fix periodic endpoint qualify OPTIONS sched deletion race by avoiding it.  The sched entry will no longer self stop and must be externally stopped.
> 
> * Added REF_DEBUG description tags to struct sched_data in pjsip_options.c.
> 
> * Fix some off-nominal ref leaks in schedule_qualify(), qualify_and_schedule().
> 
> * Reordered pjsip_options.c module start/stop code to cleanup better on error.
> 
> 
> Diffs
> -----
> 
>   /branches/13/res/res_pjsip/pjsip_options.c 422562 
>   /branches/13/res/res_pjsip.c 422562 
> 
> Diff: https://reviewboard.asterisk.org/r/3954/diff/
> 
> 
> Testing
> -------
> 
> * With the qualify_frequency option enabled, added and removed a "local_net=" line in the transport section and restarted asterisk via "core restart now".  Before the latest patch version, asterisk would crash.  With the new patch, it keeps on going.
> 
> * Set the qualify_frequency option to different values and reloaded res_pjsip each time.  The OPTIONS poll frequency changed, started, and stopped according to the new qualify_frequency value.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140903/0d7fcd81/attachment-0001.html>