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

[Mark Michelson]

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



branches/12/res/res_pjsip_outbound_registration.c
<https://reviewboard.asterisk.org/r/4164/#comment24217>

    These replacements of current_states with new_states are unsafe. Container traversals lock the container in order to ensure integrity of the container during the traversal, but they do not bump the refcount of the container. This means that if you are doing an ao2_callback, ao2_find, or ao2_unlink on current_states when this ao2_replace() is called, the current_states container will be freed mid-traversal and a crash will occur.
    
    I think your best bet here is to use an AST_GLOBAL_OBJ_STATIC() to hold the registrations states. This forces you to grab a reference to the container each time you need to use it, and this allows for safe replacement of the container from any thread.



branches/12/res/res_pjsip_outbound_registration.c
<https://reviewboard.asterisk.org/r/4164/#comment24213>

    Just FYI, this will need to be reworked when porting to 13+ because authentication details are now stored in a vector.



branches/12/res/res_pjsip_outbound_registration.c
<https://reviewboard.asterisk.org/r/4164/#comment24218>

    This ao2_replace has the same issue as the current_states container. Specifically, if an outbound registration is being performed while doing the replacement, you may end up freeing the data while it's still being used, or more likely, you may end up using a weird cross of old and new configuration details for the outbound registration (see sip_outbound_registration_perform for a place this is likely to happen).
    
    I don't think an ao2_global_obj works very well as a field on a structure (though I may be wrong; that actually may be fine). You may want to just be sure that you construct things to be more atomic when dealing with the configuration on the state. Consider following a pattern like this:
    
    struct sip_outbound_registration *config = ao2_bump(state->config);
    /* do stuff with config */
    ao2_cleanup(config);
    
    This way, you get a reference to the state's configuration and can (mostly) guarantee that it won't be freed out from under you. And if state->config changes mid-operation, you continue using the same config object that you started with.



branches/12/res/res_pjsip_outbound_registration.c
<https://reviewboard.asterisk.org/r/4164/#comment24215>

    Since you bump the refcount of new_state here, the failure case needs to be sure to decrement the refcount. Otherwise, new_state is leaked.



branches/12/res/res_pjsip_outbound_registration.c
<https://reviewboard.asterisk.org/r/4164/#comment24214>

    Probably should keep current_states locked while unlocking the old state and linking the new state.



branches/12/res/res_pjsip_outbound_registration.c
<https://reviewboard.asterisk.org/r/4164/#comment24216>

    This has the possibility to return two different types. Retrieving a registration from sorcery will retrieve a sip_outbound_registration. Finding an object in the current_states container will get a sip_oubound_registration_state


- Mark Michelson


On Nov. 10, 2014, 11:35 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4164/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2014, 11:35 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 registrations and after adding in an appropriate call to ast_sorcery_apply_config() (since it is missing) Asterisk will crash after a stack overflow occurs due to the code infinitely recursing.  The fix entails removing the outbound registration state dependency from the outbound registration sorcery object and instead keeping an in memory container that can be used to lookup the state when needed.
> 
> 
> Diffs
> -----
> 
>   branches/12/res/res_pjsip_outbound_registration.c 427675 
> 
> Diff: https://reviewboard.asterisk.org/r/4164/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, cli/ami commands, module [re]loading, and manual unregistration).
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

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