23 Jan
2014
23 Jan
'14
2:19 p.m.
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
23 Jan
23 Jan
3 p.m.
On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton(a)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
3:06 p.m.
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(a)isc.upenn.edu>wrote;wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
3:24 p.m.
On Jan 23, 2014, at 10:06 AM, Jason Penton <jason.penton(a)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(a)isc.upenn.edu> wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 
24 Jan
24 Jan
3:54 p.m.
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(a)isc.upenn.edu> wrote:
On Jan 23, 2014, at 10:06 AM, Jason Penton <jason.penton(a)gmail.com> wrote:
This email is subject to the disclaimer of Smile Communications at
http://www.smilecoms.com/disclaimer
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(a)isc.upenn.edu>
wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
4:04 p.m.
On Jan 24, 2014, at 10:54 AM, Richard Good <richard.good(a)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(a)isc.upenn.edu> wrote:
On Jan 23, 2014, at 10:06 AM, Jason Penton <jason.penton(a)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(a)isc.upenn.edu> wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton(a)gmail.com> wrote:
_______________________________________________
sr-dev mailing list
sr-dev(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
28 Jan
28 Jan
1:36 p.m.
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(a)isc.upenn.edu> wrote:
On Jan 24, 2014, at 10:54 AM, Richard Good <richard.good(a)smilecoms.com>
wrote:
admorten(a)isc.upenn.edu> wrote:
http://www.smilecoms.com/disclaimer
This email is subject to the disclaimer of Smile Communications at
http://www.smilecoms.com/disclaimer
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(a)isc.upenn.edu>
wrote:
On Jan 23, 2014, at 10:06 AM, Jason Penton <jason.penton(a)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 < >
> On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)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
<presence_mod.diff>_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
3:33 p.m.
On Jan 28, 2014, at 8:36 AM, Richard Good <richard.good(a)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(a)isc.upenn.edu> wrote:
On Jan 24, 2014, at 10:54 AM, Richard Good <richard.good(a)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(a)isc.upenn.edu> wrote:
On Jan 23, 2014, at 10:06 AM, Jason Penton <jason.penton(a)gmail.com> wrote:
_______________________________________________
sr-dev mailing list
sr-dev(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 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(a)isc.upenn.edu> wrote:
On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton(a)gmail.com> wrote:
_______________________________________________
sr-dev mailing list
sr-dev(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 
24 Jan
24 Jan
4:18 p.m.
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
27 Jan
27 Jan
6:45 a.m.
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(a)pernau.at>wrote;wrote:
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
This email is subject to the disclaimer of Smile Communications at
http://www.smilecoms.com/disclaimer
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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
3822
days inactive
3827
days old
9 comments
4 participants
participants (4)
-
Andrew Mortensen -
Jason Penton -
Klaus Darilion -
Richard Good