<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/9297">View Change</a></p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(7 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c">File res/res_stasis.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@521">Patch Set #1, Line 521:</a> <code style="font-family:monospace,monospace"> struct ast_channel *moh_channel = ast_channel_get_by_name(moh_wrapper->channel_id);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">For safety you need to check if moh_channel is NULL and unlink/unref moh_wrapper.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@526">Patch Set #1, Line 526:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ao2_lock(app_bridges_moh);<br> ao2_unlink_flags(app_bridges_moh, moh_wrapper, OBJ_NOLOCK);<br><br> ast_moh_stop(moh_channel);<br> ast_hangup(moh_channel);<br><br> ao2_unlock(app_bridges_moh);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You just need to unlink moh_wrapper from the container and unref moh_wrapper. You do not need to hold the lock to stop MOH and hangup the channel.</p><p style="white-space: pre-wrap; word-wrap: break-word;">ao2_unlink(app_bridges, moh_wrapper);<br>ao2_ref(moh_wrapper, -1);</p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_moh_stop(moh_channel);<br>ast_hangup(moh_channel);</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@567">Patch Set #1, Line 567:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* The after bridge callback assumes responsibility of the bridge_id. */<br> if (ast_bridge_set_after_callback(chan,<br> moh_after_bridge_cb, moh_after_bridge_cb_failed, bridge_id)) {<br> ast_hangup(chan);<br> return NULL;<br> }<br> bridge_id = NULL;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is dead code and needs to be removed along with the passed in callbacks and bridge_id. This is the main reason causing the issue. We were setting the after bridge callbacks on the wrong channel so the callbacks never get called. The way you are changing it makes this method unneeded.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@588">Patch Set #1, Line 588:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (ast_string_field_init(new_wrapper, 32)) {<br> ast_hangup(chan);<br> return NULL;<br> }<br> ast_string_field_set(new_wrapper, bridge_id, bridge->uniqueid);<br> ast_string_field_set(new_wrapper, channel_id, ast_channel_uniqueid(chan));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since we are passing new_wrapper to moh_channel_thread() these string field sets are critical.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (ast_string_field_init(..)<br> || ast_string_field_set(...bridge_id...)<br> || ast_string_field_set(...channel_id...)) {<br> ast_hangup(chan);<br> return NULL;<br>}</pre></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@600">Patch Set #1, Line 600:</a> <code style="font-family:monospace,monospace"> if (ast_pthread_create_detached(&threadid, NULL, moh_channel_thread, new_wrapper)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We need to pass a new_wrapper ref to moh_channel_thread(). We cannot rely on the ref held in app_bridges_moh to remain valid for moh_channel_thread() to implicitly use.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">ao2_ref(new_wrapper, +1);<br>if (ast_pthread_create_detached(...)) {<br> ao2_ref(new_wrapper, -1);<br> ...<br>}</pre></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@610">Patch Set #1, Line 610:</a> <code style="font-family:monospace,monospace">struct ast_channel *stasis_app_bridge_moh_channel(struct ast_bridge *bridge)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Before this patch bridge_moh_create() returned a pointer to chan without a ref while ast_channel_get_by_name() returned a ref with the pointer to the channel. This was a bug which your patch implicitly fixes because any callers to this function wouldn't know if they had a ref to the returned channel or not.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However, the only caller to this function in res/ari/resource_bridges.c:ast_ari_bridges_start_moh() needs to unref the returned channel after starting MOH:</p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_moh_start(...);<br>ast_channel_cleanup(moh_channel);</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@625">Patch Set #1, Line 625:</a> <code style="font-family:monospace,monospace"> ao2_unlock(app_bridges_moh);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Moving this lock doesn't help. As soon as you release the lock someone could call stasis_app_bridge_moh_stop() and kill the channel before MOH is actually started.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9297">change 9297</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9297"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ibff479e167b3320c68aaabfada7e1d0ef7bd548c </div>
<div style="display:none"> Gerrit-Change-Number: 9297 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Moritz Fain <moritz@fain.io> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 27 Jun 2018 00:06:35 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>