[sr-dev] git:3.2: modules_k/pua: Fixed race hazard relating to RLS back-end SUBSCRIBEs

[Peter Dunkley]

Module: sip-router
Branch: 3.2
Commit: 047f66369f0c3eaa75592942f56565eb0f39f848
URL:    http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=047f66369f0c3eaa75592942f56565eb0f39f848

Author: pd <peter.dunkley at crocodile-rcs.com>
Committer: pd <peter.dunkley at crocodile-rcs.com>
Date:   Fri Jan 27 15:34:16 2012 +0000

modules_k/pua: Fixed race hazard relating to RLS back-end SUBSCRIBEs

- This resulted in the "no presence dialog record for non-TERMINATED state..."
  error message coming out of RLS a lot.
- On the sending side you have can have two dialogs (one temporary and one full)
  stored for a short period of time.  This is because the full dialog is written
  before the temporary one is deleted.  This can make the lookup when a back-end
  NOTIFY is received fail because only one record is expected.  This is now
  fixed - instead of inserting and then deleting we do a swap (while the hash
  table is locked).
- Based on... (cherry picked from commit e627bc31776b521a1078b2a004e8ed179521cae2)

---

 modules_k/pua/hash.c           |   48 ++++++++++++++++++++++++++-------------
 modules_k/pua/hash.h           |    1 +
 modules_k/pua/send_subscribe.c |   16 ++++++++-----
 3 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/modules_k/pua/hash.c b/modules_k/pua/hash.c
index 805be74..1b6784c 100644
--- a/modules_k/pua/hash.c
+++ b/modules_k/pua/hash.c
@@ -215,33 +215,28 @@ void update_htable(ua_pres_t* p, time_t desired_expires, int expires,
 	}
 }
 /* insert in front; so when searching the most recent result is returned*/
-void insert_htable(ua_pres_t* presentity)
+void _insert_htable(ua_pres_t* presentity, unsigned int hash_code)
 {
 	ua_pres_t* p= NULL;
-	unsigned int hash_code;
-
-	hash_code= core_hash(presentity->pres_uri,presentity->watcher_uri, 
-			HASH_SIZE);
-	
-	lock_get(&HashT->p_records[hash_code].lock);
 
-/*	
- *	useless since always checking before calling insert
-	if(get_dialog(presentity, hash_code)!= NULL )
-	{
-		LM_DBG("Dialog already found- do not insert\n");
-		return; 
-	}
-*/	
 	p= HashT->p_records[hash_code].entity;
 
 	presentity->db_flag= INSERTDB_FLAG;
 	presentity->next= p->next;
 	
 	p->next= presentity;
+}
 
-	lock_release(&HashT->p_records[hash_code].lock);
+void insert_htable(ua_pres_t* presentity)
+{
+	unsigned int hash_code;
+
+	hash_code= core_hash(presentity->pres_uri,presentity->watcher_uri, HASH_SIZE);
+	lock_get(&HashT->p_records[hash_code].lock);
+
+	_insert_htable(presentity, hash_code);
 
+	lock_release(&HashT->p_records[hash_code].lock);
 }
 
 /* This function used to perform a search to find the hash table
@@ -302,6 +297,27 @@ void destroy_htable(void)
   return;
 }
 
+int convert_temporary_dialog(ua_pres_t *dialog)
+{
+	ua_pres_t *temp_dialog;
+	unsigned int hash_code;
+
+	hash_code= core_hash(dialog->pres_uri,dialog->watcher_uri, HASH_SIZE); 
+	lock_get(&HashT->p_records[hash_code].lock);
+
+	temp_dialog = get_temporary_dialog(dialog, hash_code);
+	if (temp_dialog)
+		delete_htable(temp_dialog, hash_code);
+	else
+		return -1;
+
+	_insert_htable(dialog, hash_code);
+
+	lock_release(&HashT->p_records[hash_code].lock);
+
+	return 1;
+}
+
 /* must lock the record line before calling this function*/
 ua_pres_t* get_dialog(ua_pres_t* dialog, unsigned int hash_code)
 {
diff --git a/modules_k/pua/hash.h b/modules_k/pua/hash.h
index 141296f..c2204c4 100644
--- a/modules_k/pua/hash.h
+++ b/modules_k/pua/hash.h
@@ -126,6 +126,7 @@ int is_dialog(ua_pres_t* dialog);
 
 ua_pres_t* get_dialog(ua_pres_t* dialog, unsigned int hash_code);
 ua_pres_t* get_temporary_dialog(ua_pres_t* dialog, unsigned int hash_code);
+int convert_temporary_dialog(ua_pres_t *dialog);
 
 int get_record_id(ua_pres_t* dialog, str** rec_id);
 typedef int (*get_record_id_t)(ua_pres_t* dialog, str** rec_id);
diff --git a/modules_k/pua/send_subscribe.c b/modules_k/pua/send_subscribe.c
index 08e3d3d..c4caad1 100644
--- a/modules_k/pua/send_subscribe.c
+++ b/modules_k/pua/send_subscribe.c
@@ -612,7 +612,11 @@ void subs_cback_func(struct cell *t, int cb_type, struct tmcb_params *ps)
 	LM_DBG("record for subscribe from %.*s to %.*s inserted in datatbase\n",
 			presentity->watcher_uri->len, presentity->watcher_uri->s,
 			presentity->pres_uri->len, presentity->pres_uri->s);
-	insert_htable(presentity);
+	if (convert_temporary_dialog(presentity) < 0)
+	{
+		LM_ERR("Could not convert temporary dialog into a dialog\n");
+		goto error;
+	}
 
 done:
 	if(hentity->ua_flag == REQ_OTHER)
@@ -620,13 +624,13 @@ done:
 		hentity->flag= flag;
 		run_pua_callbacks( hentity, msg);
 	}
+	goto end;
+
 error:	
-	lock_get(&HashT->p_records[hash_code].lock);
-	presentity = get_temporary_dialog(hentity, hash_code);
-	if (presentity!=NULL)
-		delete_htable(presentity, hash_code);
-	lock_release(&HashT->p_records[hash_code].lock);
+	if (presentity->remote_contact.s) shm_free(presentity->remote_contact.s);
+	if (presentity) shm_free(presentity);
 
+end:
 	if(hentity)
 	{	
 		shm_free(hentity);