[asterisk-dev] [Code Review] Fixes SIP registry ref count error

[Russell Bryant]

> On 2009-06-14 16:32:43, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, lines 24221-24230
> > <http://reviewboard.digium.com/r/282/diff/1/?file=5624#file5624line24221>
> >
> >     This is an interesting comment.
> >     
> >     The container has a reference.  So, while traversing the container, it should not be possible for the object to disappear.  If that's possible, then this change does not remove the race condition, it only decreases its possibility.
> >     
> >     If the object can disappear, the bug is that the container lock is not held here.  You shouldn't need a reference on the object unless you are going to hold on to it after you unlock the container.
> 
>  wrote:
>     The container lock is held within ASTOBJ_CONTAINER_TRAVERSE.  I did not add the extra ref to the object because I am aware of any race conditions that cause the object to disappear during the traversal.  It was meant more as a safety precaution to prevent an off by one ref count error which could cause a crash.

Your comment says "A log is required for the registry, and during the lock it is possible that the registry's ref count will be decremented.  This temp ref is used to make sure the registry does not go away before it is unlocked."

My point is that to me, it is absolutely a bug if that is possible.  While you're in this traversal, it should not be possible for the ref count to hit zero, ever.  So, the extra ref/unref should just be removed.  The rest of your changes look fine.

I have a few more final comments.

1) Take a look at using AST_SCHED_DEL_UNREF()

2) As we discussed, there is a comment by the sip_registry struct definition that can probably go away.

3) Do a review over the scheduler handling of sip_registry to make sure that references aren't messed up anywhere else.

Let me know if you have any questions.


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/282/#review846
-----------------------------------------------------------


On 2009-06-11 08:56:05, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/282/
> -----------------------------------------------------------
> 
> (Updated 2009-06-11 08:56:05)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> During a sip reload, the list of sip_registry objects are supposed to be traversed, unlinked, and destroyed, but destruction never takes place due to a ref counting error.  This causes a memory leak when registry items are removed from sip.conf and reloaded.  While the registries are removed from the global list, they are not removed from the scheduler.  Because of this, SIP register attempts continue to be sent out for the item even though it may no longer be in the .conf.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 199818 
> 
> Diff: http://reviewboard.digium.com/r/282/diff
> 
> 
> Testing
> -------
> 
> added register in sip.conf, started Asterisk, took out of sip.conf, reloaded, no more register attempts were made and item was destroyed.
> 
> 
> Thanks,
> 
> David
> 
>