chan_iax2: Prevent deadlock between hangup and sending lagrq/ping

channels/chan_iax.c: Prevent the deadlock between iax2_hangup and send_lagrq/
send_ping. This deadlock happens because the scheduled task send_lagrq(or
send_ping) starts execution after the call hangup procedure starts but before
it deletes the tasks in the scheduler.

The solution is to delete scheduled lagrq (and ping) task asynchronously
(i.e. schedule AST_SCHED_DEL for these tasks); By this, AST_SCHED_DEL will
be called in a new context (doesn't have callno locked).

This commit also cleans up the procedure of sending LAGRQ and PING.

main/sched.c: Do not assert when deleting non existant entry from scheduler.
This assert seems to be the reason for a lot of awkward code to avoid it.

ASTERISK-24983 #close
Reported by: Y Ateya

Change-Id: I03bec1fc8faacb89630269e935fa667c6d6c080c

1 files changed, 84 insertions, 43 deletions

diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index 8d3e7f145..efdcb134d 100644
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -395,8 +395,6 @@ static int (*iax2_regfunk)(const char *username, int onoff) = NULL;
 static struct io_context *io;
 static struct ast_sched_context *sched;
 
-#define DONT_RESCHEDULE -2
-
 static iax2_format iax2_capability = IAX_CAPABILITY_FULLBANDWIDTH;
 
 static int iaxdebug = 0;
@@ -864,6 +862,8 @@ struct chan_iax2_pvt {
 	int frames_dropped;
 	/*! received frame count: (just for stats) */
 	int frames_received;
+	/*! Destroying this call initiated. */
+	int destroy_initiated;
 	/*! num bytes used for calltoken ie, even an empty ie should contain 2 */
 	unsigned char calltoken_ie_len;
 	/*! hold all signaling frames from the pbx thread until we have a destination callno */
@@ -1671,23 +1671,48 @@ static int iax2_sched_add(struct ast_sched_context *con, int when,
 	return ast_sched_add(con, when, callback, data);
 }
 
+/*
+ * \brief Acquire the iaxsl[callno] if call exists and not having ongoing hangup.
+ * \param callno Call number to lock.
+ * \return 0 If call disappeared or has ongoing hangup procedure. 1 If call found and mutex is locked.
+ */
+static int iax2_lock_callno_unless_destroyed(int callno)
+{
+	ast_mutex_lock(&iaxsl[callno]);
+
+	/* We acquired the lock; but the call was already destroyed (we came after full hang up procedures)
+	 * or destroy initiated (in middle of hang up procedure. */
+	if (!iaxs[callno] || iaxs[callno]->destroy_initiated) {
+		ast_debug(3, "I wanted to lock callno %d, but it is dead or going to die.\n", callno);
+		ast_mutex_unlock(&iaxsl[callno]);
+		return 0;
+	}
+
+	/* Lock acquired, and callno is alive and kicking. */
+	return 1;
+}
+
 static int send_ping(const void *data);
 
 static void __send_ping(const void *data)
 {
-	int callno = (long) data;
+	int callno = PTR_TO_CALLNO(data);
 
-	ast_mutex_lock(&iaxsl[callno]);
+	if (iax2_lock_callno_unless_destroyed(callno) == 0) {
+		ast_debug(3, "Hangup initiated on call %d, aborting __send_ping\n", callno);
+		return;
+	}
 
-	if (iaxs[callno]) {
-		if (iaxs[callno]->peercallno) {
-			send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_PING, 0, NULL, 0, -1);
-			if (iaxs[callno]->pingid != DONT_RESCHEDULE) {
-				iaxs[callno]->pingid = iax2_sched_add(sched, ping_time * 1000, send_ping, data);
-			}
-		}
-	} else {
-		ast_debug(1, "I was supposed to send a PING with callno %d, but no such call exists.\n", callno);
+	/* Mark pingid as invalid scheduler id. */
+	iaxs[callno]->pingid = -1;
+
+	/* callno is now locked. */
+	if (iaxs[callno]->peercallno) {
+		/* Send PING packet. */
+		send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_PING, 0, NULL, 0, -1);
+
+		/* Schedule sending next ping. */
+		iaxs[callno]->pingid = iax2_sched_add(sched, ping_time * 1000, send_ping, data);
 	}
 
 	ast_mutex_unlock(&iaxsl[callno]);
@@ -1695,13 +1720,6 @@ static void __send_ping(const void *data)
 
 static int send_ping(const void *data)
 {
-	int callno = (long) data;
-	ast_mutex_lock(&iaxsl[callno]);
-	if (iaxs[callno] && iaxs[callno]->pingid != DONT_RESCHEDULE) {
-		iaxs[callno]->pingid = -1;
-	}
-	ast_mutex_unlock(&iaxsl[callno]);
-
 #ifdef SCHED_MULTITHREADED
 	if (schedule_action(__send_ping, data))
 #endif
@@ -1742,19 +1760,23 @@ static int send_lagrq(const void *data);
 
 static void __send_lagrq(const void *data)
 {
-	int callno = (long) data;
+	int callno = PTR_TO_CALLNO(data);
 
-	ast_mutex_lock(&iaxsl[callno]);
+	if (iax2_lock_callno_unless_destroyed(callno) == 0) {
+		ast_debug(3, "Hangup initiated on call %d, aborting __send_lagrq\n", callno);
+		return;
+	}
 
-	if (iaxs[callno]) {
-		if (iaxs[callno]->peercallno) {
-			send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_LAGRQ, 0, NULL, 0, -1);
-			if (iaxs[callno]->lagid != DONT_RESCHEDULE) {
-				iaxs[callno]->lagid = iax2_sched_add(sched, lagrq_time * 1000, send_lagrq, data);
-			}
-		}
-	} else {
-		ast_debug(1, "I was supposed to send a LAGRQ with callno %d, but no such call exists.\n", callno);
+	/* Mark lagid as invalid scheduler id. */
+	iaxs[callno]->lagid = -1;
+
+	/* callno is now locked. */
+	if (iaxs[callno]->peercallno) {
+		/* Send LAGRQ packet. */
+		send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_LAGRQ, 0, NULL, 0, -1);
+
+		/* Schedule sending next lagrq. */
+		iaxs[callno]->lagid = iax2_sched_add(sched, lagrq_time * 1000, send_lagrq, data);
 	}
 
 	ast_mutex_unlock(&iaxsl[callno]);
@@ -1762,13 +1784,6 @@ static void __send_lagrq(const void *data)
 
 static int send_lagrq(const void *data)
 {
-	int callno = (long) data;
-	ast_mutex_lock(&iaxsl[callno]);
-	if (iaxs[callno] && iaxs[callno]->lagid != DONT_RESCHEDULE) {
-		iaxs[callno]->lagid = -1;
-	}
-	ast_mutex_unlock(&iaxsl[callno]);
-
 #ifdef SCHED_MULTITHREADED
 	if (schedule_action(__send_lagrq, data))
 #endif
@@ -2052,6 +2067,16 @@ static int iax2_getpeername(struct ast_sockaddr addr, char *host, int len)
 	return res;
 }
 
+/* Call AST_SCHED_DEL on a scheduled task if it is found in scheduler. */
+static int iax2_delete_from_sched(const void* data)
+{
+	int sched_id = (int)(long)data;
+
+	AST_SCHED_DEL(sched, sched_id);
+
+	return 0;
+}
+
 /*!\note Assumes the lock on the pvt is already held, when
  * iax2_destroy_helper() is called. */
 static void iax2_destroy_helper(struct chan_iax2_pvt *pvt)
@@ -2068,11 +2093,27 @@ static void iax2_destroy_helper(struct chan_iax2_pvt *pvt)
 
 		ast_clear_flag64(pvt, IAX_MAXAUTHREQ);
 	}
-	/* No more pings or lagrq's */
-	AST_SCHED_DEL_SPINLOCK(sched, pvt->pingid, &iaxsl[pvt->callno]);
-	pvt->pingid = DONT_RESCHEDULE;
-	AST_SCHED_DEL_SPINLOCK(sched, pvt->lagid, &iaxsl[pvt->callno]);
-	pvt->lagid = DONT_RESCHEDULE;
+
+
+	/* Mark call destroy initiated flag. */
+	pvt->destroy_initiated = 1;
+
+	/*
+	 * Schedule deleting the scheduled (but didn't run yet) PINGs or LAGRQs.
+	 * Already running tasks will be terminated because of destroy_initiated.
+	 *
+	 * Don't call AST_SCHED_DEL from this thread for pingid and lagid because
+	 * it leads to a deadlock between the scheduler thread callback locking
+	 * the callno mutex and this thread which holds the callno mutex one or
+	 * more times.  It is better to have another thread delete the scheduled
+	 * callbacks which doesn't lock the callno mutex.
+	 */
+	iax2_sched_add(sched, 0, iax2_delete_from_sched, (void*)(long)pvt->pingid);
+	iax2_sched_add(sched, 0, iax2_delete_from_sched, (void*)(long)pvt->lagid);
+
+	pvt->pingid = -1;
+	pvt->lagid = -1;
+
 	AST_SCHED_DEL(sched, pvt->autoid);
 	AST_SCHED_DEL(sched, pvt->authid);
 	AST_SCHED_DEL(sched, pvt->initid);