> 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
