miconda requested changes on this pull request.
>
- LM_INFO("adding destination: %.*s\n", uri->len, uri->s);
+ LM_DBG("adding destination: %.*s\n", uri->len, uri->s);
+
+ if(ka_find_destination(uri , owner , &dest , &dest)){
+ LM_INFO("uri [%.*s] already in stack --ignoring \r\n",uri->len,
uri->s);
+ dest->counter=0;
The change of the `dest->counter` is done out of the mutex zone and this
operation can be done on an invalid structure if the `dest` was removed
meanwhile.
> +* @abstract deletes given sip uri in allocated destination stack as named
> ka_alloc_destinations_list
+*
+* @param msg sip message
+* @param uri given uri
+* @param owner given owner name, not using now
+* *
+* @result 1 successful , -1 fail
+*/
+int ka_del_destination(str *uri, str *owner){
+
+ ka_dest_t *target=0,*head=0;
+
+ if(!ka_find_destination(uri,owner,&target,&head)){
+ LM_ERR("Couldnt find destination \r\n");
+ return -1;
+ }
Similar issues, `target` and `head` are retrieved from `ka_find_destination()`,
followed by code out of the mutex zone and later used directly, but at that
time any of them can be already deleted. So removal has to be done in the same
mutex zone as it is searched.
Actually I do not see any good use for `ka_find_destination()` returning
`target` and `head`. The function can be useful to know if the address exists,
but using `target` and `head` after this function expose to segfault. Either
you keep the lock set in the function at the return time and unlock later, out
of the function, or just make it to return true/false on finding the
destination, without returning `target` and `head`.
>
extern struct tm_binds tmb;
int ka_ping_interval = 30;
ka_destinations_list_t *ka_destinations_list = NULL;
+str ka_ping_from = str_init("sip:keepal...@kamailio.org");
+int counter_del = 5;
I suggest adding `ka_` prefix to global variables that are not static and
exposed in other files via `extern`.
>
static cmd_export_t cmds[] = {
{"is_alive", (cmd_function)w_cmd_is_alive, 1,
fixup_spve_null, 0, ANY_ROUTE},
// internal API
+ {"add_destination", (cmd_function)w_add_destination, 2,
+ fixup_add_destination, 0,
REQUEST_ROUTE|BRANCH_ROUTE|ONREPLY_ROUTE},
+ {"del_destination", (cmd_function)w_del_destination, 2,
+ fixup_add_destination, 0, ANY_ROUTE},
As more functions are exported from this module, I think is better to also
prefix the exported name with `ka_`, like most modules use a common prefix for
their functions. Probably we should also add an alias named `ka_is_alive()`
for `is_alive()`.
> @@ -126,6 +138,8 @@ static int mod_init(void)
*/
static void mod_destroy(void)
{
+ lock_release(ka_destinations_list->lock);
+ lock_dealloc(ka_destinations_list->lock);
}
This two have to be enclosed in `if(ka_destinations_list) { ... }`, the
mod_destroy() is executed even when mod_init() is not finished, in that case
ka_destinations_list is NULL, crashing kamailio at shutdown.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2133#pullrequestreview-325064799_______________________________________________
Kamailio (SER) - Development Mailing List
sr-dev@lists.kamailio.org
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
