Changeset 6069

Timestamp:

Sep 12, 2019 8:46:05 AM (5 years ago)

Author:

nanang

Message:

Fix #2230: Fixed crash in STUN session due to race condition which leads to premature tdata destroy.

Location:

pjproject/trunk/pjnath

Files:

• include/pjnath/stun_session.h (modified) (1 diff)
• src/pjnath/stun_session.c (modified) (17 diffs)

pjproject/trunk/pjnath/include/pjnath/stun_session.h

@@ -349 +349 @@
     pj_uint32_t          msg_magic;     /**< Message magic.                 */
     pj_uint8_t           msg_key[12];   /**< Message/transaction key.       */
+    
+    pj_grp_lock_t       *grp_lock;      /**< Group lock (for resp cache).   */
 
     pj_stun_req_cred_info auth_info;    /**< Credential info                */

pjproject/trunk/pjnath/src/pjnath/stun_session.c

@@ -77 +77 @@
 static void stun_tsx_on_destroy(pj_stun_client_tsx *tsx);
 static void stun_sess_on_destroy(void *comp);
+static void destroy_tdata(pj_stun_tx_data *tdata, pj_bool_t force);
 
 static pj_stun_tsx_cb tsx_cb = 
@@ -154 +155 @@
         pj_grp_lock_acquire(sess->grp_lock);
         tsx_erase(sess, tdata);
-        pj_pool_release(tdata->pool);
+        destroy_tdata(tdata, PJ_TRUE);
         pj_grp_lock_release(sess->grp_lock);
     }
@@ -161 +162 @@
 
     TRACE_((THIS_FILE, "STUN transaction %p destroyed", tsx));
+}
+
+static void tdata_on_destroy(void *arg)
+{
+    pj_stun_tx_data *tdata = (pj_stun_tx_data*)arg;
+
+    pj_pool_safe_release(&tdata->pool);
 }
 
@@ -168 +176 @@
             force, tdata->client_tsx));
 
+    /* STUN session may have been destroyed, except when tdata is cached. */
+
     if (tdata->res_timer.id != PJ_FALSE) {
         pj_timer_heap_cancel_if_active(tdata->sess->cfg->timer_heap,
-                                       &tdata->res_timer, PJ_FALSE);
-        pj_list_erase(tdata);
+                                       &tdata->res_timer, PJ_FALSE);
     }
 
@@ -180 +189 @@
             pj_stun_client_tsx_set_data(tdata->client_tsx, NULL);
         }
-        pj_pool_release(tdata->pool);
+        if (tdata->grp_lock) {
+            pj_grp_lock_dec_ref(tdata->sess->grp_lock);
+            pj_grp_lock_dec_ref(tdata->grp_lock);
+        } else {
+            tdata_on_destroy(tdata);
+        }
 
     } else {
@@ -189 +203 @@
 
         } else {
-            pj_pool_release(tdata->pool);
+            pj_list_erase(tdata);
+            if (tdata->grp_lock) {
+                pj_grp_lock_dec_ref(tdata->sess->grp_lock);
+                pj_grp_lock_dec_ref(tdata->grp_lock);
+            } else {
+                tdata_on_destroy(tdata);
+            }
         }
     }
@@ -226 +246 @@
     PJ_LOG(5,(SNAME(tdata->sess), "Response cache deleted"));
 
-    pj_list_erase(tdata);
+    destroy_tdata(tdata, PJ_FALSE);
     pj_grp_lock_release(sess->grp_lock);
 
-    destroy_tdata(tdata, PJ_FALSE);
 }
 
@@ -565 +584 @@
     }
 
-    while (!pj_list_empty(&sess->cached_response_list)) {
-        pj_stun_tx_data *tdata = sess->cached_response_list.next;
-        destroy_tdata(tdata, PJ_TRUE);
-    }
-
-    if (sess->rx_pool) {
-        pj_pool_release(sess->rx_pool);
-        sess->rx_pool = NULL;
-    }
-
-    pj_pool_release(sess->pool);
+    pj_pool_safe_release(&sess->rx_pool);
+    pj_pool_safe_release(&sess->pool);
 
     TRACE_((THIS_FILE, "STUN session %p destroyed", sess));
@@ -599 +609 @@
     sess->is_destroying = PJ_TRUE;
 
-    /* We need to stop transactions and cached response because they are
+    /* We need to stop transactions because they are
      * holding the group lock's reference counter while retransmitting.
      */
@@ -609 +619 @@
     }
 
-    tdata = sess->cached_response_list.next;
-    while (tdata != &sess->cached_response_list) {
-        pj_timer_heap_cancel_if_active(tdata->sess->cfg->timer_heap,
-                                       &tdata->res_timer, PJ_FALSE);
-        tdata = tdata->next;
+    /* Destroy cached response within session lock protection to avoid
+     * race scenario with on_cache_timeout().
+     */
+    while (!pj_list_empty(&sess->cached_response_list)) {
+        pj_stun_tx_data *tdata = sess->cached_response_list.next;
+        destroy_tdata(tdata, PJ_TRUE);
     }
 
@@ -805 +816 @@
 on_error:
     if (tdata)
-        pj_pool_release(tdata->pool);
+        pj_pool_safe_release(&tdata->pool);
     pj_grp_lock_release(sess->grp_lock);
     return status;
@@ -836 +847 @@
                                 NULL, &tdata->msg);
     if (status != PJ_SUCCESS) {
-        pj_pool_release(tdata->pool);
+        pj_pool_safe_release(&tdata->pool);
         pj_grp_lock_release(sess->grp_lock);
         return status;
@@ -875 +886 @@
                                          err_code, err_msg, &tdata->msg);
     if (status != PJ_SUCCESS) {
-        pj_pool_release(tdata->pool);
+        pj_pool_safe_release(&tdata->pool);
         pj_grp_lock_release(sess->grp_lock);
         return status;
@@ -1008 +1019 @@
 
     } else {
+        /* Otherwise for non-request message, send directly to transport. */
         if (cache_res && 
             (PJ_STUN_IS_SUCCESS_RESPONSE(tdata->msg->hdr.type) ||
@@ -1014 +1026 @@
             /* Requested to keep the response in the cache */
             pj_time_val timeout;
+
+            status = pj_grp_lock_create(tdata->pool, NULL, &tdata->grp_lock);
+            if (status != PJ_SUCCESS) {
+                pj_stun_msg_destroy_tdata(sess, tdata);
+                LOG_ERR_(sess, "Error creating group lock", status);
+                goto on_return;
+            }
+            pj_grp_lock_add_ref(tdata->grp_lock);
+            pj_grp_lock_add_handler(tdata->grp_lock, tdata->pool, tdata,
+                                    &tdata_on_destroy);
+
+            /* Also add ref session group lock to make sure that the session
+             * is still valid when cache timeout callback is called.
+             */
+            pj_grp_lock_add_ref(sess->grp_lock);
             
             pj_memset(&tdata->res_timer, 0, sizeof(tdata->res_timer));
@@ -1025 +1052 @@
                                                        &tdata->res_timer,
                                                        &timeout, PJ_TRUE,
-                                                       sess->grp_lock);
+                                                       tdata->grp_lock);
             if (status != PJ_SUCCESS) {
                 pj_stun_msg_destroy_tdata(sess, tdata);
@@ -1034 +1061 @@
             pj_list_push_back(&sess->cached_response_list, tdata);
         }
-    
-        /* Otherwise for non-request message, send directly to transport. */
+
+        /* Send to transport directly. */
         status = sess->cb.on_send_msg(sess, token, tdata->pkt, 
                                       tdata->pkt_size, server, addr_len);