Race condition between transport destroy and acquire (#2470)

* Handle race condition between transport_idle_callback() and pjsip_tpmgr_acquire_transport2(). * Add transport destroy state check as additional of transport shutdown state check
pjsip · Nov 20, 2020 · 90a16c5 · 90a16c5
1 parent 0d911f8
commit 90a16c5
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 7 deletions.
diff --git a/pjsip/src/pjsip/sip_transaction.c b/pjsip/src/pjsip/sip_transaction.c
@@ -2443,7 +2443,7 @@ static void tsx_update_transport( pjsip_transaction *tsx,
 	pjsip_transport_add_ref(tp);
 	pjsip_transport_add_state_listener(tp, &tsx_tp_state_callback, tsx,
 					    &tsx->tp_st_key);
-        if (tp->is_shutdown) {
+	if (tp->is_shutdown || tp->is_destroying) {
 	    pjsip_transport_state_info info;
 
 	    pj_bzero(&info, sizeof(info));

diff --git a/pjsip/src/pjsip/sip_transport.c b/pjsip/src/pjsip/sip_transport.c
@@ -1071,6 +1071,19 @@ static void transport_idle_callback(pj_timer_heap_t *timer_heap,
 	return;
 
     entry->id = PJ_FALSE;
+
+    /* Set is_destroying flag under transport manager mutex to avoid
+     * race condition with pjsip_tpmgr_acquire_transport2().
+     */
+    pj_lock_acquire(tp->tpmgr->lock);
+    if (pj_atomic_get(tp->ref_cnt) == 0) {
+	tp->is_destroying = PJ_TRUE;
+    } else {
+	pj_lock_release(tp->tpmgr->lock);
+	return;
+    }
+    pj_lock_release(tp->tpmgr->lock);
+
     pjsip_transport_destroy(tp);
 }
 
@@ -1392,8 +1405,8 @@ PJ_DEF(pj_status_t) pjsip_transport_shutdown2(pjsip_transport *tp,
     mgr = tp->tpmgr;
     pj_lock_acquire(mgr->lock);
 
-    /* Do nothing if transport is being shutdown already */
-    if (tp->is_shutdown) {
+    /* Do nothing if transport is being shutdown/destroyed already */
+    if (tp->is_shutdown || tp->is_destroying) {
 	pj_lock_release(mgr->lock);
 	pj_lock_release(tp->lock);
 	return PJ_SUCCESS;
@@ -2256,6 +2269,13 @@ PJ_DEF(pj_status_t) pjsip_tpmgr_acquire_transport2(pjsip_tpmgr *mgr,
 	    return PJSIP_ETPNOTSUITABLE;
 	}
 
+	/* Make sure the transport is not being destroyed */
+	if (seltp->is_destroying) {
+	    pj_lock_release(mgr->lock);
+	    TRACE_((THIS_FILE,"Transport to be acquired is being destroyed"));
+	    return PJ_ENOTFOUND;
+	}
+
 	/* We could also verify that the destination address is reachable
 	 * from this transport (i.e. both are equal), but if application
 	 * has requested a specific transport to be used, assume that
@@ -2311,8 +2331,10 @@ PJ_DEF(pj_status_t) pjsip_tpmgr_acquire_transport2(pjsip_tpmgr *mgr,
 	    if (tp_entry) {
 		transport *tp_iter = tp_entry;
 		do {
-		    /* Don't use transport being shutdown */
-		    if (!tp_iter->tp->is_shutdown) {
+		    /* Don't use transport being shutdown/destroyed */
+		    if (!tp_iter->tp->is_shutdown &&
+			!tp_iter->tp->is_destroying)
+		    {
 			if (sel && sel->type == PJSIP_TPSELECTOR_LISTENER &&
 			    sel->u.listener)
 			{
@@ -2382,7 +2404,7 @@ PJ_DEF(pj_status_t) pjsip_tpmgr_acquire_transport2(pjsip_tpmgr *mgr,
 	    TRACE_((THIS_FILE, "Transport found but from different listener"));
 	}
 
-	if (tp_ref!=NULL && !tp_ref->is_shutdown) {
+	if (tp_ref!=NULL && !tp_ref->is_shutdown && !tp_ref->is_destroying) {
 	    /*
 	     * Transport found!
 	     */
@@ -2624,7 +2646,7 @@ PJ_DEF(pj_status_t) pjsip_transport_add_state_listener (
 
     PJ_ASSERT_RETURN(tp && cb && key, PJ_EINVAL);
 
-    if (tp->is_shutdown) {
+    if (tp->is_shutdown || tp->is_destroying) {
 	*key = NULL;
 	return PJ_EINVALIDOP;
     }