<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/3603/">https://reviewboard.asterisk.org/r/3603/</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/3603/diff/1/?file=59449#file59449line7113" style="color: black; font-weight: bold; text-decoration: underline;">/branches/11/main/channel.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; ">int ast_do_masquerade(struct ast_channel *original)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">7113</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>if (x != AST_GENERATOR_FD)</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">7113</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>if (x != AST_GENERATOR_FD && ast_channel_fd_isset(clonechan, x)) {</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 is hazardous. It makes it so that some file descriptors on the "original" channel which were from the past channel that inhabited the structure will survive if the new channel taking its place does not have those file descriptors set. Effectively the file descriptors on the channel become a combination of the both of them.
In the case of a generator this is correct because the channel taking its place should have the same generator as before (if you get masqueraded in you should hear ringing or music on hold).
The question becomes for this - what is the proper behavior? That of generators or that of everything else? If I masquerade into another channel should I then be using the jitterbuffer dialplan function that they invoked? If so should it be reset since there is a new source of media?
Right now this doesn't work because that behavior is undefined. The jitterbuffer code itself assumes yes, but as you've said the file descriptor is not preserved so it can't.</pre>
</div>
<br />
<p>- Joshua Colp</p>
<br />
<p>On June 10th, 2014, 12:12 a.m. UTC, Corey Farrell 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 Corey Farrell.</div>
<p style="color: grey;"><i>Updated June 10, 2014, 12:12 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-22409">ASTERISK-22409</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;">During masquerade it is possible for the AST_JITTERBUFFER_FD to be cleared (set to -1). This change adds a check when copying channel fd's to prevent clearing an FD with -1. This seems to resolve the bad audio quality experienced after the masquerade. When AST_JITTERBUFFER_FD was set to -1, this prevented the channel from polling that timer. This caused RTP packets to be received late, and discarded.
The changes to func_jitterbuffer.c were created first. ast_free(jbframe); is needed to prevent jbframe from leaking if it is rejected by jb_impl. This ensures we don't start leaking packets if they are received too late or rejected by jb_impl for any other reason.
The second change to func_jitterbuffer prevents a leak of ast_null_frame's that were duplicated (ie with ast_frdup or ast_frisolate). I believe this leak might actually be unrelated to the masquerade issue, and likely occurs for every single ast_null_frame.</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;">Verified the scenario outlined in ASTERISK-22409 no longer experiences audio quality loss, and no longer causes leaks (tested under valgrind). I patched asterisk to ensure that ast_frfree performed an immediate free to ensure valgrind would report any attempted use after free.
In early testing, I used debug messages instead of the added ast_frfree's - I verified the leaked frames reported by valgrind matched exactly to the number of debug messages.
For the masquerade fix I tested with some debug code that showed the old and new FD, this is how I found the valid FD being replaced by -1. See JIRA ticket for example output.
I have not tested this issue or fix against 12+, but the relevant code is the same as 11 - func_jitterbuffer code was moved to core but still the same code.</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/11/main/channel.c <span style="color: grey">(415593)</span></li>
<li>/branches/11/funcs/func_jitterbuffer.c <span style="color: grey">(415593)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3603/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>