Re: [osip-dev] implicit subscription

[Aymeric Moizard]

inline!

2017-06-26 23:28 GMT+02:00 FEICHTER Christoph <address@hidden>:

hi,

 

sorry – my fault.

everything fine !

 

your patch v4 partly works.

Nevertheless I have some improvements / hardening issues; attached as v5.

 

here are my explanations:

 

-          jcallback.c, function cb_rcv2xx
for performance reasons I put the handling of NOTIFY and REFER in an else-if cascade

ok! 

-          jcallback.c, function cb_rcv2xx
eXcall_api.c, function eXosip_call_send_answer
I re-worked the parsing a bit:

o   searching for the expires-parameter did not work, due to the different length of “active” and “pending”

o   you forgot to take into account the “;” between “active”/”pending” and the “expires” parameter

o   the length of “expires” is 7, not 6 chars
à I added const char’s for the strings to search for; so we can use strlen to get the right length

Please check again my version!!!

///CHECK 6 char for "active" or 7 char for "pending"

        if (0 == osip_strncasecmp (sub_state->hvalue, "active", 6)

          ||0 == osip_strncasecmp (sub_state->hvalue, "pending", 7)) {

//In the next strstr call, we are searching STARTING FROM HVALUE+6 for "expires" occurence.

  -> this means we start on the ";expires=180" if "active" was found.

  -> this means we start of the g;expires=180" if "pending" was found.

            const char *tmp = strstr(sub_state->hvalue+6, "expires");

-> in both case, the first occurence is found and tmp pointer is on first letter of "expires"

            const char *ss_expires = NULL;

            if (tmp!=NULL) {

//Here, we search for "=" starting 7 letter after "expires", so in both case, we are on "=" and ss_expires

//ends up being at tmp+7

              ss_expires = strchr(tmp+7, '=');

-> conclusion, my code handles correctly both "active" and "pending".

-> my code handle correctly any additionnal space before or after ";" and any other before or after "=".

-> this is why I do prefer my code! see below for more info on SEMI, EQUAL...

o   I did not like the double “if (ss_expires!=NULL)”

Accepted! ;)

 

o   I use “;expires=” for searching for the expires-parameter

My code modification were made to accept any space (and LWS) inside the string! As I pointed above. 

Looking at rfc3265, here is the definition of Subscription-State

Subscription-State   = "Subscription-State" HCOLON substate-value
                          *( SEMI subexp-params )
   substate-value       = "active" / "pending" / "terminated"
                          / extension-substate

subexp-params        =   ("reason" EQUAL event-reason-value)
                          / ("expires" EQUAL delta-seconds)
                          / ("retry-after" EQUAL delta-seconds)
                          / generic-param

I have no much doubt that SEMI and EQUAL can contains space and other LWS...

o   check “sub_state->hvalue != NULL” was missing
otherwise crash in case of empty “Subscription-State” header

Good point! 

o   check “refer_sub->hvalue != NULL” was missing
otherwise crash in case of empty “Refer-Sub” header

Good point! 

-          Although the expires parameter should be present in the Subscription-State header of the NOTIFY
I added to use the default (excontext->implicit_subscription_expires) in this case.

Right. My patch didn't fallback to default. I have modified my version to include that

jd->implicit_subscription_expire_time = now + excontext->implicit_subscription_expires;

I also moved the log to show when expires is not present. Also, my code also use default

value when expires is not showing a good value... (negative one or above 600.)

Additionnal change:

At the end of "eXosip_call_terminate_with_reason", your code do not include my change:

when a subscription exist, it will remove the dialog when sending a BYE.

Instead my code avoid to delete the dialog if implicit_subscription_expire_time has a value.

This is required to send NOTIFY after sending a BYE.

Other minor udp.c change were accepted!

Here is the newer v6 patch...

https://sip.antisip.com/git_diff_implicit-subscription_v6.patch

I should commit it by tomorrow. However, if anything is wrong let me know!

Regards

Aymeric

 

br,

Christoph