Re: [sr-dev] git:master: modules_k/presence: Fixed DB insert race hazard on the watchers table

Hi Peter,

You do not create transactions for Subscribe requests before calling handle_subscribe() function to catch retransmissions? I wonder how can you get to the situation of another process inserting a record (with exactly the same callid, to tag & from tag) between the query and the insert.

Regards, Anca

On 03/29/2012 06:13 PM, Peter Dunkley wrote:

Module: sip-router Branch: master Commit: 5a89af6ea8b83ecc781d3f169023fde8388a2da6 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=5a89af6e...

Author: Peter Dunkleypeter.dunkley@crocodile-rcs.com Committer: Peter Dunkleypeter.dunkley@crocodile-rcs.com Date: Thu Mar 29 16:02:13 2012 +0100

modules_k/presence: Fixed DB insert race hazard on the watchers table

• The time between the query on the watchers table (which determines there is no matching entry) and the insert is substantial. During a soak I observed inserts failing because rows had been inserted in this time window.
• The fix is to use replace (where available) instead of insert.
• Also fixed a small whitespace issue I noticed, and added an extra use_table call (as I think there was one missing).

---

modules_k/presence/presence.c | 2 +- modules_k/presence/subscribe.c | 32 ++++++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/modules_k/presence/presence.c b/modules_k/presence/presence.c index dd415f7..f6f9152 100644 --- a/modules_k/presence/presence.c +++ b/modules_k/presence/presence.c @@ -833,7 +833,7 @@ int update_watchers_status(str pres_uri, pres_ev_t* ev, str* rules_doc) }ws_t; ws_t* ws_list= NULL;

• LM_DBG("start\n");

• LM_DBG("start\n");

  if(ev->content_type.s== NULL) {

diff --git a/modules_k/presence/subscribe.c b/modules_k/presence/subscribe.c index 0c914c7..0a9db83 100644 --- a/modules_k/presence/subscribe.c +++ b/modules_k/presence/subscribe.c @@ -315,6 +315,12 @@ int insert_subs_db(subs_t* s, int type) query_vals[reason_col].val.str_val= s->reason; query_vals[socket_info_col].val.str_val= s->sockinfo_str;

• if (pa_dbf.use_table(pa_db,&active_watchers_table)< 0)
• {

• LM_ERR("in use table sql operation\n");	
  

• return -1;
  

• }
• LM_DBG("inserting subscription in active_watchers table\n"); if(pa_dbf.insert(pa_db, query_cols, query_vals, n_query_cols)< 0) {

@@ -2310,10 +2316,28 @@ int insert_db_subs_auth(subs_t* subs) return -1; }

• if(pa_dbf.insert(pa_db, db_keys, db_vals, n_query_cols )< 0)
• {

• LM_ERR("in sql insert\n");
  

• return -1;
  

• if (pa_dbf.replace != NULL)

• {

• if(pa_dbf.replace(pa_db, db_keys, db_vals, n_query_cols,
  

• 			2, 0)<  0)
  

• {
  

• 	LM_ERR("in sql replace\n");
  

• 	return -1;
  

• }
  

• }

• else

• {

• /* If you use insert() instead of replace() be prepared for some
  

•    DB error messages.  There is a lot of time between the
  

•    query() that indicated there was no matching entry in the DB
  

•    and this insert(), so on a multi-user system it is entirely
  

•    possible (even likely) that a record will be added after the
  

•    query() but before this insert(). */
  

• if(pa_dbf.insert(pa_db, db_keys, db_vals, n_query_cols )<  0)
  

• {	
  

• 	LM_ERR("in sql insert\n");
  

• 	return -1;
  

• }
  

  }

  return 0;

---

sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev