[PATCH] Workaround for ai_canonname sometimes coming back as null

Fix a weird bug where git-daemon was segfaulting when started by sh(1)
because ai_canonname was null.

---
I'm not really sure why being started by sh has any measurable impact.
git-daemon works fine if I start it manually from an interactive prompt.

Easy reproduction script (the git clone command will fail reliably for  
me without this patch):

#!/bin/sh
mkdir temp
cd temp
mkdir narf
cd narf
git init
echo a > a
git add a
git commit -am 'hi'
cd ..
git daemon --base-path="$(pwd)"\
  --listen=127.0.0.1\
  --export-all\
  --pid-file=gitdaemon.pid \
  --detach --reuseaddr
git clone git://127.0.0.1/narf bla
kill `cat gitdaemon.pid`


  daemon.c |    5 ++++-
  1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/daemon.c b/daemon.c
index 13401f1..b1fede0 100644
--- a/daemon.c
+++ b/daemon.c

@@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args,  

int buflen)
  				inet_ntop(AF_INET, &sin_addr->sin_addr,
  					  addrbuf, sizeof(addrbuf));
  				free(canon_hostname);
-				canon_hostname = xstrdup(ai->ai_canonname);
+				if (ai->ai_canonname)
+					canon_hostname = xstrdup(ai->ai_canonname);
+				else
+					canon_hostname = "unknown";
  				free(ip_address);
  				ip_address = xstrdup(addrbuf);
  				break;
-- 
1.6.3.rc3.12.gb7937

2009/4/29 Augie Fackler [off-list ref]:

This last line will crash some lines down, when canon_hostname is free'd:

		inet_ntop(hent->h_addrtype, &sa.sin_addr,
			  addrbuf, sizeof(addrbuf));

		free(canon_hostname); /* CRASH */
		canon_hostname = xstrdup(hent->h_name);
		free(ip_address);

2009/4/29 Alex Riesen [off-list ref]:

2009/4/29 Augie Fackler [off-list ref]:

quoted

@@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args, int

buflen)
                               inet_ntop(AF_INET, &sin_addr->sin_addr,
                                         addrbuf, sizeof(addrbuf));
                               free(canon_hostname);
-                               canon_hostname = xstrdup(ai->ai_canonname);
+                               if (ai->ai_canonname)
+                                       canon_hostname =
xstrdup(ai->ai_canonname);
+                               else
+                                       canon_hostname = "unknown";

This last line will crash some lines down, when canon_hostname is free'd:

Actually, it will crash in the line just above. On the same reasons.

On Apr 29, 2009, at 4:55 PM, Alex Riesen wrote:

2009/4/29 Augie Fackler [off-list ref]:

quoted

@@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args,  

int
buflen)
                               inet_ntop(AF_INET, &sin_addr- 

quoted

sin_addr,

                                         addrbuf, sizeof(addrbuf));
                               free(canon_hostname);
-                               canon_hostname = xstrdup(ai- 

quoted

ai_canonname);

+                               if (ai->ai_canonname)
+                                       canon_hostname =
xstrdup(ai->ai_canonname);
+                               else
+                                       canon_hostname = "unknown";

This last line will crash some lines down, when canon_hostname is  
free'd:

		inet_ntop(hent->h_addrtype, &sa.sin_addr,
			  addrbuf, sizeof(addrbuf));

		free(canon_hostname); /* CRASH */
		canon_hostname = xstrdup(hent->h_name);
		free(ip_address);

Odd, because I'm running with that exact code and not seeing the  
problem. Should I resubmit an updated patch that xstrdup's unknown  
into canon_hostname?

2009/4/29 Augie Fackler [off-list ref]:

On Apr 29, 2009, at 4:55 PM, Alex Riesen wrote:

quoted

2009/4/29 Augie Fackler [off-list ref]:

quoted

@@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args, int

buflen)
                              inet_ntop(AF_INET, &sin_addr->sin_addr,
                                        addrbuf, sizeof(addrbuf));
                              free(canon_hostname);
-                               canon_hostname =
xstrdup(ai->ai_canonname);
+                               if (ai->ai_canonname)
+                                       canon_hostname =
xstrdup(ai->ai_canonname);
+                               else
+                                       canon_hostname = "unknown";

This last line will crash some lines down, when canon_hostname is free'd:

Odd, because I'm running with that exact code and not seeing the problem.
Should I resubmit an updated patch that xstrdup's unknown into
canon_hostname?

I think you can just let canon_hostname be NULL (i.e. don't strdup it,
if ai_canonname is NULL). NULL values of canon_hostname seem
to be handled just fine: see path_ok and strbuf_expand_dict_cb (strbuf.c)

2009/4/29 Augie Fackler [off-list ref]:

quoted

This last line will crash some lines down, when canon_hostname is free'd:

Odd, because I'm running with that exact code and not seeing the problem.

Pure luck (see the message regarding "the line above". The first was bogus,
of course. It is in the other leg of #ifndef NO_IPV6). The "line above" will
crash should you have more than one element in gai list.

Fixes a weird bug where git-daemon was segfaulting
when started by sh(1) because ai_canonname was null.
---
Fixed based on feedback.

  daemon.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/daemon.c b/daemon.c
index 13401f1..ae21d92 100644
--- a/daemon.c
+++ b/daemon.c

@@ -459,7 +459,7 @@ static void parse_extra_args(char *extra_args, int  

buflen)
  				inet_ntop(AF_INET, &sin_addr->sin_addr,
  					  addrbuf, sizeof(addrbuf));
  				free(canon_hostname);
-				canon_hostname = xstrdup(ai->ai_canonname);
+				canon_hostname = ai->ai_canonname ? xstrdup(ai->ai_canonname) :  
NULL;
  				free(ip_address);
  				ip_address = xstrdup(addrbuf);
  				break;
-- 
1.6.2.GIT