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:
int delete_shtable(shtable_t htable,unsigned int hash_code,str to_tag) { subs_t* s= NULL, *ps= NULL; int found= -1;
lock_get(&htable[hash_code].lock); ps= htable[hash_code].entries; s= ps->next; while(s) { if(s->to_tag.len== to_tag.len && strncmp(s->to_tag.s, to_tag.s, to_tag.len)== 0) { found= s->local_cseq +1; ps->next= s->next; if(s->contact.s!=NULL) shm_free(s->contact.s); shm_free(s); break; } ps= s; s= s->next; } lock_release(&htable[hash_code].lock); return found; }
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...
any comments? Am I missing something here?
Cheers jason
On Jan 23, 2014, at 9:19 AM, Jason Penton jason.penton@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
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?
Cheers Jason
On Thu, Jan 23, 2014 at 5:00 PM, Andrew Mortensen admorten@isc.upenn.eduwrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton jason.penton@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
On Jan 23, 2014, at 10:06 AM, Jason Penton jason.penton@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@isc.upenn.edu wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton jason.penton@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
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@isc.upenn.edu wrote:
On Jan 23, 2014, at 10:06 AM, Jason Penton jason.penton@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@isc.upenn.edu> wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton jason.penton@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@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
On Jan 24, 2014, at 10:54 AM, Richard Good richard.good@smilecoms.com wrote:
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.
Looks good to me. Thanks.
andrew
On 23 January 2014 17:24, Andrew Mortensen admorten@isc.upenn.edu wrote:
On Jan 23, 2014, at 10:06 AM, Jason Penton jason.penton@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@isc.upenn.edu wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton jason.penton@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@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
<presence_mod.diff>_______________________________________________ sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
Hi
The attached patch fixes RLS module use of pres_delete_shtable - its fairly straight forward as pres_delete_shtable is only used once and not exported as I thought it might be.
If everyone is in agreement I will commit this change and the presence module change to the master branch.
Regards Richard.
24 January 2014 18:04, Andrew Mortensen admorten@isc.upenn.edu wrote:
On Jan 24, 2014, at 10:54 AM, Richard Good richard.good@smilecoms.com wrote:
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.
Looks good to me. Thanks.
andrew
On 23 January 2014 17:24, Andrew Mortensen admorten@isc.upenn.edu
wrote:
On Jan 23, 2014, at 10:06 AM, Jason Penton jason.penton@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@isc.upenn.edu> wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton jason.penton@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@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
<presence_mod.diff>_______________________________________________ sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@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
On Jan 28, 2014, at 8:36 AM, Richard Good richard.good@smilecoms.com wrote:
Hi
The attached patch fixes RLS module use of pres_delete_shtable - its fairly straight forward as pres_delete_shtable is only used once and not exported as I thought it might be.
If everyone is in agreement I will commit this change and the presence module change to the master branch.
I'm in favor.
andrew
24 January 2014 18:04, Andrew Mortensen admorten@isc.upenn.edu wrote:
On Jan 24, 2014, at 10:54 AM, Richard Good richard.good@smilecoms.com wrote:
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.
Looks good to me. Thanks.
andrew
On 23 January 2014 17:24, Andrew Mortensen admorten@isc.upenn.edu wrote:
On Jan 23, 2014, at 10:06 AM, Jason Penton jason.penton@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@isc.upenn.edu wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton jason.penton@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@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
<presence_mod.diff>_______________________________________________ sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@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
<rls_mod.diff>_______________________________________________ sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
Am 23.01.2014 15:19, schrieb Jason Penton:
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...
any comments? Am I missing something here?
Maybe it depends on the totag generation. If this code handles incoming SUBSCRIBEs and the totag is generated by Kamailio in a unique way, then it should be safe. If to tags are not generated by Kamailio (and maybe in a bad way), then there may be collisions, but then may be collisions also on callid and from-tag, if the clients generate them in a broken way.
regards Klaus
Hi
The problem we are having is with processing subscription to reg event at the S-CSCF - we process SUBSCRIBE messages in ims_registrar_scscf module and we are seeing to_tag collisions (though never to_tag/from_tag/call_id collisions) - we process the initial SUBSCRIBE that has no to_tag - we calculate the to_tag with:
tmb.t_get_reply_totag(msg, &ttag);
and store this as part of the subscription.
It seems that this can return a to tag that is the same even if the call_id/from_tag of the initial SUBSCRIBE are different.
I'll investigate further today.
Regards Richard.
On 24 January 2014 18:18, Klaus Darilion klaus.mailinglists@pernau.atwrote:
Am 23.01.2014 15:19, schrieb Jason Penton:
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...
any comments? Am I missing something here?
Maybe it depends on the totag generation. If this code handles incoming SUBSCRIBEs and the totag is generated by Kamailio in a unique way, then it should be safe. If to tags are not generated by Kamailio (and maybe in a bad way), then there may be collisions, but then may be collisions also on callid and from-tag, if the clients generate them in a broken way.
regards Klaus
sr-dev mailing list sr-dev@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
-
Andrew Mortensen -
Jason Penton -
Klaus Darilion -
Richard Good