[sr-dev] presence module (API)

Richard Good richard.good at smilecoms.com
Fri Jan 24 16:54:43 CET 2014


Hi

The attached patch fixes our problem.

As you suggested delete_shtable takes a subs_t pointer as an argument and
compares the dialog's full tag set before deleting.

If everyone is OK with this patch, on Monday I will look at a patch for the
RLS module - it looks a little bit tricky as I think RLS exports the
pres_delete_shtable through its own API as rls_delete_shtable, though I'm
not 100% sure on this.

Regards
Richard.



On 23 January 2014 17:24, Andrew Mortensen <admorten at isc.upenn.edu> wrote:

>
> On Jan 23, 2014, at 10:06 AM, Jason Penton <jason.penton at gmail.com> wrote:
>
> > Hey Andrew,
> >
> > Shot for the prompt response. I am happy to make these changes if we all
> agree... I was concerned like you of the impact on the consumers, but as
> you say just RLS.
> >
> > Can I go ahead?
>
> You have my support. Post the patch here for review.
>
> andrew
>
>
>
> >
> > On Thu, Jan 23, 2014 at 5:00 PM, Andrew Mortensen <
> admorten at isc.upenn.edu> wrote:
> >
> > On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton at gmail.com>
> wrote:
> >
> > > Hey all,
> > >
> > > I was taking a look at the presence module API code, specfically the
> API code for the hash table. I see that when we delete a subscription
> according to hash.c:
> > > [snip]
> > > Why are we only searching on to-tag? What if there is a collision on
> the hash AND the to-tag is the same for 2+ different subscriptions. This is
> even more likely of happening considering that the hash calculation is
> based only on the callid and to-tag...
> >
> > At least nominally the tag should be globally unique, per RFC3261:
> >
> > 19.3 ... When a tag is generated by a UA for insertion into a request or
> >    response, it MUST be globally unique and cryptographically random
> >    with at least 32 bits of randomness.
> >
> > But I agree that poorly behaved UAs could cause problems here. (We
> actually ran into duplicate to-tags recently with vendor handsets booted
> simultaneously.)
> >
> > It looks to me like we should be calling search_shtable, which compares
> the dialog's full tag set. The problem is that unlike other similar calls,
> delete_shtable doesn't take a subs_t pointer as an argument, so the only
> point of reference  is the to-tag. Fortunately it looks like there's only
> one place delete_shtable's called in the presence module, so it shouldn't
> be much work to start using search_shtable to find the subscription to
> delete.
> >
> > To complicate things further, though, delete_shtable is exported as part
> of the presence API, so we'd need to update all API consumers as well. That
> doesn't seem unreasonable, as a quick git grep indicates the only current
> user of the presence API is the rls module.
> >
> > andrew
> > _______________________________________________
> > sr-dev mailing list
> > sr-dev at lists.sip-router.org
> > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
> >
> > _______________________________________________
> > sr-dev mailing list
> > sr-dev at lists.sip-router.org
> > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>
>
> _______________________________________________
> sr-dev mailing list
> sr-dev at lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>

This email is subject to the disclaimer of Smile Communications at http://www.smilecoms.com/disclaimer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20140124/5e61a2e2/attachment.html>
-------------- next part --------------
diff --git a/modules/presence/hash.c b/modules/presence/hash.c
index de7f08f..026fe1b 100644
--- a/modules/presence/hash.c
+++ b/modules/presence/hash.c
@@ -275,7 +275,7 @@ int insert_shtable(shtable_t htable,unsigned int hash_code, subs_t* subs)
 	return 0;
 }
 
-int delete_shtable(shtable_t htable,unsigned int hash_code,str to_tag)
+int delete_shtable(shtable_t htable,unsigned int hash_code,subs_t* subs)
 {
 	subs_t* s= NULL, *ps= NULL;
 	int found= -1;
@@ -287,8 +287,12 @@ int delete_shtable(shtable_t htable,unsigned int hash_code,str to_tag)
 		
 	while(s)
 	{
-		if(s->to_tag.len== to_tag.len &&
-				strncmp(s->to_tag.s, to_tag.s, to_tag.len)== 0)
+		if(s->callid.len==subs->callid.len &&
+				strncmp(s->callid.s, subs->callid.s, subs->callid.len)==0 &&
+			s->to_tag.len== subs->to_tag.len &&
+				strncmp(s->to_tag.s, subs->to_tag.s, subs->to_tag.len)==0 &&
+			s->from_tag.len== subs->from_tag.len &&
+				strncmp(s->from_tag.s, subs->from_tag.s, subs->from_tag.len)== 0)
 		{
 			found= s->local_cseq +1;
 			ps->next= s->next;
diff --git a/modules/presence/hash.h b/modules/presence/hash.h
index 7c7010d..4bcbefb 100644
--- a/modules/presence/hash.h
+++ b/modules/presence/hash.h
@@ -78,7 +78,7 @@ struct subscription* search_shtable(shtable_t htable, str callid,str to_tag,str
 
 int insert_shtable(shtable_t htable, unsigned int hash_code, struct subscription* subs);
 
-int delete_shtable(shtable_t htable, unsigned int hash_code, str to_tag);
+int delete_shtable(shtable_t htable, unsigned int hash_code, struct subscription* subs);
 
 int update_shtable(shtable_t htable, unsigned int hash_code, struct subscription* subs,
 		int type);
@@ -99,7 +99,7 @@ typedef int (*insert_shtable_t)(shtable_t htable, unsigned int hash_code,
 		struct subscription* subs);
 
 typedef int (*delete_shtable_t)(shtable_t htable, unsigned int hash_code,
-		str to_tag);
+		struct subscription* subs);
 
 typedef int (*update_shtable_t)(shtable_t htable, unsigned int hash_code,
 		struct subscription* subs, int type);
diff --git a/modules/presence/subscribe.c b/modules/presence/subscribe.c
index aff55f3..0a18eff 100644
--- a/modules/presence/subscribe.c
+++ b/modules/presence/subscribe.c
@@ -456,11 +456,19 @@ int update_subs_db(subs_t* subs, int type)
 void delete_subs(str* pres_uri, str* ev_name, str* to_tag,
 		str* from_tag, str* callid)
 {
+	subs_t subs;
+	memset(&subs, 0, sizeof(subs_t));
+	
+	subs.pres_uri = *pres_uri;
+	subs.from_tag = *from_tag;
+	subs.to_tag = *to_tag;
+	subs.callid = *callid;
+	
 	/* delete record from hash table also if not in dbonly mode */
 	if(subs_dbmode != DB_ONLY)
 	{
 		unsigned int hash_code= core_hash(pres_uri, ev_name, shtable_size);
-		if(delete_shtable(subs_htable, hash_code, *to_tag) < 0)
+		if(delete_shtable(subs_htable, hash_code, &subs) < 0)
 			LM_ERR("Failed to delete subscription from memory\n");
 	}
 


More information about the sr-dev mailing list