[asterisk-dev] [Code Review] Incorrect reloading of realtime peer causes mailbox list to expand indefinitely

[Nick Lewis]

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



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/666/#comment4319>

    There may also need to be a corresponding test condition for "type", "user" and FINDPEERS to prevent realtime loading when a peer is wanted but a user is found. This may also remove the need for a similar test later in find_peer()
    
    Trivially there may be unnecessary extra parenthesis around the second &&
    
    Suggested condition:
    if (
     !cmp(name,"type") && 
     (
      (!cmp(value,"peer") && wo==FINDUSERS) 
      || 
      (!cmp(value,"user") && wo==FINDPEERS)
     ) 
    )



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/666/#comment4322>

    I do not think this switch case is used because realtime_peer() returns null p under these conditions



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/666/#comment4323>

    It may be too late to do this here as realtime_peer() has already loaded p into memory. Better to do this test early in realtime_peer() instead



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/666/#comment4324>

    Suggest removing all this block and letting realtime_peer() do it


- Nick


On 2010-05-19 11:16:27, pabelanger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/666/
> -----------------------------------------------------------
> 
> (Updated 2010-05-19 11:16:27)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Posting this patch for review, it is marked as Major on the tracker and numerous people seem to be affected.
> 
> Copied from issue:
> ---
> Due to the current code's inability to short-circuit the reloading of a realtime peer when it's looking for a user, the mailbox list is repeatedly appended to.
> 
> For example, if you issue a 'sip show peer 1234', you may get something like:
> Mailbox : 1234 at default
> 
> But after any inbound peer match (resulting in a call to find_peer), it will result in:
> Mailbox : 1234 at default, 1234 at default
> 
> And so on, ad infinitum.
> 
> This can cause a large number of NOTIFYs to be sent to the peer, in some cases resulting in a phone crash or reboot.
> 
> There may be other effects, but this is the observed behavior. 
> ---
> 
> 
> This addresses bugs 16320, 17254 and 17313.
>     https://issues.asterisk.org/view.php?id=16320
>     https://issues.asterisk.org/view.php?id=17254
>     https://issues.asterisk.org/view.php?id=17313
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 264161 
> 
> Diff: https://reviewboard.asterisk.org/r/666/diff
> 
> 
> Testing
> -------
> 
> Marquis created the patch, and other reporters successfully running it.
> 
> 
> Thanks,
> 
> pabelanger
> 
>