threads / patch / 19069

patchRe: [PATCH] daemon.c: fix segfault on OS X

Subject: Re: [PATCH] daemon.c: fix segfault on OS X

## tl;dr

5 messages between Apr 26, 2009 and Apr 27, 2009. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Benjamin Kramer· Apr 26, 2009, 20:01 UTC · lore

[PATCH] daemon.c: fix segfault on OS X

On OS X (and maybe other unices) getaddrinfo(3) returns NULL in the ai_canonname field if it is called with an IP address.

steps to reproduce: $ git daemon --export-all $ git clone git://127.0.0.1/frotz => git daemon's fork (silently) segfaults.

Remove the pointless loop while at it.
Signed-off-by: Benjamin Kramer <benny.kra@googlemail.com>
---
 daemon.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)
Show changes to daemon.c +10 −10
diff --git a/daemon.c b/daemon.c
index 13401f1..5f455ab 100644
--- a/daemon.c
+++ b/daemon.c
@@ -444,27 +444,27 @@ static void parse_extra_args(char *extra_args, int buflen)
 	if (hostname) {
 #ifndef NO_IPV6
 		struct addrinfo hints;
-		struct addrinfo *ai, *ai0;
+		struct addrinfo *ai;
 		int gai;
 		static char addrbuf[HOST_NAME_MAX + 1];
 
 		memset(&hints, 0, sizeof(hints));
 		hints.ai_flags = AI_CANONNAME;
 
-		gai = getaddrinfo(hostname, 0, &hints, &ai0);
+		gai = getaddrinfo(hostname, 0, &hints, &ai);
 		if (!gai) {
-			for (ai = ai0; ai; ai = ai->ai_next) {
-				struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
+			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
 
-				inet_ntop(AF_INET, &sin_addr->sin_addr,
-					  addrbuf, sizeof(addrbuf));
+			inet_ntop(AF_INET, &sin_addr->sin_addr,
+				  addrbuf, sizeof(addrbuf));
+			free(ip_address);
+			ip_address = xstrdup(addrbuf);
+
+			if (ai->ai_canonname) {
 				free(canon_hostname);
 				canon_hostname = xstrdup(ai->ai_canonname);
-				free(ip_address);
-				ip_address = xstrdup(addrbuf);
-				break;
 			}
-			freeaddrinfo(ai0);
+			freeaddrinfo(ai);
 		}
 #else
 		struct hostent *hent;
-- 
1.6.3.rc2.9.g76386
Junio C Hamano· Apr 27, 2009, 02:28 UTC · re: Benjamin Kramer · lore
Benjamin Kramer <benny.kra@googlemail.com> writes:
Show 9 quoted lines
> On OS X (and maybe other unices) getaddrinfo(3) returns NULL
> in the ai_canonname field if it is called with an IP address.
>
> steps to reproduce:
> $ git daemon --export-all
> $ git clone git://127.0.0.1/frotz
> => git daemon's fork (silently) segfaults.
>
> Remove the pointless loop while at it.

Hmm. This codepath comes from dd467629, both the loop and the use of getaddrinfo.

I have a mild suspicion that the loop originally was meant to notice that an element in the addrinfo linked list is unusable and skip it to find a usable one in the list or something like that, but as it stands it processes the first entry (or fails to process and segfaults for you ;-) and breaks out, which indeed is pointless.

But the part your patch touches is about supporting virtual hosting via pattern interpolation, and the daemon will still segfault even with your patch when somebody uses %CH expansion, because canon_hostname is left as NULL, won't it? I suspect in such a case it might be safer to use a copy of the ip_address or something.

Benjamin Kramer· Apr 27, 2009, 13:59 UTC · re: Junio C Hamano · lore

[PATCH v2] daemon.c: fix segfault on OS X

On OS X (and maybe other unices), getaddrinfo(3) returns NULL in the ai_canonname field if it's called with an IP address for the hostname. We'll now use the IP address for the hostname if ai_canonname was NULL, this also matches the behaviour on Linux.

steps to reproduce: $ git daemon --export-all $ git clone git://127.0.0.1/frotz => git daemon's fork (silently) segfaults.

Remove the pointless loop while at it. There is only one iteration because of the break; on the last line and there are no continues.

Signed-off-by: Benjamin Kramer <benny.kra@googlemail.com>
---
Junio C Hamano wrote:
Show 5 quoted lines
> But the part your patch touches is about supporting virtual hosting via
> pattern interpolation, and the daemon will still segfault even with your
> patch when somebody uses %CH expansion, because canon_hostname is left as
> NULL, won't it?  I suspect in such a case it might be safer to use a copy
> of the ip_address or something.
It doesn't segfault but it just assumes an empty hostname:
  $ git daemon --verbose --export-all --interpolated-path=%CH/%D
    [3251] Connection from 127.0.0.1:49423
    [3251] Extended attributes (16 bytes) exist <host=127.0.0.1>
    [3251] Request upload-pack for '/frotz'
    [3251] Interpolated dir '//frotz'

This upated patch uses a copy of ip_address. I did a quick test on my Linux box and it looks like Linux' getaddrinfo(3) always returns the IP address in ai_canonname instead of NULL when it is called with an IP address hostname.

  $ git daemon --verbose --export-all --interpolated-path=%CH/%D
    [3871] Connection from 127.0.0.1:49427
    [3871] Extended attributes (16 bytes) exist <host=127.0.0.1>
    [3871] Request upload-pack for '/frotz'
    [3871] Interpolated dir '127.0.0.1//frotz'

Btw, if I connect to an IPv6 host with interpolated-path=%IP the IP address gets converted to 0.0.0.0. Is this desired behaviour or yet another bug?

(I hope thunderbird didn't f*ck up the formatting this time)
 daemon.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)
Show changes to daemon.c +14 −14
diff --git a/daemon.c b/daemon.c
index 13401f1..daa4c8e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -444,27 +444,27 @@ static void parse_extra_args(char *extra_args, int buflen)
 	if (hostname) {
 #ifndef NO_IPV6
 		struct addrinfo hints;
-		struct addrinfo *ai, *ai0;
+		struct addrinfo *ai;
 		int gai;
 		static char addrbuf[HOST_NAME_MAX + 1];
 
 		memset(&hints, 0, sizeof(hints));
 		hints.ai_flags = AI_CANONNAME;
 
-		gai = getaddrinfo(hostname, 0, &hints, &ai0);
+		gai = getaddrinfo(hostname, 0, &hints, &ai);
 		if (!gai) {
-			for (ai = ai0; ai; ai = ai->ai_next) {
-				struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
-
-				inet_ntop(AF_INET, &sin_addr->sin_addr,
-					  addrbuf, sizeof(addrbuf));
-				free(canon_hostname);
-				canon_hostname = xstrdup(ai->ai_canonname);
-				free(ip_address);
-				ip_address = xstrdup(addrbuf);
-				break;
-			}
-			freeaddrinfo(ai0);
+			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
+
+			inet_ntop(AF_INET, &sin_addr->sin_addr,
+				  addrbuf, sizeof(addrbuf));
+			free(ip_address);
+			ip_address = xstrdup(addrbuf);
+
+			free(canon_hostname);
+			canon_hostname = xstrdup(ai->ai_canonname ?
+						 ai->ai_canonname : ip_address);
+
+			freeaddrinfo(ai);
 		}
 #else
 		struct hostent *hent;
-- 
1.6.3.rc3.1.g35108
Johannes Schindelin· Apr 27, 2009, 15:53 UTC · re: Benjamin Kramer · lore

Re: [PATCH v2] daemon.c: fix segfault on OS X

Hi,
On Mon, 27 Apr 2009, Benjamin Kramer wrote:
Show 12 quoted lines
> On OS X (and maybe other unices), getaddrinfo(3) returns NULL
> in the ai_canonname field if it's called with an IP address for
> the hostname. We'll now use the IP address for the hostname if
> ai_canonname was NULL, this also matches the behaviour on Linux.
> 
> steps to reproduce:
> $ git daemon --export-all
> $ git clone git://127.0.0.1/frotz
> => git daemon's fork (silently) segfaults.
> 
> Remove the pointless loop while at it. There is only one iteration
> because of the break; on the last line and there are no continues.

Thanks, I indeed overlooked the "break;", Dscho

Johannes Schindelin· Apr 27, 2009, 09:09 UTC · re: Benjamin Kramer · lore
Hi,
Disclaimer: if you are offended by constructive criticism, or likely to
answer with insults to the comments I offer, please stop reading this mail
now (and please do not answer my mail, either). :-)
Still with me?  Good.  Nice to meet you.

Just for the record: responding to a patch is my strongest way of saying that I appreciate your work.

On Sun, 26 Apr 2009, Benjamin Kramer wrote:
Show 9 quoted lines
> On OS X (and maybe other unices) getaddrinfo(3) returns NULL
> in the ai_canonname field if it is called with an IP address.
> 
> steps to reproduce:
> $ git daemon --export-all
> $ git clone git://127.0.0.1/frotz
> => git daemon's fork (silently) segfaults.
> 
> Remove the pointless loop while at it.

Why is it pointless? You have to explain why there is no possiblity to get multiple addrinfos back. (And come to think about it, I think it is perfectly possible for getaddrinfo to return multiple addresses for the same hostname.)

But what is more puzzling to me is what your patch is actually trying to do: fix the segfault. I have to assume -- as you were pretty scarce with information on that -- that the ip_address is never set, and that is causing the segfault. Now, with your patch, it seems to me that the ip_address will just be the empty string, which is hardly correct.

Am I wrong?

Ciao, Dscho

← back to recent threads