[asterisk-dev] [Code Review]: Fix memory leak, possible crash in sip show peers

Tue Feb 14 20:47:10 CST 2012

> On Feb. 14, 2012, 6:39 p.m., rmudgett wrote:
> > On the whole, I dislike AO2_ITERATOR_SNAPSHOT.  I especially don't like it when there already exists a more flexible way to do it that doesn't have so many implementation holes in it.
> 
> rmudgett wrote:
>     I should have stated that it has an API hole as well as implementation errors.  You cannot know if the iterator snapshot failed without looking into the iterator struct which is documented as "opaque".

See my answer to your finding below on the opaqueness problem.

At this point I'm fine throwing this away.  I think there are benefits to having something purely provided by the ao2 API - if for no other reason then having using the ao2 callback function with no callback function (see, that's not terribly clear) functionally providing the same thing is not intuitive.  It might be slightly faster too, if for no other reason then there's less function calls and it doesn't have to unlink the object during the traversal.  That's a very minor benefit that I can't really back up with evidence however.

I'll take that up in asterisk-dev tomorrow.

> On Feb. 14, 2012, 6:39 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_sip.c, line 16685
> > <https://reviewboard.asterisk.org/r/1738/diff/3/?file=24184#file24184line16685>
> >
> >     You have no way of knowing if the ao2_iterator_init snapshot was successful.  If it fails, you will crash.

Yup, that is a problem.  However, its a pretty limited problem - the initialization of the ao2 iterator will only fail if an allocation of the container fails, or if one of the bucket entries can't be allocated.  Given that Asterisk is usually doomed to a very fast death if memory allocations start failing, I don't see this as that big of a limitation.  That being said, yup, you'd have to look into the container to determine if it failed or not, which isn't ideal.

> On Feb. 14, 2012, 6:39 p.m., rmudgett wrote:
> > /branches/1.8/include/asterisk/astobj2.h, lines 1074-1079
> > <https://reviewboard.asterisk.org/r/1738/diff/3/?file=24185#file24185line1074>
> >
> >     You need to make the note stronger about thread safety.  You must lock the container before using this flag if other threads can access the container.

How can this be made "stronger"?  I don't mind wording it differently, but "the container should be locked" is pretty explicit.

> On Feb. 14, 2012, 6:39 p.m., rmudgett wrote:
> > /branches/1.8/main/astobj2.c, line 462
> > <https://reviewboard.asterisk.org/r/1738/diff/3/?file=24186#file24186line462>
> >
> >     This is a misuse of the AO2_ITERATOR_DONTLOCK flag.  You are now guaranteed to iterate over the original container without a lock unless the container is already locked as a condition of calling __ao2_copy_container().

That was actually the intent.  This is a private function, and I don't want it locking the original container - its up to the caller of the function to know whether or not they should lock the container.  This is for a few reasons:
1) There are times where locking the original container is overkill (when only one thread can be accessing the container at a time)
2) If someone (such as chan_sip, in this example) needs to lock the container because they need to do multiple operations on the container, including creating this iterator, then locking the container again here is (a) unnecessary and (b) requires recursive mutexes.  Yes, we have recursive mutex locks, but I try to avoid having to rely on that behavior as much as possible.

Over-locking is costly, and unnecessary.

> On Feb. 14, 2012, 6:39 p.m., rmudgett wrote:
> > /branches/1.8/main/astobj2.c, lines 464-467
> > <https://reviewboard.asterisk.org/r/1738/diff/3/?file=24186#file24186line464>
> >
> >     Doing this does not really buy you anything.  It just made the code error prone.  You have a ref leak here if the link fails.

Yes, it should unref this if this fails.  But I wouldn't say that it doesn't "buy anything" - if the link succeeds, the copy has to be unlocked.  If the link doesn't succeed, it doesn't need to be unlocked.

> On Feb. 14, 2012, 6:39 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_sip.c, line 16684
> > <https://reviewboard.asterisk.org/r/1738/diff/3/?file=24184#file24184line16684>
> >
> >     peerarray is never checked for failure.

Yup.  I'll fix that.

- Matt

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

On Feb. 14, 2012, 5:12 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1738/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2012, 5:12 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and rmudgett.
> 
> 
> Summary
> -------
> 
> The SIP show peers command stores pointers to the current peers in the peers container in a fixed size array.  The size of this array is determined a bit before iterating through the peers container.  The container is not locked during the array initialization, nor while the iterator is initialized.  There are at least two problems with this:
> 1) The array is allocated at the beginning of the method.  There are a number of exit points from the function that occur before the array is deallocated, resulting in a memory leak.
> 2) The array size is supposed to be the current number of peers in the container.  However, the peers container is not locked when the size is determined, and the version of the iterator is not set until ao2_iterator_next is called.  Thus, any items added to the peers container prior to the first traversal of the container will be "counted" during the traversal, and the array can be overrun.
> 
> This would typically occur if some other thread performed a sip reload at the same time as the sip show peers command, and additional peers were added in between when the array was sized and when the peers were traversed.
> 
> This patch takes a bit more of a pessimistic view of the thread-safety involved with showing the peers.  We lock the container during the period that we build the array for sorting and when we initialize the ao2_iterator.  The ao2_iterator initialization has been modified as well to set the version to the container version, as opposed to waiting for the first traversal of the container.  This should be safe:
> 1) If an item is added to the container prior to/while traversing it, the version number of that item will be greater then the version of iterator, and will be ignored
> 2) If an item is removed from the container prior to/while traversing it, the number of items added to the array will be less then the array size, which is acceptable
> 
> Note that you can still have a situation where a reload occurs during a sip show peers which changes what the command will output while it is building the peer array; however, that's no different then the current behavior.  The only way to prevent that would be to lock the peers container during the entire traversal, which would potentially impact performance on normal SIP operations.
> 
> 
> This addresses bugs ASTERISK-19231 and ASTERISK-19361.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19231
>     https://issues.asterisk.org/jira/browse/ASTERISK-19361
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 355276 
>   /branches/1.8/include/asterisk/astobj2.h 355276 
>   /branches/1.8/main/astobj2.c 355276 
> 
> Diff: https://reviewboard.asterisk.org/r/1738/diff
> 
> 
> Testing
> -------
> 
> Basic testing involving running sip show peers and reloading.  More testing will need to be done by the issue reporter, who was performing "sip reloads" alongside a script that ran "sip show peers" once a second.
> 
> 
> Thanks,
> 
> Matt
> 
>

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