<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/3964/">https://reviewboard.asterisk.org/r/3964/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 3rd, 2014, 2:08 p.m. CDT, <b>rmudgett</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/3964/diff/1/?file=67001#file67001line2466" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/cdr.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static void handle_standard_bridge_enter_message(struct cdr_object *cdr,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2465</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="cm">/* This is guaranteed to succeed: the new CDR is created in the single state</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2466</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="cm">/* This is guaranteed to succeed: the new CDR is created in the single state</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2466</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span> * and will be able to handle the bridge enter message</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2467</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span> * and will be able to handle the bridge enter message</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2467</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span> */</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2468</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span> */</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2468</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">handle_standard_bridge_enter_message</span><span class="p">(</span><span class="n">cdr</span><span class="p">,</span> <span class="n">bridge</span><span class="p">,</span> <span class="n">channel</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2469</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">handle_standard_bridge_enter_message</span><span class="p">(</span><span class="n">cdr</span><span class="p">,</span> <span class="n">bridge</span><span class="p">,</span> <span class="n">channel</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;">This is a tail recursion that could be eliminated by pulling the above for loop into its own routine. Then the cdr lock also won't be recursively obtained.</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;">I don't want to expand the scope of this review.
If we want to make additional improvements to the CDR code, I'm all for that, but it should be done on a separate issue as part of a different patch. Making changes in CDRs that does not apply directly to this bug on this review feels like a bad idea.</pre>
<br />
<p>- Matt</p>
<br />
<p>On September 1st, 2014, 11:51 a.m. CDT, Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Sept. 1, 2014, 11:51 a.m.</i></p>
<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-24241">ASTERISK-24241</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">This patch fixes an issue where CDRs would get stuck generating an infinite number of CDRs, eventually crashing Asterisk.
When a channel enters into a multi-party bridge, the CDR engine creates mappings of each participant to each other participant, picking the 'A' party as it goes. So, if we have four channels in a multi-party bridge (Alice, Bob, Charlie, Denise), we would have something like:
Alice => Bob
Alice => Charlie
Alice => Denise
Bob => Charlie
Bob => Denise
Charlie => Denise
This works fine when participants enter the bridge a single time.
When a participant leaves a bridge, the CDRs for that channel are transitioned to a finalized state. In the previous example, if Bob leaves the bridge, then the following CDRs are all marked as finalized:
Alice => Bob (F)
Bob => Charlie (F)
Bob => Denise (F)
The bug occurs if Bob rejoins. When the CDR engine creates mappings between the channels, a CDR in the finalized state reports back to the engine "nope, I can't handle this, you need a new CDR". That is correct, but the code that generates the mappings makes two mistakes at that point:
(1) It bails instead of looking for more CDRs, immediately making a new CDR and appending it to the chain. When you make a new CDR for a channel, it does not originally have a Party B - so the engine calls the bridge mapping code "knowing" that Bob will be a perfect Party B.
(2) When a new CDR is appended to a chain, one code path allowed us to continue on down the chain inspecting more CDRs. We do update the 'new' CDR, but we end up transitioning it to the Bridge state and calling the original code on it again, hitting point #1 again (as there is still an original CDR in the finalized state).
Let's say Bob enters the bridge again. The CDR engine starts making pairs of CDRs, starting with Alice. Since Alice and Bob's original CDR is finalized (he left the bridge), the record returns "you need a new one". The CDR engine makes a new CDR for Alice, appends it to her CDRs, and then tells Bob to go see if he can find a CDR for Alice that is suitable. This hits bug #2, which maps Bob to Alice but then keeps looping instead of returning automatically. When we're done updating the new CDR, the original code takes over, hits point #1 again, and generates a new CDR. Hilarity!
We end up with:
Alice => Bob (F) <--- this keeps getting inspected and telling us to generate new CDRs :-(
Alice => Bob (new)
Alice => Bob (new)
Alice => Bob (new)
...
This patch fixes the two glitches mentioned above.
(1) If, when making matches, a CDR returns 'you need a new one', we keep inspecting the chain to see if anyone else can take a match for us
(2) When a match is made, we return if we successfully placed our channel as the Party B on an existing CDR, instead of continuing to match up
And, even after this bug, I still maintain this is easier to diagnose and fix then the original CDR code in features.c :-) If nothing else, all of this code was in one place, and I didn't have to think "what happens if something masquerades right now".</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;">All existing unit tests/testsuite tests pass.
Issue was reproduced using the test on https://reviewboard.asterisk.org/r/3965. The test creates 5 Local channels, puts them in a multi-party bridges, then removes channels 0 and 3 and re-adds them.
Without this patch - explosions. With this patch - we generate the correct number of CDRs even when channels bounce themselves in and out of multi-party bridges.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/12/main/cdr.c <span style="color: grey">(422502)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3964/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>