Attempt to fix the race condition in dialog locking.

git-svn-id: http://svn.pjsip.org/repos/pjproject/trunk@671 74dad513-b988-da41-8d7b-12977e46ad98

6 files changed, 140 insertions, 47 deletions

diff --git a/pjsip/include/pjsip/sip_dialog.h b/pjsip/include/pjsip/sip_dialog.h
index 123209be..191196a5 100644
--- a/pjsip/include/pjsip/sip_dialog.h
+++ b/pjsip/include/pjsip/sip_dialog.h
@@ -108,7 +108,7 @@ struct pjsip_dialog
     /* Dialog's system properties. */
     char		obj_name[PJ_MAX_OBJ_NAME];  /**< Standard id.	    */
     pj_pool_t	       *pool;	    /**< Dialog's pool.			    */
-    pj_mutex_t	       *mutex;	    /**< Dialog's mutex. Do not call!!
+    pj_mutex_t	       *mutex_;	    /**< Dialog's mutex. Do not call!!
 					 Use pjsip_dlg_inc_lock() instead!  */
     pjsip_user_agent   *ua;	    /**< User agent instance.		    */
     pjsip_endpoint     *endpt;	    /**< Endpoint instance.		    */
diff --git a/pjsip/include/pjsip/sip_ua_layer.h b/pjsip/include/pjsip/sip_ua_layer.h
index ca6db080..08e692ca 100644
--- a/pjsip/include/pjsip/sip_ua_layer.h
+++ b/pjsip/include/pjsip/sip_ua_layer.h
@@ -79,6 +79,25 @@ PJ_DECL(pj_status_t) pjsip_ua_init_module(pjsip_endpoint *endpt,
  */
 PJ_DECL(pjsip_user_agent*) pjsip_ua_instance(void);
 
+
+/**
+ * Lock the dialog's hash table. This function is normally called by
+ * dialog code only.
+ *
+ * @return		PJ_SUCCESS on success or the appropriate error code.
+ */
+PJ_DECL(pj_status_t) pjsip_ua_lock_dlg_table(void);
+
+
+/**
+ * Unlock the dialog's hash table. This function is normally called by
+ * dialog code only.
+ *
+ * @return		PJ_SUCCESS on success or the appropriate error code.
+ */
+PJ_DECL(pj_status_t) pjsip_ua_unlock_dlg_table(void);
+
+
 /**
  * Destroy the user agent layer.
  *
diff --git a/pjsip/src/pjsip-simple/evsub.c b/pjsip/src/pjsip-simple/evsub.c
index e5b4f526..a6f6b2ca 100644
--- a/pjsip/src/pjsip-simple/evsub.c
+++ b/pjsip/src/pjsip-simple/evsub.c
@@ -668,6 +668,9 @@ static pj_status_t evsub_create( pjsip_dialog *dlg,
 	return PJSIP_SIMPLE_ENOPKG;
 
 
+    /* Must lock dialog before using pool etc. */
+    pjsip_dlg_inc_lock(dlg);
+
     /* Init attributes: */
 
     sub = pj_pool_zalloc(dlg->pool, sizeof(struct pjsip_evsub));
@@ -714,8 +717,10 @@ static pj_status_t evsub_create( pjsip_dialog *dlg,
     /* Register as dialog usage: */
 
     status = pjsip_dlg_add_usage(dlg, &mod_evsub.mod, dlgsub_head);
-    if (status != PJ_SUCCESS)
+    if (status != PJ_SUCCESS) {
+	pjsip_dlg_dec_lock(dlg);
 	return status;
+    }
 
 
     PJ_LOG(5,(sub->obj_name, "%s subscription created, using dialog %s",
@@ -723,6 +728,7 @@ static pj_status_t evsub_create( pjsip_dialog *dlg,
 	      dlg->obj_name));
 
     *p_evsub = sub;
+    pjsip_dlg_dec_lock(dlg);
 
     return PJ_SUCCESS;
 }
diff --git a/pjsip/src/pjsip-ua/sip_inv.c b/pjsip/src/pjsip-ua/sip_inv.c
index 32947446..e363eb90 100644
--- a/pjsip/src/pjsip-ua/sip_inv.c
+++ b/pjsip/src/pjsip-ua/sip_inv.c
@@ -435,6 +435,9 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uac( pjsip_dialog *dlg,
     /* Verify arguments. */
     PJ_ASSERT_RETURN(dlg && p_inv, PJ_EINVAL);
 
+    /* Must lock dialog first */
+    pjsip_dlg_inc_lock(dlg);
+
     /* Normalize options */
     if (options & PJSIP_INV_REQUIRE_100REL)
 	options |= PJSIP_INV_SUPPORT_100REL;
@@ -444,7 +447,7 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uac( pjsip_dialog *dlg,
 
     /* Create the session */
     inv = pj_pool_zalloc(dlg->pool, sizeof(pjsip_inv_session));
-    PJ_ASSERT_RETURN(inv != NULL, PJ_ENOMEM);
+    pj_assert(inv != NULL);
 
     inv->pool = dlg->pool;
     inv->role = PJSIP_ROLE_UAC;
@@ -461,14 +464,18 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uac( pjsip_dialog *dlg,
     if (local_sdp) {
 	status = pjmedia_sdp_neg_create_w_local_offer(dlg->pool, local_sdp,
 						      &inv->neg);
-	if (status != PJ_SUCCESS)
+	if (status != PJ_SUCCESS) {
+	    pjsip_dlg_dec_lock(dlg);
 	    return status;
+	}
     }
 
     /* Register invite as dialog usage. */
     status = pjsip_dlg_add_usage(dlg, &mod_inv.mod, inv);
-    if (status != PJ_SUCCESS)
+    if (status != PJ_SUCCESS) {
+	pjsip_dlg_dec_lock(dlg);
 	return status;
+    }
 
     /* Increment dialog session */
     pjsip_dlg_inc_session(dlg, &mod_inv.mod);
@@ -476,6 +483,8 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uac( pjsip_dialog *dlg,
     /* Done */
     *p_inv = inv;
 
+    pjsip_dlg_dec_lock(dlg);
+
     PJ_LOG(5,(inv->obj_name, "UAC invite session created for dialog %s",
 	      dlg->obj_name));
 
@@ -840,6 +849,9 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uas( pjsip_dialog *dlg,
 		     msg->line.req.method.id == PJSIP_INVITE_METHOD,
 		     PJ_EINVALIDOP);
 
+    /* Lock dialog */
+    pjsip_dlg_inc_lock(dlg);
+
     /* Normalize options */
     if (options & PJSIP_INV_REQUIRE_100REL)
 	options |= PJSIP_INV_SUPPORT_100REL;
@@ -849,7 +861,7 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uas( pjsip_dialog *dlg,
 
     /* Create the session */
     inv = pj_pool_zalloc(dlg->pool, sizeof(pjsip_inv_session));
-    PJ_ASSERT_RETURN(inv != NULL, PJ_ENOMEM);
+    pj_assert(inv != NULL);
 
     inv->pool = dlg->pool;
     inv->role = PJSIP_ROLE_UAS;
@@ -872,8 +884,10 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uas( pjsip_dialog *dlg,
 	if (status == PJ_SUCCESS)
 	    status = pjmedia_sdp_validate(rem_sdp);
 
-	if (status != PJ_SUCCESS)
+	if (status != PJ_SUCCESS) {
+	    pjsip_dlg_dec_lock(dlg);
 	    return status;
+	}
     }
 
     /* Create negotiator. */
@@ -888,13 +902,17 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uas( pjsip_dialog *dlg,
 	status = PJ_SUCCESS;
     }
 
-    if (status != PJ_SUCCESS)
+    if (status != PJ_SUCCESS) {
+	pjsip_dlg_dec_lock(dlg);
 	return status;
+    }
 
     /* Register invite as dialog usage. */
     status = pjsip_dlg_add_usage(dlg, &mod_inv.mod, inv);
-    if (status != PJ_SUCCESS)
+    if (status != PJ_SUCCESS) {
+	pjsip_dlg_dec_lock(dlg);
 	return status;
+    }
 
     /* Increment session in the dialog. */
     pjsip_dlg_inc_session(dlg, &mod_inv.mod);
@@ -909,6 +927,7 @@ PJ_DEF(pj_status_t) pjsip_inv_create_uas( pjsip_dialog *dlg,
     inv->invite_tsx->mod_data[mod_inv.mod.id] = tsx_inv_data;
 
     /* Done */
+    pjsip_dlg_dec_lock(dlg);
     *p_inv = inv;
 
     PJ_LOG(5,(inv->obj_name, "UAS invite session created for dialog %s",
diff --git a/pjsip/src/pjsip/sip_dialog.c b/pjsip/src/pjsip/sip_dialog.c
index cc68ab12..3149be4c 100644
--- a/pjsip/src/pjsip/sip_dialog.c
+++ b/pjsip/src/pjsip/sip_dialog.c
@@ -80,7 +80,7 @@ static pj_status_t create_dialog( pjsip_user_agent *ua,
 
     pj_list_init(&dlg->inv_hdr);
 
-    status = pj_mutex_create_recursive(pool, "dlg%p", &dlg->mutex);
+    status = pj_mutex_create_recursive(pool, "dlg%p", &dlg->mutex_);
     if (status != PJ_SUCCESS)
 	goto on_error;
 
@@ -89,16 +89,18 @@ static pj_status_t create_dialog( pjsip_user_agent *ua,
     return PJ_SUCCESS;
 
 on_error:
-    if (dlg->mutex)
-	pj_mutex_destroy(dlg->mutex);
+    if (dlg->mutex_)
+	pj_mutex_destroy(dlg->mutex_);
     pjsip_endpt_release_pool(endpt, pool);
     return status;
 }
 
 static void destroy_dialog( pjsip_dialog *dlg )
 {
-    if (dlg->mutex)
-	pj_mutex_destroy(dlg->mutex);
+    if (dlg->mutex_) {
+	pj_mutex_destroy(dlg->mutex_);
+	dlg->mutex_ = NULL;
+    }
     pjsip_endpt_release_pool(dlg->endpt, dlg->pool);
 }
 
@@ -606,8 +608,7 @@ static pj_status_t unregister_and_destroy_dialog( pjsip_dialog *dlg )
     PJ_LOG(5,(dlg->obj_name, "Dialog destroyed"));
 
     /* Destroy this dialog. */
-    pj_mutex_destroy(dlg->mutex);
-    pjsip_endpt_release_pool(dlg->endpt, dlg->pool);
+    destroy_dialog(dlg);
 
     return PJ_SUCCESS;
 }
@@ -638,13 +639,13 @@ PJ_DEF(pj_status_t) pjsip_dlg_set_route_set( pjsip_dialog *dlg,
 
     PJ_ASSERT_RETURN(dlg, PJ_EINVAL);
 
-    pj_mutex_lock(dlg->mutex);
+    pjsip_dlg_inc_lock(dlg);
 
     /* Clear route set. */
     pj_list_init(&dlg->route_set);
 
     if (!route_set) {
-	pj_mutex_unlock(dlg->mutex);
+	pjsip_dlg_dec_lock(dlg);
 	return PJ_SUCCESS;
     }
 
@@ -658,8 +659,7 @@ PJ_DEF(pj_status_t) pjsip_dlg_set_route_set( pjsip_dialog *dlg,
 	r = r->next;
     }
 
-    pj_mutex_unlock(dlg->mutex);
-
+    pjsip_dlg_dec_lock(dlg);
     return PJ_SUCCESS;
 }
 
@@ -672,9 +672,9 @@ PJ_DEF(pj_status_t) pjsip_dlg_inc_session( pjsip_dialog *dlg,
 {
     PJ_ASSERT_RETURN(dlg && mod, PJ_EINVAL);
 
-    pj_mutex_lock(dlg->mutex);
+    pjsip_dlg_inc_lock(dlg);
     ++dlg->sess_count;
-    pj_mutex_unlock(dlg->mutex);
+    pjsip_dlg_dec_lock(dlg);
 
     PJ_LOG(5,(dlg->obj_name, "Session count inc to %d by %.*s",
 	      dlg->sess_count, (int)mod->name.slen, mod->name.ptr));
@@ -684,12 +684,17 @@ PJ_DEF(pj_status_t) pjsip_dlg_inc_session( pjsip_dialog *dlg,
 
 /*
  * Lock dialog and increment session counter temporarily
- * to prevent it from being deleted.
+ * to prevent it from being deleted. In addition, it must lock
+ * the user agent's dialog table first, to prevent deadlock.
  */
 PJ_DEF(void) pjsip_dlg_inc_lock(pjsip_dialog *dlg)
 {
-    pj_mutex_lock(dlg->mutex);
+    pjsip_ua_lock_dlg_table();
+
+    pj_mutex_lock(dlg->mutex_);
     dlg->sess_count++;
+
+    pjsip_ua_unlock_dlg_table();
 }
 
 
@@ -699,16 +704,20 @@ PJ_DEF(void) pjsip_dlg_inc_lock(pjsip_dialog *dlg)
  */
 PJ_DEF(void) pjsip_dlg_dec_lock(pjsip_dialog *dlg)
 {
+    pjsip_ua_lock_dlg_table();
+
     pj_assert(dlg->sess_count > 0);
     --dlg->sess_count;
 
     if (dlg->sess_count==0 && dlg->tsx_count==0) {
-	pj_mutex_unlock(dlg->mutex);
-	pj_mutex_lock(dlg->mutex);
+	pj_mutex_unlock(dlg->mutex_);
+	pj_mutex_lock(dlg->mutex_);
 	unregister_and_destroy_dialog(dlg);
     } else {
-	pj_mutex_unlock(dlg->mutex);
+	pj_mutex_unlock(dlg->mutex_);
     }
+
+    pjsip_ua_unlock_dlg_table();
 }
 
 
@@ -724,7 +733,8 @@ PJ_DEF(pj_status_t) pjsip_dlg_dec_session( pjsip_dialog *dlg,
     PJ_LOG(5,(dlg->obj_name, "Session count dec to %d by %.*s",
 	      dlg->sess_count-1, (int)mod->name.slen, mod->name.ptr));
 
-    pj_mutex_lock(dlg->mutex);
+    pjsip_dlg_inc_lock(dlg);
+    --dlg->sess_count;
     pjsip_dlg_dec_lock(dlg);
 
     return PJ_SUCCESS;
@@ -748,7 +758,7 @@ PJ_DEF(pj_status_t) pjsip_dlg_add_usage( pjsip_dialog *dlg,
 	      "Module %.*s added as dialog usage, data=%p",
 	      (int)mod->name.slen, mod->name.ptr, mod_data));
 
-    pj_mutex_lock(dlg->mutex);
+    pjsip_dlg_inc_lock(dlg);
 
     /* Usages are sorted on priority, lowest number first.
      * Find position to put the new module, also makes sure that
@@ -757,7 +767,7 @@ PJ_DEF(pj_status_t) pjsip_dlg_add_usage( pjsip_dialog *dlg,
     for (index=0; index<dlg->usage_cnt; ++index) {
 	if (dlg->usage[index] == mod) {
 	    pj_assert(!"This module is already registered");
-	    pj_mutex_unlock(dlg->mutex);
+	    pjsip_dlg_dec_lock(dlg);
 	    return PJSIP_ETYPEEXISTS;
 	}
 
@@ -777,7 +787,7 @@ PJ_DEF(pj_status_t) pjsip_dlg_add_usage( pjsip_dialog *dlg,
     /* Increment count. */
     ++dlg->usage_cnt;
 
-    pj_mutex_unlock(dlg->mutex);
+    pjsip_dlg_dec_lock(dlg);
 
     return PJ_SUCCESS;
 }
@@ -896,7 +906,7 @@ PJ_DEF(pj_status_t) pjsip_dlg_create_request( pjsip_dialog *dlg,
     PJ_ASSERT_RETURN(dlg && method && p_tdata, PJ_EINVAL);
 
     /* Lock dialog. */
-    pj_mutex_lock(dlg->mutex);
+    pjsip_dlg_inc_lock(dlg);
 
     /* Use outgoing CSeq and increment it by one. */
     if (cseq <= 0)
@@ -921,7 +931,7 @@ PJ_DEF(pj_status_t) pjsip_dlg_create_request( pjsip_dialog *dlg,
     }
 
     /* Unlock dialog. */
-    pj_mutex_unlock(dlg->mutex);
+    pjsip_dlg_dec_lock(dlg);
 
     *p_tdata = tdata;
 
@@ -1113,12 +1123,12 @@ PJ_DEF(pj_status_t) pjsip_dlg_create_response(	pjsip_dialog *dlg,
 	return status;
 
     /* Lock the dialog. */
-    pj_mutex_lock(dlg->mutex);
+    pjsip_dlg_inc_lock(dlg);
 
     dlg_beautify_response(dlg, st_code, tdata);
 
     /* Unlock the dialog. */
-    pj_mutex_unlock(dlg->mutex);
+    pjsip_dlg_dec_lock(dlg);
 
     /* Done. */
     *p_tdata = tdata;
diff --git a/pjsip/src/pjsip/sip_ua_layer.c b/pjsip/src/pjsip/sip_ua_layer.c
index 2010dace..5e63e71c 100644
--- a/pjsip/src/pjsip/sip_ua_layer.c
+++ b/pjsip/src/pjsip/sip_ua_layer.c
@@ -209,6 +209,30 @@ PJ_DEF(pjsip_user_agent*) pjsip_ua_instance(void)
 }
 
 
+/**
+ * Lock the dialog's hash table. This function is normally called by
+ * dialog code only.
+ *
+ * @return		PJ_SUCCESS on success or the appropriate error code.
+ */
+PJ_DEF(pj_status_t) pjsip_ua_lock_dlg_table(void)
+{
+    return pj_mutex_lock(mod_ua.mutex);
+}
+
+
+/**
+ * Unlock the dialog's hash table. This function is normally called by
+ * dialog code only.
+ *
+ * @return		PJ_SUCCESS on success or the appropriate error code.
+ */
+PJ_DEF(pj_status_t) pjsip_ua_unlock_dlg_table(void)
+{
+    return pj_mutex_unlock(mod_ua.mutex);
+}
+
+
 /*
  * Get the endpoint where this UA is currently registered.
  */
@@ -561,12 +585,18 @@ static pj_bool_t mod_ua_on_rx_request(pjsip_rx_data *rdata)
     /* Mark the dialog id of the request. */
     rdata->endpt_info.mod_data[mod_ua.mod.id] = dlg;
 
+    /* Lock the dialog */
+    pjsip_dlg_inc_lock(dlg);
+
     /* Done processing in the UA */
     pj_mutex_unlock(mod_ua.mutex);
 
     /* Pass to dialog. */
     pjsip_dlg_on_rx_request(dlg, rdata);
 
+    /* Unlock the dialog. This may destroy the dialog */
+    pjsip_dlg_dec_lock(dlg);
+
     /* Report as handled. */
     return PJ_TRUE;
 }
@@ -590,13 +620,19 @@ static pj_bool_t mod_ua_on_rx_response(pjsip_rx_data *rdata)
      * the response is a forked response.
      */
 
+    /* Lock user agent dlg table before we're doing anything. */
+    pj_mutex_lock(mod_ua.mutex);
+
     /* Check if transaction is present. */
     tsx = pjsip_rdata_get_tsx(rdata);
     if (tsx) {
 	/* Check if dialog is present in the transaction. */
 	dlg = pjsip_tsx_get_dlg(tsx);
-	if (!dlg)
+	if (!dlg) {
+	    /* Unlock dialog hash table. */
+	    pj_mutex_unlock(mod_ua.mutex);
 	    return PJ_FALSE;
+	}
 
 	/* Get the dialog set. */
 	dlg_set = dlg->dlg_set;
@@ -620,11 +656,11 @@ static pj_bool_t mod_ua_on_rx_response(pjsip_rx_data *rdata)
 	     * This must be some stateless response sent by other modules,
 	     * or a very late response.
 	     */
+	    /* Unlock dialog hash table. */
+	    pj_mutex_unlock(mod_ua.mutex);
 	    return PJ_FALSE;
 	}
 
-	/* Lock user agent before accessing the hash table. */
-	pj_mutex_lock(mod_ua.mutex);
 
 	/* Get the dialog set. */
 	dlg_set = pj_hash_get(mod_ua.dlg_table, 
@@ -632,10 +668,10 @@ static pj_bool_t mod_ua_on_rx_response(pjsip_rx_data *rdata)
 			      rdata->msg_info.from->tag.slen,
 			      NULL);
 
-	/* Done with accessing the hash table. */
-	pj_mutex_unlock(mod_ua.mutex);
-
 	if (!dlg_set) {
+	    /* Unlock dialog hash table. */
+	    pj_mutex_unlock(mod_ua.mutex);
+
 	    /* Strayed 2xx response!! */
 	    PJ_LOG(4,(THIS_FILE, 
 		      "Received strayed 2xx response (no dialog is found)"
@@ -666,9 +702,6 @@ static pj_bool_t mod_ua_on_rx_response(pjsip_rx_data *rdata)
 	int st_code = rdata->msg_info.msg->line.status.code;
 	pj_str_t *to_tag = &rdata->msg_info.to->tag;
 
-	/* Must hold UA mutex before accessing dialog set. */
-	pj_mutex_lock(mod_ua.mutex);
-
 	dlg = dlg_set->dlg_list.next;
 
 	while (dlg != (pjsip_dialog*)&dlg_set->dlg_list) {
@@ -733,9 +766,6 @@ static pj_bool_t mod_ua_on_rx_response(pjsip_rx_data *rdata)
 
 	}
 
-	/* Done with the dialog set. */
-	pj_mutex_unlock(mod_ua.mutex);
-
     } else {
 	/* Either this is a non-INVITE response, or subsequent INVITE
 	 * within dialog. The dialog should have been identified when
@@ -751,9 +781,18 @@ static pj_bool_t mod_ua_on_rx_response(pjsip_rx_data *rdata)
     /* Put the dialog instance in the rdata. */
     rdata->endpt_info.mod_data[mod_ua.mod.id] = dlg;
 
+    /* Acquire lock to the dialog. */
+    pjsip_dlg_inc_lock(dlg);
+
+    /* Unlock dialog hash table. */
+    pj_mutex_unlock(mod_ua.mutex);
+
     /* Pass the response to the dialog. */
     pjsip_dlg_on_rx_response(dlg, rdata);
 
+    /* Unlock the dialog. This may destroy the dialog. */
+    pjsip_dlg_dec_lock(dlg);
+
     /* Done. */
     return PJ_TRUE;
 }