[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: Code-Review-1

(7 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);
For safety you need to check if moh_channel is NULL and unlink/unref moh_wrapper.


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@526
PS1, Line 526: 	ao2_lock(app_bridges_moh);
             : 	ao2_unlink_flags(app_bridges_moh, moh_wrapper, OBJ_NOLOCK);
             : 
             : 	ast_moh_stop(moh_channel);
             : 	ast_hangup(moh_channel);
             : 
             : 	ao2_unlock(app_bridges_moh);
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.

ao2_unlink(app_bridges, moh_wrapper);
ao2_ref(moh_wrapper, -1);

ast_moh_stop(moh_channel);
ast_hangup(moh_channel);


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;
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.


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@588
PS1, Line 588: 	if (ast_string_field_init(new_wrapper, 32)) {
             : 		ast_hangup(chan);
             : 		return NULL;
             : 	}
             : 	ast_string_field_set(new_wrapper, bridge_id, bridge->uniqueid);
             : 	ast_string_field_set(new_wrapper, channel_id, ast_channel_uniqueid(chan));
Since we are passing new_wrapper to moh_channel_thread() these string field sets are critical.

if (ast_string_field_init(..)
   || ast_string_field_set(...bridge_id...)
   || ast_string_field_set(...channel_id...)) {
   ast_hangup(chan);
   return NULL;
}


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)) {
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.

ao2_ref(new_wrapper, +1);
if (ast_pthread_create_detached(...)) {
    ao2_ref(new_wrapper, -1);
    ...
}


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@610
PS1, Line 610: struct ast_channel *stasis_app_bridge_moh_channel(struct ast_bridge *bridge)
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.

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:

ast_moh_start(...);
ast_channel_cleanup(moh_channel);


https://gerrit.asterisk.org/#/c/9297/1/res/res_stasis.c@625
PS1, Line 625: 	ao2_unlock(app_bridges_moh);
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.



-- 
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: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 00:06:35 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180626/29f18367/attachment.html>