[Asterisk-code-review] Fixed locking issue in stasis app bridge moh channel (asterisk[15])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/9297 )

Change subject: Fixed locking issue in stasis_app_bridge_moh_channel
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c
File res/res_stasis.c:

https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@521
PS1, Line 521: 	struct ast_channel *moh_channel = ast_channel_get_by_name(moh_wrapper->channel_id);
> Correct. I will just return in case this is NULL. Possible but should not h
For safety you need to do the below if you don't get the channel:

if (!moh_channel) {
   ao2_unlink(app_bridges_moh, moh_wrapper);
   ao2_ref(moh_wrapper, -1);
   return NULL;
}


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@567
PS1, Line 567: 	/* The after bridge callback assumes responsibility of the bridge_id. */
             : 	if (ast_bridge_set_after_callback(chan,
             : 		moh_after_bridge_cb, moh_after_bridge_cb_failed, bridge_id)) {
             : 		ast_hangup(chan);
             : 		return NULL;
             : 	}
             : 	bridge_id = NULL;
> When are these callbacks going to be called? Sounds like when the bridge is
We are attaching the callbacks to chan which is the ;1 channel of the unreal channel pair.  We should have attached them to the ;2 channel which is pushed into the bridge by ast_unreal_channel_push_to_bridge().  The callbacks are called when the specific channel leaves the bridging system not when the bridge is destroyed.  Since the ;1 channel is never put into a bridge the callbacks never get called.  This is the main reason causing the issue because the wrapper is not removed from the app_bridges_moh container.

Your patch moves things around so we do not need to use the after bridge callbacks.  This is why the code is now dead code.


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@600
PS1, Line 600: 	if (ast_pthread_create_detached(&threadid, NULL, moh_channel_thread, new_wrapper)) {
> Ok, will fix that. I'm not deep enough into ao2 stuff.
correct


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@625
PS1, Line 625: 	ao2_unlock(app_bridges_moh);
> It's not about locking the channel. It's about locking the wrapper.
I said nothing about locking the channel.  Calling ast_channel_get_by_name() could return a dead channel if you hold the container lock.  Even if the channel is live when you get it that channel could die at any time.  Holding the wrapper container lock does not prevent that.

Are you thinking that it is better to get a dead channel to attempt starting MOH on?  Or maybe if we cannot find the channel we should loop around and try creating the channel again?



-- 
To view, visit https://gerrit.asterisk.org/9297
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibff479e167b3320c68aaabfada7e1d0ef7bd548c
Gerrit-Change-Number: 9297
Gerrit-PatchSet: 1
Gerrit-Owner: Moritz Fain <moritz at fain.io>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Moritz Fain <moritz at fain.io>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 16:07:47 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180627/8e5c56d2/attachment.html>