From f9465fd40dceba5f0d2d99bda959b6b9e22b4523 Mon Sep 17 00:00:00 2001 From: Matthew Jordan Date: Thu, 9 Jan 2014 15:41:31 +0000 Subject: [PATCH] app_confbridge: Fix crash caused when waitmarked/marked users leave together When waitmarked users join a ConfBridge, the conference state is transitioned from EMPTY -> INACTIVE. In this state, the users are maintined in a waiting users list. When a marked user joins, the ConfBridge conference transitions from INACTIVE -> MULTI_MARKED, and all users are put onto the active list of users. This process works correctly. When the marked user leaves, if they are the last marked user, the MULTI_MARKED state does the following: (1) It plays back a message to the bridge stating that the leader has left the conference. This requires an unlocking of the bridge. (2) It moves waitmarked users back to the waiting list (3) It transitions to the appropriate state: in this case, INACTIVE However, because it plays the prompt back to the bridge before moving the users and before finishing the state transition, this creates a race condition: with the bridge unlocked, waitmarked users who leave the conference (or are kicked from it) can cause a state transition of the bridge to another state before the conference is transitioned to the INACTIVE state. This causes the state machine to get a bit wonky, often leading to a crash when the MULTI_MARKED state attempts to conclude its processing. This patch fixes this problem: (1) It prevents kicked users from being kicked again. That's just a nicety. (2) More importantly, it fixes the race condition by only playing the prompt once the state has transitioned correctly to INACTIVE. If waitmarked users sneak out during the prompt being played, no harm no foul. Review: https://reviewboard.asterisk.org/r/3108/ Note that the patch committed here is essentially the same as uploaded by Simon Moxon on ASTERISK-22740, with the addition of the double kick prevention. (closes issue AST-1258) Reported by: Steve Pitts (closes issue ASTERISK-22740) Reported by: Simon Moxon patches: ASTERISK-22740.diff uploaded by Simon Moxon (license 6546) git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/11@405215 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- apps/app_confbridge.c | 6 +++--- apps/confbridge/conf_state_multi_marked.c | 25 ++++++++++++++--------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/apps/app_confbridge.c b/apps/app_confbridge.c index 9fc3af3ad3..86051cb4a3 100644 --- a/apps/app_confbridge.c +++ b/apps/app_confbridge.c @@ -1961,7 +1961,7 @@ static int action_kick_last(struct conference_bridge *conference_bridge, ast_stream_and_wait(bridge_channel->chan, conf_get_sound(CONF_SOUND_ERROR_MENU, conference_bridge_user->b_profile.sounds), ""); - } else if (last_participant) { + } else if (last_participant && !last_participant->kicked) { last_participant->kicked = 1; ast_bridge_remove(conference_bridge->bridge, last_participant->chan); ao2_unlock(conference_bridge); @@ -2139,7 +2139,7 @@ static int kick_conference_participant(struct conference_bridge *bridge, const c ao2_lock(bridge); AST_LIST_TRAVERSE(&bridge->active_list, participant, list) { - if (!strcasecmp(ast_channel_name(participant->chan), channel)) { + if (!strcasecmp(ast_channel_name(participant->chan), channel) && !participant->kicked) { participant->kicked = 1; ast_bridge_remove(bridge->bridge, participant->chan); ao2_unlock(bridge); @@ -2147,7 +2147,7 @@ static int kick_conference_participant(struct conference_bridge *bridge, const c } } AST_LIST_TRAVERSE(&bridge->waiting_list, participant, list) { - if (!strcasecmp(ast_channel_name(participant->chan), channel)) { + if (!strcasecmp(ast_channel_name(participant->chan), channel) && !participant->kicked) { participant->kicked = 1; ast_bridge_remove(bridge->bridge, participant->chan); ao2_unlock(bridge); diff --git a/apps/confbridge/conf_state_multi_marked.c b/apps/confbridge/conf_state_multi_marked.c index 7e71086209..6089ac6f02 100644 --- a/apps/confbridge/conf_state_multi_marked.c +++ b/apps/confbridge/conf_state_multi_marked.c @@ -80,23 +80,16 @@ static void leave_active(struct conference_bridge_user *cbu) static void leave_marked(struct conference_bridge_user *cbu) { struct conference_bridge_user *cbu_iter; + int need_prompt = 0; conf_remove_user_marked(cbu->conference_bridge, cbu); if (cbu->conference_bridge->markedusers == 0) { - /* Play back the audio prompt saying the leader has left the conference */ - if (!ast_test_flag(&cbu->u_profile, USER_OPT_QUIET)) { - ao2_unlock(cbu->conference_bridge); - ast_autoservice_start(cbu->chan); - play_sound_file(cbu->conference_bridge, - conf_get_sound(CONF_SOUND_LEADER_HAS_LEFT, cbu->b_profile.sounds)); - ast_autoservice_stop(cbu->chan); - ao2_lock(cbu->conference_bridge); - } + need_prompt = 1; AST_LIST_TRAVERSE_SAFE_BEGIN(&cbu->conference_bridge->active_list, cbu_iter, list) { /* Kick ENDMARKED cbu_iters */ - if (ast_test_flag(&cbu_iter->u_profile, USER_OPT_ENDMARKED)) { + if (ast_test_flag(&cbu_iter->u_profile, USER_OPT_ENDMARKED) && !cbu_iter->kicked) { if (ast_test_flag(&cbu_iter->u_profile, USER_OPT_WAITMARKED) && !ast_test_flag(&cbu_iter->u_profile, USER_OPT_MARKEDUSER)) { AST_LIST_REMOVE_CURRENT(list); @@ -162,6 +155,18 @@ static void leave_marked(struct conference_bridge_user *cbu) break; /* Stay in marked */ } } + + if (need_prompt) { + /* Play back the audio prompt saying the leader has left the conference */ + if (!ast_test_flag(&cbu->u_profile, USER_OPT_QUIET)) { + ao2_unlock(cbu->conference_bridge); + ast_autoservice_start(cbu->chan); + play_sound_file(cbu->conference_bridge, + conf_get_sound(CONF_SOUND_LEADER_HAS_LEFT, cbu->b_profile.sounds)); + ast_autoservice_stop(cbu->chan); + ao2_lock(cbu->conference_bridge); + } + } } static void transition_to_marked(struct conference_bridge_user *cbu)