[asterisk-dev] [Code Review] 4178: res_pjsip_outbound_publish: stack overflow when using non-default sorcery wizard

Kevin Harwell reviewboard at asterisk.org
Tue Nov 18 17:45:34 CST 2014



> On Nov. 15, 2014, 9:22 a.m., Matt Jordan wrote:
> > branches/13/res/res_pjsip_outbound_publish.c, lines 274-275
> > <https://reviewboard.asterisk.org/r/4178/diff/1/?file=68968#file68968line274>
> >
> >     I don't really like the pattern that we have here.
> >     
> >     We technically have two destructor functions:
> >      * sip_outbound_publish_client_destroy, which destroys the state object
> >      * destroy_old_state, which must only be called on a state object once just prior to it being destroyed
> >     
> >     That feels wrong. I would expect that destroy_old_state should just be a part of sip_outbound_publish_client_destroy.
> >     
> >     If sip_outbound_publish_client_destroy actually is the only destructor for the object, *and* the container holds the last reference for the object (assuming it isn't being used elsewhere, in which case reference counting will still work just fine), the you should be able to just destroy the container. That will remove the container's reference to its objects, which will cause their destruction.

The problem that I ran into is that pjsip_publishc object holds a reference to the state until unpublish is called.  This means the state destructor won't get called until after "unpublishing".  "destroy_old_state" is perhaps a bad name for this function.  I'll rename it to something more appropriate like cancel_and_unpublish since that is the intent and add comments to make it more clear.  Reading this part of the code will then make more sense: Old states no longer in use (the config object has been deleted for instance) are cancelled and unpublished (state ref dec held by pjsip_publishc) and then removed from the memory container (state ref dec on old_states container).

The only other way this might work is by adding a wrapper around the state object.  This wrapper is stored in the ao2_container and then when it gets destroyed cancel and unpublish on its associated state gets called thus releasing the state object.  Thoughts?


- Kevin


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


On Nov. 13, 2014, 2:08 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4178/
> -----------------------------------------------------------
> 
> (Updated Nov. 13, 2014, 2:08 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24514
>     https://issues.asterisk.org/jira/browse/ASTERISK-24514
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> When using a non-default sorcery wizard (in this instance realtime) for outbound publishes Asterisk will crash after a stack overflow occurs due to the code infinitely recursing.  The fix entails removing the outbound publish state dependency from the outbound publish sorcery object and instead keeping an in memory container that can be used to lookup the state when needed.
> 
> 
> Diffs
> -----
> 
>   branches/13/res/res_pjsip_outbound_publish.c 427787 
> 
> Diff: https://reviewboard.asterisk.org/r/4178/diff/
> 
> 
> Testing
> -------
> 
> On top of running the current testsuite tests I also manually tested various configurations and scenarios using a static configuration file as well as dynamic realtime.  Verified that the crash no longer occurs and the potentially affected functionality works as expected (for instance, module [re]loading).
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

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


More information about the asterisk-dev mailing list