[Asterisk-code-review] chan sip.c: Fix reinviteid deadlock potential. (asterisk[13])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: chan_sip.c: Fix reinviteid deadlock potential.
......................................................................


chan_sip.c: Fix reinviteid deadlock potential.

This patch is part of a series to resolve deadlocks in chan_sip.c.

Stopping a scheduled event can result in a deadlock if the scheduled event
is running when you try to stop the event.  If you hold a lock needed by
the scheduled event while trying to stop the scheduled event then a
deadlock can happen.  The general strategy for resolving the deadlock
potential is to push the actual starting and stopping of the scheduled
events off onto the scheduler/do_monitor() thread by scheduling an
immediate one shot scheduled event.  Some restructuring may be needed
because the code may assume that the start/stop of the scheduled events is
immediate.

ASTERISK-25023

Change-Id: I9c11b9d597468f63916c99e1dabff9f4a46f84c1
---
M channels/chan_sip.c
1 file changed, 37 insertions(+), 10 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, approved; Verified
  George Joseph: Looks good to me, but someone else must approve



diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index c7cab1c..f369df4 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -7041,19 +7041,42 @@
 	return 0;
 }
 
+/* Run by the sched thread. */
 static int reinvite_timeout(const void *data)
 {
 	struct sip_pvt *dialog = (struct sip_pvt *) data;
-	struct ast_channel *owner = sip_pvt_lock_full(dialog);
+	struct ast_channel *owner;
+
+	owner = sip_pvt_lock_full(dialog);
 	dialog->reinviteid = -1;
 	check_pendings(dialog);
 	if (owner) {
 		ast_channel_unlock(owner);
 		ast_channel_unref(owner);
 	}
-	ao2_unlock(dialog);
-	dialog_unref(dialog, "unref for reinvite timeout");
+	sip_pvt_unlock(dialog);
+	dialog_unref(dialog, "reinviteid complete");
 	return 0;
+}
+
+/* Run by the sched thread. */
+static int __stop_reinviteid(const void *data)
+{
+	struct sip_pvt *pvt = (void *) data;
+
+	AST_SCHED_DEL_UNREF(sched, pvt->reinviteid,
+		dialog_unref(pvt, "Stop scheduled reinviteid"));
+	dialog_unref(pvt, "Stop reinviteid action");
+	return 0;
+}
+
+static void stop_reinviteid(struct sip_pvt *pvt)
+{
+	dialog_ref(pvt, "Stop reinviteid action");
+	if (ast_sched_add(sched, 0, __stop_reinviteid, pvt) < 0) {
+		/* Uh Oh.  Expect bad behavior. */
+		dialog_unref(pvt, "Failed to schedule stop reinviteid action");
+	}
 }
 
 /*! \brief  sip_hangup: Hangup SIP call
@@ -7247,7 +7270,12 @@
 				 * So, just in case, check for pending actions after a bit of time to trigger the pending
 				 * bye that we are setting above */
 				if (p->ongoing_reinvite && p->reinviteid < 0) {
-					p->reinviteid = ast_sched_add(sched, 32 * p->timer_t1, reinvite_timeout, dialog_ref(p, "ref for reinvite_timeout"));
+					p->reinviteid = ast_sched_add(sched, 32 * p->timer_t1,
+						reinvite_timeout, dialog_ref(p, "Schedule reinviteid"));
+					if (p->reinviteid < 0) {
+						/* Uh Oh.  Expect bad behavior. */
+						dialog_unref(p, "Failed to schedule reinviteid");
+					}
 				}
 			}
 		}
@@ -22935,13 +22963,14 @@
 		if (p->reinviteid > -1) {
 			/* Outstanding p->reinviteid timeout, so wait... */
 			return;
-		} else if (p->invitestate == INV_PROCEEDING || p->invitestate == INV_EARLY_MEDIA) {
+		}
+		if (p->invitestate == INV_PROCEEDING || p->invitestate == INV_EARLY_MEDIA) {
 			/* if we can't BYE, then this is really a pending CANCEL */
 			p->invitestate = INV_CANCELLED;
 			transmit_request(p, SIP_CANCEL, p->lastinvite, XMIT_RELIABLE, FALSE);
 			/* If the cancel occurred on an initial invite, cancel the pending BYE */
 			if (!ast_test_flag(&p->flags[1], SIP_PAGE2_DIALOG_ESTABLISHED)) {
-				ast_clear_flag(&p->flags[0], SIP_PENDINGBYE);
+				ast_clear_flag(&p->flags[0], SIP_PENDINGBYE | SIP_NEEDREINVITE);
 			}
 			/* Actually don't destroy us yet, wait for the 487 on our original
 			   INVITE, but do set an autodestruct just in case we never get it. */
@@ -22957,7 +22986,7 @@
 			}
 			/* Perhaps there is an SD change INVITE outstanding */
 			transmit_request_with_auth(p, SIP_BYE, 0, XMIT_RELIABLE, TRUE);
-			ast_clear_flag(&p->flags[0], SIP_PENDINGBYE);
+			ast_clear_flag(&p->flags[0], SIP_PENDINGBYE | SIP_NEEDREINVITE);
 		}
 		sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
 	} else if (ast_test_flag(&p->flags[0], SIP_NEEDREINVITE)) {
@@ -23249,9 +23278,7 @@
 
 	if ((resp >= 200 && reinvite)) {
 		p->ongoing_reinvite = 0;
-		if (p->reinviteid > -1) {
-			AST_SCHED_DEL_UNREF(sched, p->reinviteid, dialog_unref(p, "unref dialog for reinvite timeout because of a final response"));
-		}
+		stop_reinviteid(p);
 	}
 
 	/* Final response, clear out pending invite */

-- 
To view, visit https://gerrit.asterisk.org/2398
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I9c11b9d597468f63916c99e1dabff9f4a46f84c1
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>