[Asterisk-code-review] sig_pri: Fix deadlock caused by sig_pri_queue_hangup (asterisk[13])

[Friendly Automation]

Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/13505 )

Change subject: sig_pri:  Fix deadlock caused by sig_pri_queue_hangup
......................................................................

sig_pri:  Fix deadlock caused by sig_pri_queue_hangup

The change to add setting hangupsource to sig_pri_queue_hangup()
made in https://gerrit.asterisk.org/c/asterisk/+/12857 casued
deadlocks when a hangup request was received from the core at the
same time a hanguprequest was received from the remote end via the
D channel.

Although the PRI's channel private structure was being unlocked
before setting the hangupsource, the PRI's own lock was still being
held during the process.  If channel actions were also coming from
the core, a deadlock on the PRI could result.  This deadlock could
then escalate to the entire DAHDI subsystem via DAHDI's global
interface list lock, especially if someone used the PRI CLI commands.

Fix:

* We now unlock the PRI as well as the PRI's channel private
  structure before setting the hangupsource, then relock both
  afterwards.

ASTERISK-28605
Reported by: Dirk Wendland

Change-Id: Id74aaa5d4e3746063dbe9deed188eb65193cb9c9
---
M channels/sig_pri.c
1 file changed, 7 insertions(+), 1 deletion(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Frederic LE FOLL: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Friendly Automation: Approved for Submit



diff --git a/channels/sig_pri.c b/channels/sig_pri.c
index 90dc051..3fda34a 100644
--- a/channels/sig_pri.c
+++ b/channels/sig_pri.c
@@ -1387,6 +1387,10 @@
  * \note Assumes the pri->lock is already obtained.
  * \note Assumes the sig_pri_lock_private(pri->pvts[chanpos]) is already obtained.
  *
+ * \note The unlocking/locking sequence now present has been stress tested
+ *       without deadlocks.  Please don't change it without consulting
+ *       core development team members.
+ *
  * \return Nothing
  */
 static void sig_pri_queue_hangup(struct sig_pri_span *pri, int chanpos)
@@ -1404,9 +1408,11 @@
 		ast_queue_hangup(owner);
 		ast_channel_unlock(owner);
 
-		/* Tell the CDR this DAHDI channel hung up */
 		sig_pri_unlock_private(pri->pvts[chanpos]);
+		ast_mutex_unlock(&pri->lock);
+		/* Tell the CDR this DAHDI channel hung up */
 		ast_set_hangupsource(owner, ast_channel_name(owner), 0);
+		ast_mutex_lock(&pri->lock);
 		sig_pri_lock_private(pri->pvts[chanpos]);
 
 		ao2_ref(owner, -1);

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13505
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Id74aaa5d4e3746063dbe9deed188eb65193cb9c9
Gerrit-Change-Number: 13505
Gerrit-PatchSet: 2
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Frederic LE FOLL <frederic.lefoll at c-s.fr>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200108/cb70c78d/attachment.html>