No subject

Fri Sep 2 03:59:05 CDT 2011

e iterator is the same.  The AO2_ITERATOR_SNAPSHOT lets us take a few advan=
tages with the actual iterating - since the iterator owns the copied contai=
ner and the objects inside are ref'd, we no longer need to lock the contain=
er when we traverse it.  The AO2_ITERATOR_DONTLOCK flag is added by default=
 when this flag is used.

Tested with 1.8.  Used valgrind to check for any introduced memory leaks, u=
sed the ref counting debug option to check for ref counting errors.  Checke=
d with multiple calls to "sip show peers", as well as reloads of chan_sip.

Summary
-------

The SIP show peers command stores pointers to the current peers in the peer=
s 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 loc=
ked 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 numb=
er of exit points from the function that occur before the array is dealloca=
ted, resulting in a memory leak.
2) The array size is supposed to be the current number of peers in the cont=
ainer.  However, the peers container is not locked when the size is determi=
ned, 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 tr=
aversal of the container will be "counted" during the traversal, and the ar=
ray can be overrun.

This would typically occur if some other thread performed a sip reload at t=
he 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 invo=
lved 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.  Th=
e 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 v=
ersion number of that item will be greater then the version of iterator, an=
d will be ignored
2) If an item is removed from the container prior to/while traversing it, t=
he number of items added to the array will be less then the array size, whi=
ch 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.  T=
he 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.

Diffs (updated)
-----

  /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 reload=
s" alongside a script that ran "sip show peers" once a second.

Thanks,

Matt

--===============8489260289899266696==
Content-Type: text/html; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

<html>
 <body>
  <div style=3D"font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor=3D"#f9f3c9" width=3D"100%" cellpadding=3D"8" style=3D"bor=
der: 1px #c9c399 solid;">
    <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href=3D"https://reviewboard.asterisk.org/r/1738/">https://reviewbo=
ard.asterisk.org/r/1738/</a>
     </td>
    </tr>
   </table>
   <br />

<table bgcolor=3D"#fefadf" width=3D"100%" cellspacing=3D"0" cellpadding=3D"=
8" style=3D"background-image: url('https://reviewboard.asterisk.org/media/r=
b/images/review_request_box_top_bg.png'); background-position: left top; ba=
ckground-repeat: repeat-x; border: 1px black solid;">
 <tr>
  <td>

<div>Review request for Asterisk Developers, Mark Michelson and rmudgett.</=
div>
<div>By Matt Jordan.</div>

<p style=3D"color: grey;"><i>Updated Feb. 14, 2012, 12:50 p.m.</i></p>

<h1 style=3D"color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</=
h1>
<table width=3D"100%" bgcolor=3D"#ffffff" cellspacing=3D"0" cellpadding=3D"=
10" style=3D"border: 1px solid #b8b5a0">
 <tr>
  <td>
   <pre style=3D"margin: 0; padding: 0; white-space: pre-wrap; white-space:=
 -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap=
: break-word;">Take 2 of this, basing the approach on Kevin&#39;s suggestio=
n of using a snapshot of the container.  This patch adds a new iterator fla=
g, AO2_ITERATOR_SNAPSHOT, that when passed to ao2_iterator_init, will creat=
e a copy of the container and associate it with the passed back iterator.