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

Benjamin Kramer [off-list ref] writes:

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.

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

Junio C Hamano wrote:

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

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

Hi,

On Mon, 27 Apr 2009, Benjamin Kramer wrote:

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