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

[Matt Jordan]

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



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24265>

    Given that this small function is only called by ast_sip_publish_client_get, I think the entire thing could be moved to that function.



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24266>

    Rather than forward declaring this, you could simply move the function to this spot. Since it is logically the 'destruction callback' for state objects, it probably belongs with the rest of the state-specific functions anyway.



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24268>

    This returns an int, but currently always returns 0. That's fine, since this is a callback function for an ao2_callback, and there isn't much we can do.
    
    However, if states or state is not valid, we should at least log an ERROR, as things will be in an extremely "bad" state at that point.



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24267>

    This is a common anti-pattern to slip into that is dangerous.
    
    There's no guarantee that states is non-NULL after the call to ao2_global_obj_ref(current_states) - you could be in a module unload, the container has been destroyed, and somehow this function gets called.
    
    Any time you have a global container, you have to check that it is valid before using it. This should be:
    
    struct ast_sip_outbound_publish_client *state;
    
    if (!states) {
        return -1;
    }
    
    state = ao2_find(...)



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24269>

    I'm with Richard on this: I think it's almost always better to break up the allocation and the validity check - particularly for long and complex allocations.
    
    new_state = ao2_container_alloc(...);
    if (!new_state) {
    
    }



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24270>

    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.



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24271>

    This is unsafe, as previously noted. states can be NULL.



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24272>

    Break up allocation.



branches/13/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/4178/#comment24273>

    This is another reason to unify the destructors of the state objects: this will cause the current_states container to be destroyed, without fist calling destroy_old_state on the objects:
    
    ao2_callback(current_states, OBJ_NODATA | OBJ_UNLINK, destroy_old_state, NULL);


- Matt Jordan


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/20141115/392580bb/attachment-0001.html>