Summary:	ASTERISK-15212: [patch] Incorrect reloading of realtime peer causes mailbox list to expand indefinitely
Reporter:	Bradley Watkins (marquis)	Labels:
Date Opened:	2009-11-24 18:22:16.000-0600	Date Closed:	2010-06-07 17:59:53
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_sip/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20100525__issue16320.diff.txt ( 1) find_peer_fix.patch
Description:	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@default But after any inbound peer match (resulting in a call to find_peer), it will result in: Mailbox     : 1234@default, 1234@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. ****** ADDITIONAL INFORMATION ****** The proposed patch pushes the condition FINDUSER/FINDPEER/etc. down to realtime_peer, where a decision can be made to stop when a user is requested but a peer is found. Also, this includes (and is somewhat related to) the patch in issue 16021 as it touches much the same code. Patch is against latest /branches/1.6.1 though issue and fix were tested on 1.6.1.10.
Comments:	By: Leif Madsen (lmadsen) 2010-05-18 12:18:16 OK, so I have ASTERISK-16022 marked as Resolved by this issue, which I guess means this issue needs to be reviewed and committed right? Also what do we need to do with ASTERISK-14941? Is that all part of this same group of issues, or is that a separate fix for something else? By: Leif Madsen (lmadsen) 2010-05-18 12:20:07 From what I'm reading, I believe this issue resolves both 17254 and 16021? Is that right? By: Leif Madsen (lmadsen) 2010-05-18 12:46:53 Marking this issue as Ready for Review. It'd be nice if we could get this up on reviewboard and someone to sign off on it before we commit, especially since this is a change to chan_sip.c. By: Bradley Watkins (marquis) 2010-05-18 14:01:40 16021 touches the same code, although technically it's not the *same* issue. This is definitely ready for review and commit. Thanks! By: Leif Madsen (lmadsen) 2010-05-19 10:22:18 Assigned to pabelanger just so he can post this to reviewboard and update the issue, unless Marquis can get to it first ;) By: Kevin Scott Adams (nivek) 2010-05-21 13:09:36 Marquis...Answer so we can move on!!!! By: Paul Belanger (pabelanger) 2010-05-24 14:37:55 Please follow along in the reviewboard discussion: https://reviewboard.asterisk.org/r/666/ Russell has posted some comments about this patch.  I have not had time to reply to them, perhaps the reporter can make the required updates. By: Tilghman Lesher (tilghman) 2010-05-25 13:57:14 I think I'd rather go with something a little simpler.  Instead of trying to avoid adding the entry to the list of mailboxes, just add duplicate detection code, as I've done here.  This additionally removes inadvertent duplicates. By: Bradley Watkins (marquis) 2010-06-03 17:15:41 I haven't tested the patch with your new approach, but it looks reasonable to me.  The only reservation I have is that the simpler approach treats the *symptom* while my original patch fixes the root cause. The only reason this might be materially different is if there are other, unknown issues with the way realtime peers are being loaded.  I have absolutely ZERO proof (or even idle speculation, based on a cursory glance over the code) that this is the case, and so I'm happy with the new way.  chan_sip is a wooly place... By: Digium Subversion (svnbot) 2010-06-07 17:47:13 Repository: asterisk Revision: 268817 U   trunk/channels/chan_sip.c U   trunk/channels/sip/include/sip.h ------------------------------------------------------------------------ r268817 | tilghman | 2010-06-07 17:47:13 -0500 (Mon, 07 Jun 2010) | 9 lines Mailbox list would previously grow at each reload, containing duplicates. Also, optimize the allocation of mailboxes to avoid additional memory structures. (closes issue ASTERISK-15212) Reported by: Marquis Patches:       20100525__issue16320.diff.txt uploaded by tilghman (license 14) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=268817 By: Digium Subversion (svnbot) 2010-06-07 17:59:52 Repository: asterisk Revision: 268819 _U  branches/1.6.2/ U   branches/1.6.2/channels/chan_sip.c ------------------------------------------------------------------------ r268819 | tilghman | 2010-06-07 17:59:52 -0500 (Mon, 07 Jun 2010) | 16 lines Merged revisions 268817 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........  r268817 | tilghman | 2010-06-07 17:47:13 -0500 (Mon, 07 Jun 2010) | 9 lines    Mailbox list would previously grow at each reload, containing duplicates.    Also, optimize the allocation of mailboxes to avoid additional memory structures.    (closes issue ASTERISK-15212)   Reported by: Marquis   Patches:         20100525__issue16320.diff.txt uploaded by tilghman (license 14) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=268819