threads / patch / 19132

patchWorkaround for ai_canonname sometimes coming back as null

Subject: [PATCH] Workaround for ai_canonname sometimes coming back as null

## tl;dr

11 messages between Apr 29, 2009 and Apr 30, 2009. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Augie Fackler· Apr 29, 2009, 21:48 UTC · lore

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(-)
Show changes to daemon.c +4 −1
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
Alex Riesen· Apr 29, 2009, 21:55 UTC · re: Augie Fackler · lore

Re: [PATCH] Workaround for ai_canonname sometimes coming back as null

2009/4/29 Augie Fackler <durin42@gmail.com>:
Show 11 quoted lines
> @@ -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:
		inet_ntop(hent->h_addrtype, &sa.sin_addr,
			  addrbuf, sizeof(addrbuf));
		free(canon_hostname); /* CRASH */
		canon_hostname = xstrdup(hent->h_name);
		free(ip_address);
Augie Fackler· Apr 29, 2009, 21:56 UTC · re: Alex Riesen · lore

Re: [PATCH] Workaround for ai_canonname sometimes coming back as null

On Apr 29, 2009, at 4:55 PM, Alex Riesen wrote:
Show 25 quoted lines
> 2009/4/29 Augie Fackler <durin42@gmail.com>:
>> @@ -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:
>
> 		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?

Alex Riesen· Apr 29, 2009, 22:01 UTC · re: Augie Fackler · lore

Re: [PATCH] Workaround for ai_canonname sometimes coming back as null

2009/4/29 Augie Fackler <durin42@gmail.com>:
Show 23 quoted lines
> On Apr 29, 2009, at 4:55 PM, Alex Riesen wrote:
>> 2009/4/29 Augie Fackler <durin42@gmail.com>:
>>>
>>> @@ -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)

Alex Riesen· Apr 29, 2009, 22:04 UTC · re: Augie Fackler · lore

Re: [PATCH] Workaround for ai_canonname sometimes coming back as null

2009/4/29 Augie Fackler <durin42@gmail.com>:
Show 5 quoted lines
>>
>> 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.

Augie Fackler· Apr 29, 2009, 23:04 UTC · re: Alex Riesen · lore

[PATCH] Don't crash if ai_canonname comes back as null

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(-)
Show changes to daemon.c +1 −1
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
Junio C Hamano· Apr 29, 2009, 23:21 UTC · re: Augie Fackler · lore

Re: [PATCH] Don't crash if ai_canonname comes back as null

Augie Fackler <durin42@gmail.com> writes:
> Fixes a weird bug where git-daemon was segfaulting
> when started by sh(1) because ai_canonname was null.
> ---
> Fixed based on feedback.
Hmm.

I've been waiting for feedback to a patch proposed earlier in the same area, which is <49F5BA55.3060606@googlemail.com> ($gmane/117670). How does this new one relate to it?

Show 20 quoted lines
>  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
Augie Fackler· Apr 29, 2009, 23:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] Don't crash if ai_canonname comes back as null

On Apr 29, 2009, at 6:21 PM, Junio C Hamano wrote:
Show 12 quoted lines
> Augie Fackler <durin42@gmail.com> writes:
>
>> Fixes a weird bug where git-daemon was segfaulting
>> when started by sh(1) because ai_canonname was null.
>> ---
>> Fixed based on feedback.
>
> Hmm.
>
> I've been waiting for feedback to a patch proposed earlier in the same
> area, which is <49F5BA55.3060606@googlemail.com> ($gmane/117670).  How
> does this new one relate to it?

I can't comment much on the correctness of the code - my patch was the minimal change to have it not crash.

The other patch also works for me to prevent the crash, and looks like it might be a little more correct in terms of having a meaningful hostname.

Show 22 quoted lines
>> 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
>
Jon Loeliger· Apr 30, 2009, 14:13 UTC · re: Augie Fackler · lore

Re: [PATCH] Don't crash if ai_canonname comes back as null

Show 22 quoted lines
> 
> On Apr 29, 2009, at 6:21 PM, Junio C Hamano wrote:
> 
> > Augie Fackler <durin42@gmail.com> writes:
> >
> >> Fixes a weird bug where git-daemon was segfaulting
> >> when started by sh(1) because ai_canonname was null.
> >> ---
> >> Fixed based on feedback.
> >
> > Hmm.
> >
> > I've been waiting for feedback to a patch proposed earlier in the same
> > area, which is <49F5BA55.3060606@googlemail.com> ($gmane/117670).  How
> > does this new one relate to it?
> 
> I can't comment much on the correctness of the code - my patch was the  
> minimal change to have it not crash.
> 
> The other patch also works for me to prevent the crash, and looks like  
> it might be a little more correct in terms of having a meaningful  
> hostname.

So, I wasn't CC'ed on the referenced patch ($gmane/117670), but it seems to me that there might be value in actually looping over the whole list of addrinfo results exactly in the case that it does return a null canonical name for one of its addresses? Perhaps an inverse call to getnameinfo() is warranted too?

Sorry, I'm just not certain here.
jdl
Junio C Hamano· Apr 30, 2009, 16:57 UTC · re: Jon Loeliger · lore

Re: [PATCH] Don't crash if ai_canonname comes back as null

Jon Loeliger <jdl@jdl.com> writes:
Show 5 quoted lines
>> > I've been waiting for feedback to a patch proposed earlier in the same
>> > area, which is <49F5BA55.3060606@googlemail.com> ($gmane/117670).  How
>> > does this new one relate to it?
>> ... 
> So, I wasn't CC'ed on the referenced patch ($gmane/117670), but it

You did got CC'ed, but I got a bounce from your freescale address, so this time I tried another address of yours I knew about.

> seems to me that there might be value in actually looping over the
> whole list of addrinfo results exactly in the case that it does
> return a null canonical name for one of its addresses?

That is what I speculated when commenting on ($gmane/117670), but I think the original loop was not doing any check, and instead always exited early during its first iteration. Perhaps we can re-add a loop that does something useful, but I do not know what it would be offhand.

> Perhaps an
> inverse call to getnameinfo() is warranted too?

In this case the name being looked up is _ours_; it is not like "the client claims to be frotz---does frotz reverse map to him correctly?" situation, so reverse lookup might not be so interesting.

There is one thing that could potentially be useful when the daemon runs on a multi-homed host; git.jdl.com may have eth1 facing public and eth0 facing internal networks. Depending on which address you got the request to, you may want to serve different contents, and if you got request to "hostname" that is not you as far as getaddrinfo() is concerned, you may want to do yet another thing that is different from the two name-addr mapping returned by getaddrinfo().

I do not see enough information to do that kind of thing is passed to the parse_extra_args() function in the current callchain, though. We do have a call to getpeername() but we do not seem to do getsockname() to learn about our end of the connection.

Alex Riesen· Apr 29, 2009, 21:56 UTC · re: Alex Riesen · lore

Re: [PATCH] Workaround for ai_canonname sometimes coming back as null

2009/4/29 Alex Riesen <raa.lkml@gmail.com>:
Show 15 quoted lines
> 2009/4/29 Augie Fackler <durin42@gmail.com>:
>> @@ -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.

← back to recent threads