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

[rmudgett]

> On Sept. 10, 2014, 6:51 a.m., Joshua Colp wrote:
> > /branches/13/res/res_pjsip.c, line 2443
> > <https://reviewboard.asterisk.org/r/3954/diff/3/?file=67221#file67221line2443>
> >
> >     unsigned int

Sure, if you want.


> On Sept. 10, 2014, 6:51 a.m., Joshua Colp wrote:
> > /branches/13/res/res_pjsip.c, lines 2478-2480
> > <https://reviewboard.asterisk.org/r/3954/diff/3/?file=67221#file67221line2478>
> >
> >     I'm hesitant to say that this will always be true. I fear that due to the asynchronous nature of things (was this tested with TCP for example?) that the sending of the request may not block and return an error, but it would inevitably end up with a transport error callback.
> >     
> >     An example: A target using TCP transport that has no existing established connection.

I know that pjsip_endpt_send_request() will return failure and call the callback for normal errors.  I also know from looking at the pjproject code that pjsip_endpt_send_request() can return error and not call the callback.  Either we assume that pjsip_endpt_send_request() will _always_ call the callback when it returns error and do what I did in version 2 of the patch or we go this way.  One way or the other we are going to leak something or the returned error codes need to be mapped to which ones indicate that the callback will happen.


- rmudgett


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


On Sept. 4, 2014, 6:41 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3954/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2014, 6:41 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_request_cb() and qualify_contact_cb() to not cleanup the token when the PJSIP event is PJSIP_EVENT_TRANSPORT_ERROR since the initial function call will do the clean up.
> 
> * Made send_request_cb() able to handle repeated challenges (Up to 10).
> 
> * 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 422583 
>   /branches/13/res/res_pjsip.c 422583 
> 
> 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/20140910/6f2b0a59/attachment-0001.html>