@miconda requested changes on this pull request.

In src/modules/keepalive/keepalive_api.c:

>  
-	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.

In src/modules/keepalive/keepalive_api.c:

> +* @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.

In src/modules/keepalive/keepalive_mod.c:

>  
 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:keepalive@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.

In src/modules/keepalive/keepalive_mod.c:

>  
 
 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().

In src/modules/keepalive/keepalive_mod.c:

> @@ -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, view it on GitHub, or unsubscribe.