[Kamailio-Devel] [ openser-Bugs-2092890 ] nathelper concurrency issue

[SourceForge.net]

Bugs item #2092890, was opened at 2008-09-04 11:55
Message generated for change (Comment added) made by tramjoe
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=743020&aid=2092890&group_id=139143

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: modules
Group: ver 1.4.x
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Nobody/Anonymous (nobody)
Assigned to: Henning Westerholt (henningw)
Summary: nathelper concurrency issue

Initial Comment:
nathelper.c:child_init

the following piece of code is unsafe. pnode->rn_address is written to without proper precaution.

the code causes SIGCHLD when:
- process A removes the colon
- process B detects there is no colon (because A removed it)
- process A restores the colon
- process B invokes getaddrinfo on the restored string, which is an invalid hostname

i suggest creating a temp variable to hold the hostname (without colon)

-- code --

old_colon = cp = strrchr(pnode->rn_address, ':');
if (cp != NULL) {
	old_colon = cp;
	*cp = '\0';
	cp++;
}
if (cp == NULL || *cp == '\0')
	cp = CPORT;

memset(&hints, 0, sizeof(hints));
hints.ai_flags = 0;
hints.ai_family = (pnode->rn_umode == 6) ? AF_INET6 : AF_INET;
hints.ai_socktype = SOCK_DGRAM;
if ((n = getaddrinfo(pnode->rn_address, cp, &hints, &res)) != 0) {
	LM_ERR("%s\n", gai_strerror(n));
	return -1;
}
if (old_colon)
	*old_colon = ':'; /* restore rn_address */

----------------------------------------------------------------------

Comment By: Jerome Martin (tramjoe)
Date: 2008-09-15 12:54

Message:
Just to note I am the author of previous comment.

jmartin at longphone.fr

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2008-09-15 12:49

Message:
Just dropping a quick note to second a backport of the #2105556 fix to 1.4
/ 1.3 branch.
with both versions I had to tweak the init script to actually check for
the number of running processes and retry the start when the number is too
low. This happens more than often, and the average retries needed here is
3. It would definately help to fix this :-)


----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2008-09-15 10:03

Message:
It should be backported, yes.

The frequency depends on the number of RTP proxies and child processes,
and I guess the number of CPU cores aswell.

At startup, OpenSER crashes ~90% of the time, with 8 proxies, 16 children
and 8 cores..

----------------------------------------------------------------------

Comment By: Henning Westerholt (henningw)
Date: 2008-09-11 15:38

Message:
Hi,

do you think the fix from #2105556 should be backported to the 1.3 and 1.4
branches? How frequent this problem happens in your installation?
 
Henning

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=743020&aid=2092890&group_id=139143