<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1738/">https://reviewboard.asterisk.org/r/1738/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 15th, 2012, 7:31 a.m., <b>Kevin Fleming</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1738/diff/3/?file=24186#file24186line466" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/astobj2.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static struct ao2_container *__ao2_copy_container(struct ao2_container *c)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">466</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_unlock</span><span class="p">(</span><span class="n">copy</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Since you are working on this code, I think it's worth considering changing internal_ao2_link to *not* lock the container; locking and unlocking it for each object being inserted in this loop is going to make the loop take an enormous amount more CPU (and real) time than is necessary.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In versions post 1.8, internal_ao2_link can (and does) use the OBJ_NOLOCK flag to prevent this from happening. I'm not sure if we want to backport this to 1.8 however.</pre>
<br />
<p>- Matt</p>
<br />
<p>On February 16th, 2012, 1:51 p.m., Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-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="color: grey;"><i>Updated Feb. 16, 2012, 1:51 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="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;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="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;">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.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19231">ASTERISK-19231</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19361">ASTERISK-19361</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(355620)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1738/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>