[asterisk-dev] [Code Review] 2860: pjsip: race condition in registrar

[Matt Jordan]

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



branches/12/res/res_pjsip_registrar.c
<https://reviewboard.asterisk.org/r/2860/#comment18946>

    I'd rename this function to 'serializer_find_or_create'. It's nice to be explicit about the creation aspect of the function.



branches/12/res/res_pjsip_registrar.c
<https://reviewboard.asterisk.org/r/2860/#comment18944>

    This may be a reference leak.
    
    serializer_create will create an ao2 object with a reference count of 1.
    
    When you ao2_link it into the serializers container, the reference count of the object goes to 2.
    
    This means that the container does not hold the only reference to the object - so merely destroying the container will not destroy all objects in the container - it will actually orphan them.
    
    On a successful ao2_link, you should drop the creation reference provided by serializer_create.



branches/12/res/res_pjsip_registrar.c
<https://reviewboard.asterisk.org/r/2860/#comment18947>

    Richard's wiki page was rather unclear about what the unsorted comparison function should look like. He's updated it to be more explicit about the right way to do it. Much like my CDR changes, this need to return not the value of strcmp but CMP_MATCH:
    
    "This is a compare function of a non-sorted container so it needs to return CMP_MATCH not strcmp() values.
    
    return cmp ? 0 : CMP_MATCH;"
    
    See the updated wiki page as well:
    
    https://wiki.asterisk.org/wiki/pages/viewpage.action?pageId=25919686



branches/12/res/res_pjsip_registrar.c
<https://reviewboard.asterisk.org/r/2860/#comment18948>

    These aren't ACL failures.
    
    We've definitely overused this particular security event.
    
    This particular off nominal path is much closer to the existing security event "ast_security_event_req_no_support". I don't believe we have a res_pjsip method for that, but it should be trivial to add one and have it raise the correct security event.



branches/12/res/res_pjsip_registrar.c
<https://reviewboard.asterisk.org/r/2860/#comment18949>

    For these security events, which would fail essentially due to *very* off nominal internal reasons, it's probably best to use ast_security_event_mem_limit.



branches/12/res/res_pjsip_registrar.c
<https://reviewboard.asterisk.org/r/2860/#comment18945>

    This will leak the items in the container.
    
    ao2_callback with OBJ_UNLINK will release the container's references to the items, leaving them with the creation reference of 1.
    
    The ao2_cleanup will then destroy the container, but since all items have been removed, it will just dispose of the container.
    
    Note that container destruction already calls ao2_callback(container, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, NULL, NULL) in order to remove all of the items - so calling it explicitly here isn't needed.


- Matt Jordan


On Sept. 18, 2013, 7:04 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2860/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2013, 7:04 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and Mark Michelson.
> 
> 
> Bugs: AST-1213
>     https://issues.asterisk.org/jira/browse/AST-1213
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> While handling a registration request a race condition could occur if/when two+ clients registered at the same time.  This happened when one request obtained a copy of the current contacts for an AOR and another request did the same before the first request updated.  Thus the second would update and overwrite the first (or vice-versa depending on which actually updated first).  In the case of it being the same contact two "add" events would be raised.
> 
> pjsip registration handling is now serialized to alleviate this issue.
> 
> 
> Diffs
> -----
> 
>   branches/12/res/res_pjsip_registrar.c 399392 
> 
> Diff: https://reviewboard.asterisk.org/r/2860/diff/
> 
> 
> Testing
> -------
> 
> Had a few phones register themselves with asterisk using pjsip registration.  Also ran all the pjsip registration testsuite tests and made sure they all passed.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130923/c998fad1/attachment-0001.htm>