From 22f0a53ce44e5541433917226bf2c03fa4f44ff7 Mon Sep 17 00:00:00 2001 From: George Joseph Date: Tue, 12 May 2026 18:30:04 -0600 Subject: [PATCH] channel.c: Move setting RTP stats from ast_softhangup to ast_ari_channels_hangup. The original trigger for setting the RTP stats in ast_softhangup() came from an ARI issue where stats weren't being set in time to be reported on STASIS_END events. The thought was that setting them in a common place like ast_softhangup() would ensure the stats were set in possibly other scenarios. Unfortunately, setting the RTP stats variables in ast_softhangup() broke ABI as it required that no channel locks be held which was not the case earlier. Given that the original issue was ARI, we can move setting the stats to ast_ari_channels_hangup() in resource_channels just before it calls ast_softhangup(). This might not catch all cases of the stats not being set, but it won't break ABI or deadlock either. Resolves: #1928 --- include/asterisk/channel.h | 9 +-------- main/channel.c | 34 ---------------------------------- res/ari/resource_channels.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 42 deletions(-) diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h index c24f8ed6e4..90a7dc293d 100644 --- a/include/asterisk/channel.h +++ b/include/asterisk/channel.h @@ -1654,14 +1654,7 @@ void ast_softhangup_all(void); * (use this if you are trying to * safely hangup a channel managed by another thread. * - * \warning The channel passed to this function must NOT be locked. - * ast_softhangup() calls ast_rtp_instance_set_stats_vars() to set RTP QOS variables. - * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will - * attempt to lock the bridge peer as well as this channel. This can cause - * a lock inversion if we already have this channel locked and another - * thread tries to set bridge variables on the peer because it will have - * locked the peer first, then this channel. For this reason, we must - * NOT have the channel locked when we call ast_softhangup(). + * \note The channel passed to this function does not need to be locked. * * \return Returns 0 regardless */ diff --git a/main/channel.c b/main/channel.c index 146e2115ee..a305978e01 100644 --- a/main/channel.c +++ b/main/channel.c @@ -2464,41 +2464,7 @@ int ast_softhangup(struct ast_channel *chan, int cause) RAII_VAR(struct ast_json *, blob, NULL, ast_json_unref); int res; int tech_cause = 0; - struct ast_rtp_glue *glue; - struct ast_rtp_instance *rtp = NULL; - const struct ast_channel_tech *tech; - - /* - * Only hold the channel lock long enough to get the rtp instance. - * glue->get_rtp_info() will bump the refcount on it. - */ - ast_channel_lock(chan); - tech = ast_channel_tech(chan); - glue = ast_rtp_instance_get_glue(tech->type); - if (glue) { - glue->get_rtp_info(chan, &rtp); - } - ast_channel_unlock(chan); - /* - * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will - * attempt to lock the bridge peer as well as this channel. This can cause - * a lock inversion if we already have this channel locked and another - * thread tries to set bridge variables on the peer because it will have - * locked the peer first, then this channel. For this reason, we must - * NOT have the channel locked when we call ast_rtp_instance_set_stats_vars(). - * This should be safe since glue->get_rtp_info() will have bumped the - * refcount on the rtp instance so it can't go away while the channel - * is unlocked. - */ - if (rtp) { - ast_rtp_instance_set_stats_vars(chan, rtp); - ao2_ref(rtp, -1); - } - - /* - * Now it's safe to lock the channel again. - */ ast_channel_lock(chan); res = ast_softhangup_nolock(chan, cause); diff --git a/res/ari/resource_channels.c b/res/ari/resource_channels.c index 1077eebd49..527803e952 100644 --- a/res/ari/resource_channels.c +++ b/res/ari/resource_channels.c @@ -931,6 +931,9 @@ void ast_ari_channels_hangup(struct ast_variable *headers, { RAII_VAR(struct ast_channel *, chan, NULL, ao2_cleanup); int cause; + struct ast_rtp_glue *glue; + struct ast_rtp_instance *rtp = NULL; + const struct ast_channel_tech *tech; chan = ast_channel_get_by_name(args->channel_id); if (chan == NULL) { @@ -969,6 +972,34 @@ void ast_ari_channels_hangup(struct ast_variable *headers, } ast_channel_hangupcause_set(chan, cause); + + /* + * Only hold the channel lock long enough to get the rtp instance. + * glue->get_rtp_info() will bump the refcount on it. + */ + ast_channel_lock(chan); + tech = ast_channel_tech(chan); + glue = ast_rtp_instance_get_glue(tech->type); + if (glue) { + glue->get_rtp_info(chan, &rtp); + } + ast_channel_unlock(chan); + /* + * If this channel is in a bridge, ast_rtp_instance_set_stats_vars() will + * attempt to lock the bridge peer as well as this channel. This can cause + * a lock inversion if we already have this channel locked and another + * thread tries to set bridge variables on the peer because it will have + * locked the peer first, then this channel. For this reason, we must + * NOT have the channel locked when we call ast_rtp_instance_set_stats_vars(). + * This should be safe since glue->get_rtp_info() will have bumped the + * refcount on the rtp instance so it can't go away while the channel + * is unlocked. + */ + if (rtp) { + ast_rtp_instance_set_stats_vars(chan, rtp); + ao2_ref(rtp, -1); + } + ast_softhangup(chan, AST_SOFTHANGUP_EXPLICIT); ast_ari_response_no_content(response); -- 2.47.3