<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 />
<div>
<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#file67001line2279" 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 bridge_candidate_add_to_cdr(struct cdr_object *cdr,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2279</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="nf">bridge_candidate_process</span><span class="p">(</span><span class="k">struct</span> <span class="n">cdr_object</span> <span class="o">*</span><span class="n">cdr</span><span class="p">,</span> <span class="k">struct</span> <span class="n">cdr_object</span> <span class="o">*</span><span class="n">base_cand_cdr</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">2279</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="nf">bridge_candidate_process</span><span class="p">(</span><span class="k">struct</span> <span class="n">cdr_object</span> <span class="o">*</span><span class="n">cdr</span><span class="p">,</span> <span class="k">struct</span> <span class="n">cdr_object</span> <span class="o">*</span><span class="n">base_cand_cdr</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">* Nothing cares about this return value.
* The RAII_VAR and SCOPED_AO2LOCK can easily be eliminated by changing the "return 0" statements in the for loop to breaks.</pre>
</div>
<br />
<div>
<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#file67001line2439" 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="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2438</font></th>
<td bgcolor="#fdfebc" 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="tb"> </span><span class="k">break</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2439</font></th>
<td bgcolor="#fdfebc" 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="tb"> </span><span class="k">continue</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This change has no effect. The break only exited the switch statement and then control goes to the next iteration in the loop. Changing it to a continue just does the same thing.</pre>
</div>
<br />
<div>
<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="margin-left: 2em; 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>
</div>
<br />
<p>- rmudgett</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>