From 772985be5e82b7ee33e1a823fa7e1de01a31f292 Mon Sep 17 00:00:00 2001
From: Wenpeng Song <wenpeng.song@iopsys.eu>
Date: Wed, 31 Jan 2024 13:06:16 +0000
Subject: [PATCH] Fix a dead lock and remove some duplicated indications during
un-hold
Fix a dead lock during un-hold procedure under race condition by
* Remove duplicated indications for un-hold.
* Narrow the usage of channel lock inside `chan_voicemngr_modify_codec
* Remove the extra redirection for the case from: https://dev.iopsys.eu/voice/asterisk-chan-
voicemngr/-/merge_requests/26 as it is not needed for now.
The dead lock is due to `ast_channel_get_by_name` used by `chan_voicemngr_modify_codec`(which
called during ssrc related indications) https://dev.iopsys.eu/voice/asterisk-chan-
voicemngr/-/blob/devel/src/channels/chan_voicemngr.c#L774 has a channel lock from
`ast_channel_by_name_cb`. And indications for different channels also have locks from
`ast_indicate_data`. They may waiting for each other during some massive indications between
bridged channels.
(cherry picked from commit 1e3a03a82ed730eccdee999b7ac019043e07d148)
---
src/channels/chan_voicemngr.c | 44 +++++++++++++++++++++++------------
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/src/channels/chan_voicemngr.c b/src/channels/chan_voicemngr.c
index fe1bf68..b75815d 100644
--- a/src/channels/chan_voicemngr.c
+++ b/src/channels/chan_voicemngr.c
@@ -766,6 +766,7 @@ static void chan_voicemngr_modify_codec(struct chan_voicemngr_subchannel *sub) {
ast_debug(4, "sub->owner Channel %s, ast_channel_codec_get(): %s\n",ast_channel_name(sub->owner), ast_channel_codec_get(sub->owner));
ast_channel_lock(sub->owner);
struct ast_bridge *bridge = ast_channel_internal_bridge(sub->owner);
+ ast_channel_unlock(sub->owner);
struct ast_bridge_channel *bridge_channel;
struct ast_channel *bridged_chan=NULL;
if(bridge){
@@ -780,32 +781,41 @@ static void chan_voicemngr_modify_codec(struct chan_voicemngr_subchannel *sub) {
if (bridged_chan) {
ast_debug(5, "Got bridged_chan\n");
// bridged_chan is allocated but codec and ptime is not set so need to check it here and set default
+ ast_channel_lock(sub->owner);
ast_channel_ptime_set(sub->owner, ast_channel_ptime_get(bridged_chan) ? ast_channel_ptime_get(bridged_chan) : default_ptime);
+ ast_channel_unlock(sub->owner);
if (strncmp(ast_channel_name(bridged_chan), "TELCHAN", 5) == 0 ) {
// local chan, internal call, using alaw.
ast_log(LOG_NOTICE, "INTERNAL CALL, %s\n", ast_channel_name(bridged_chan));
+ ast_channel_lock(sub->owner);
ast_channel_codec_set(sub->owner, "alaw");
+ ast_channel_unlock(sub->owner);
+ ast_channel_lock(bridged_chan);
ast_channel_codec_set(bridged_chan, "alaw");
+ ast_channel_unlock(bridged_chan);
// set sip client to 0 for internal call
sub->sip_client_id = 0;
struct ast_format_cap *caps;
caps = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
if (caps) {
- ast_format_cap_append(caps, map_rtpname_to_format("alaw"), 0);
+ ast_format_cap_append(caps, map_rtpname_to_format("alaw"), 0);
+ ast_channel_lock(bridged_chan);
ast_channel_nativeformats_set(bridged_chan, caps);
+ ast_channel_unlock(bridged_chan);
ao2_ref(caps, -1);
} else {
ao2_cleanup(caps);
}
} else {
// get codec from bridged_chan's writeformat if not set
- ast_channel_codec_set(sub->owner, !ast_strlen_zero(ast_channel_codec_get(bridged_chan)) ?
- ast_channel_codec_get(bridged_chan) : ast_format_get_name(ast_channel_writeformat(bridged_chan)));
+ ast_channel_lock(sub->owner);
+ ast_channel_codec_set(sub->owner, !ast_strlen_zero(ast_channel_codec_get(bridged_chan)) ?
+ ast_channel_codec_get(bridged_chan) : ast_format_get_name(ast_channel_writeformat(bridged_chan)));
+ ast_channel_unlock(sub->owner);
}
ast_channel_unref(bridged_chan);
} else if (ast_strlen_zero(ast_channel_codec_get(sub->owner))) {
//return if has no codec set
- ast_channel_unlock(sub->owner);
return;
}
ast_debug(5, "set format\n");
@@ -814,7 +824,9 @@ static void chan_voicemngr_modify_codec(struct chan_voicemngr_subchannel *sub) {
caps = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
if (caps) {
ast_format_cap_append(caps, map_rtpname_to_format((char *)ast_channel_codec_get(sub->owner)), 0);
+ ast_channel_lock(sub->owner);
ast_channel_nativeformats_set(sub->owner, caps);
+ ast_channel_unlock(sub->owner);
ao2_ref(caps, -1);
} else {
ao2_cleanup(caps);
@@ -842,7 +854,6 @@ static void chan_voicemngr_modify_codec(struct chan_voicemngr_subchannel *sub) {
if (data.mask & UBUS_DATA_CODEC_BIT) {
sub->updated_codec = 1;
}
- ast_channel_unlock(sub->owner);
}
}
@@ -851,8 +862,6 @@ static int chan_voicemngr_indicate(struct ast_channel *ast, int condition, const
struct chan_voicemngr_subchannel *sub = ast_channel_tech_pvt(ast);
struct chan_voicemngr_subchannel *sub_peer = chan_voicemngr_subchannel_get_peer(sub);
struct ast_bridge_channel *play_bridge_channel;
- struct ast_bridge *myBridge;
- struct ast_frame astFrame;
int res = 0;
ast_log(LOG_NOTICE, "Channel %s gets an indication, condition = %d, sub->channel_state: %s, sub->codec: %d,ast_channel_codec_get(ast): %s\n",
@@ -870,12 +879,16 @@ static int chan_voicemngr_indicate(struct ast_channel *ast, int condition, const
sub->channel_state = INCALL;
+ /* not needed as pjsip handles this indication as well, keep it here in case of regression.
// Tell all participants to re-sync RTP stream.
+ struct ast_bridge *myBridge;
+ struct ast_frame astFrame;
myBridge = ast_channel_internal_bridge(ast);
memset(&astFrame, 0, sizeof astFrame);
astFrame.frametype = AST_FRAME_CONTROL;
astFrame.subclass.integer = AST_CONTROL_SRCUPDATE;
ast_bridge_queue_everyone_else(myBridge, NULL, &astFrame);
+ */
pvt_unlock(sub->parent);
break;
@@ -933,11 +946,6 @@ static int chan_voicemngr_indicate(struct ast_channel *ast, int condition, const
} else {
ast_log(LOG_ERROR, "can't get the peer channel, unattended call transfer will not be proceeded\n");
}
-
- } else if(sub->channel_state == INCALL) {
- /* for some specific scenarios, unhold was wrong indicate as unhold_for_transfer from pjsip
- just redirect to unhold to make less effect to transfer */
- ast_indicate(sub->owner, AST_CONTROL_UNHOLD);
}
break;
case AST_CONTROL_TRANSFER:
@@ -2895,7 +2903,10 @@ static void handle_hookflash(struct chan_voicemngr_subchannel *sub, struct chan_
if (owner) {
if(bridge){
AST_LIST_TRAVERSE(&bridge->channels, bridge_channel, entry) {
- ast_indicate(bridge_channel->chan, AST_CONTROL_HOLD);
+ if ((strstr(ast_channel_name(bridge_channel->chan), "TELCHAN") != NULL) && (strcmp(ast_channel_name(bridge_channel->chan), ast_channel_name(owner)))) {
+ // only indicate other local channels, as asterisk will handle the pjsip channels with the queued frame.
+ ast_indicate(bridge_channel->chan, AST_CONTROL_HOLD);
+ }
}
}
@@ -2937,8 +2948,11 @@ static void handle_hookflash(struct chan_voicemngr_subchannel *sub, struct chan_
if (peer_owner) {
if(bridge){
AST_LIST_TRAVERSE(&bridge->channels, bridge_channel, entry) {
- ast_indicate(bridge_channel->chan, AST_CONTROL_UNHOLD);
- ast_log(LOG_NOTICE,"Pick up old unhold\n");
+ if ((strstr(ast_channel_name(bridge_channel->chan), "TELCHAN") != NULL) && (strcmp(ast_channel_name(bridge_channel->chan), ast_channel_name(peer_owner)))) {
+ // only indicate other local channels, as asterisk will handle the pjsip channels with the queued frame.
+ ast_indicate(bridge_channel->chan, AST_CONTROL_UNHOLD);
+ ast_log(LOG_NOTICE,"Pick up old unhold\n");
+ }
}
}
chan_voicemngr_send_ubus_event("CALL_UNHOLD",p->line_id);
--
GitLab