{"thread":{"id":"19132","subject":"[PATCH] Workaround for ai_canonname sometimes coming back as null","startedAt":"2009-04-29T21:48:57Z","lastAt":"2009-04-30T16:57:03Z","messageCount":11,"participants":["Augie Fackler","Alex Riesen","Junio C Hamano","Jon Loeliger"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"112702","messageId":"9C355DCC-0240-4B9E-83CA-083B51C2E34C@gmail.com","threadId":"19132","inReplyTo":null,"subject":"[PATCH] Workaround for ai_canonname sometimes coming back as null","fromName":"Augie Fackler","fromEmail":"durin42@gmail.com","sentAt":"2009-04-29T21:48:57Z","receivedAt":"2009-04-29T21:48:57Z","isPatch":true,"sender":{"key":"durin42@gmail.com","avatar":"https://gravatar.com/avatar/72a475e7d51be65c5c8db591c22f45e0fc18dbef4df90c9f4b2ac6d67603e765?d=mp&s=160"},"body":"Fix a weird bug where git-daemon was segfaulting when started by sh(1)\nbecause ai_canonname was null.\n\n---\nI'm not really sure why being started by sh has any measurable impact.\ngit-daemon works fine if I start it manually from an interactive prompt.\n\nEasy reproduction script (the git clone command will fail reliably for  \nme without this patch):\n\n#!/bin/sh\nmkdir temp\ncd temp\nmkdir narf\ncd narf\ngit init\necho a > a\ngit add a\ngit commit -am 'hi'\ncd ..\ngit daemon --base-path=\"$(pwd)\"\\\n  --listen=127.0.0.1\\\n  --export-all\\\n  --pid-file=gitdaemon.pid \\\n  --detach --reuseaddr\ngit clone git://127.0.0.1/narf bla\nkill `cat gitdaemon.pid`\n\n\n  daemon.c |    5 ++++-\n  1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 13401f1..b1fede0 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args,  \nint buflen)\n  \t\t\t\tinet_ntop(AF_INET, &sin_addr->sin_addr,\n  \t\t\t\t\t  addrbuf, sizeof(addrbuf));\n  \t\t\t\tfree(canon_hostname);\n-\t\t\t\tcanon_hostname = xstrdup(ai->ai_canonname);\n+\t\t\t\tif (ai->ai_canonname)\n+\t\t\t\t\tcanon_hostname = xstrdup(ai->ai_canonname);\n+\t\t\t\telse\n+\t\t\t\t\tcanon_hostname = \"unknown\";\n  \t\t\t\tfree(ip_address);\n  \t\t\t\tip_address = xstrdup(addrbuf);\n  \t\t\t\tbreak;\n-- \n1.6.3.rc3.12.gb7937\n"},{"id":"112704","messageId":"81b0412b0904291455n47f83e9ftcbdec0ff1c0ea03@mail.gmail.com","threadId":"19132","inReplyTo":"9C355DCC-0240-4B9E-83CA-083B51C2E34C@gmail.com","subject":"Re: [PATCH] Workaround for ai_canonname sometimes coming back as null","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-29T21:55:01Z","receivedAt":"2009-04-29T21:55:01Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/29 Augie Fackler <durin42@gmail.com>:\n> @@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args, int\n> buflen)\n>                                inet_ntop(AF_INET, &sin_addr->sin_addr,\n>                                          addrbuf, sizeof(addrbuf));\n>                                free(canon_hostname);\n> -                               canon_hostname = xstrdup(ai->ai_canonname);\n> +                               if (ai->ai_canonname)\n> +                                       canon_hostname =\n> xstrdup(ai->ai_canonname);\n> +                               else\n> +                                       canon_hostname = \"unknown\";\n\nThis last line will crash some lines down, when canon_hostname is free'd:\n\n\t\tinet_ntop(hent->h_addrtype, &sa.sin_addr,\n\t\t\t  addrbuf, sizeof(addrbuf));\n\n\t\tfree(canon_hostname); /* CRASH */\n\t\tcanon_hostname = xstrdup(hent->h_name);\n\t\tfree(ip_address);\n"},{"id":"112706","messageId":"6B7EA51D-8412-4E6A-BA7B-156FD5B755E8@gmail.com","threadId":"19132","inReplyTo":"81b0412b0904291455n47f83e9ftcbdec0ff1c0ea03@mail.gmail.com","subject":"Re: [PATCH] Workaround for ai_canonname sometimes coming back as null","fromName":"Augie Fackler","fromEmail":"durin42@gmail.com","sentAt":"2009-04-29T21:56:08Z","receivedAt":"2009-04-29T21:56:08Z","isPatch":true,"sender":{"key":"durin42@gmail.com","avatar":"https://gravatar.com/avatar/72a475e7d51be65c5c8db591c22f45e0fc18dbef4df90c9f4b2ac6d67603e765?d=mp&s=160"},"body":"\nOn Apr 29, 2009, at 4:55 PM, Alex Riesen wrote:\n\n> 2009/4/29 Augie Fackler <durin42@gmail.com>:\n>> @@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args,  \n>> int\n>> buflen)\n>>                                inet_ntop(AF_INET, &sin_addr- \n>> >sin_addr,\n>>                                          addrbuf, sizeof(addrbuf));\n>>                                free(canon_hostname);\n>> -                               canon_hostname = xstrdup(ai- \n>> >ai_canonname);\n>> +                               if (ai->ai_canonname)\n>> +                                       canon_hostname =\n>> xstrdup(ai->ai_canonname);\n>> +                               else\n>> +                                       canon_hostname = \"unknown\";\n>\n> This last line will crash some lines down, when canon_hostname is  \n> free'd:\n>\n> \t\tinet_ntop(hent->h_addrtype, &sa.sin_addr,\n> \t\t\t  addrbuf, sizeof(addrbuf));\n>\n> \t\tfree(canon_hostname); /* CRASH */\n> \t\tcanon_hostname = xstrdup(hent->h_name);\n> \t\tfree(ip_address);\n\n\nOdd, because I'm running with that exact code and not seeing the  \nproblem. Should I resubmit an updated patch that xstrdup's unknown  \ninto canon_hostname?\n"},{"id":"112705","messageId":"81b0412b0904291456h6e6fa2f0h840e7b3569573be0@mail.gmail.com","threadId":"19132","inReplyTo":"81b0412b0904291455n47f83e9ftcbdec0ff1c0ea03@mail.gmail.com","subject":"Re: [PATCH] Workaround for ai_canonname sometimes coming back as null","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-29T21:56:09Z","receivedAt":"2009-04-29T21:56:09Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/29 Alex Riesen <raa.lkml@gmail.com>:\n> 2009/4/29 Augie Fackler <durin42@gmail.com>:\n>> @@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args, int\n>> buflen)\n>>                                inet_ntop(AF_INET, &sin_addr->sin_addr,\n>>                                          addrbuf, sizeof(addrbuf));\n>>                                free(canon_hostname);\n>> -                               canon_hostname = xstrdup(ai->ai_canonname);\n>> +                               if (ai->ai_canonname)\n>> +                                       canon_hostname =\n>> xstrdup(ai->ai_canonname);\n>> +                               else\n>> +                                       canon_hostname = \"unknown\";\n>\n> This last line will crash some lines down, when canon_hostname is free'd:\n>\n\nActually, it will crash in the line just above. On the same reasons.\n"},{"id":"112707","messageId":"81b0412b0904291501w501eecd4y18927018d57bdbdc@mail.gmail.com","threadId":"19132","inReplyTo":"6B7EA51D-8412-4E6A-BA7B-156FD5B755E8@gmail.com","subject":"Re: [PATCH] Workaround for ai_canonname sometimes coming back as null","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-29T22:01:28Z","receivedAt":"2009-04-29T22:01:28Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/29 Augie Fackler <durin42@gmail.com>:\n> On Apr 29, 2009, at 4:55 PM, Alex Riesen wrote:\n>> 2009/4/29 Augie Fackler <durin42@gmail.com>:\n>>>\n>>> @@ -459,7 +459,10 @@ static void parse_extra_args(char *extra_args, int\n>>> buflen)\n>>>                               inet_ntop(AF_INET, &sin_addr->sin_addr,\n>>>                                         addrbuf, sizeof(addrbuf));\n>>>                               free(canon_hostname);\n>>> -                               canon_hostname =\n>>> xstrdup(ai->ai_canonname);\n>>> +                               if (ai->ai_canonname)\n>>> +                                       canon_hostname =\n>>> xstrdup(ai->ai_canonname);\n>>> +                               else\n>>> +                                       canon_hostname = \"unknown\";\n>>\n>> This last line will crash some lines down, when canon_hostname is free'd:\n>>\n>\n> Odd, because I'm running with that exact code and not seeing the problem.\n> Should I resubmit an updated patch that xstrdup's unknown into\n> canon_hostname?\n>\n\nI think you can just let canon_hostname be NULL (i.e. don't strdup it,\nif ai_canonname is NULL). NULL values of canon_hostname seem\nto be handled just fine: see path_ok and strbuf_expand_dict_cb (strbuf.c)\n"},{"id":"112708","messageId":"81b0412b0904291504k3261df5fl692d09c6c761887e@mail.gmail.com","threadId":"19132","inReplyTo":"6B7EA51D-8412-4E6A-BA7B-156FD5B755E8@gmail.com","subject":"Re: [PATCH] Workaround for ai_canonname sometimes coming back as null","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-04-29T22:04:14Z","receivedAt":"2009-04-29T22:04:14Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/4/29 Augie Fackler <durin42@gmail.com>:\n>>\n>> This last line will crash some lines down, when canon_hostname is free'd:\n>>\n>\n> Odd, because I'm running with that exact code and not seeing the problem.\n\nPure luck (see the message regarding \"the line above\". The first was bogus,\nof course. It is in the other leg of #ifndef NO_IPV6). The \"line above\" will\ncrash should you have more than one element in gai list.\n"},{"id":"112719","messageId":"C2AC0D7A-3E11-4A3A-8447-5D7582547B13@gmail.com","threadId":"19132","inReplyTo":"81b0412b0904291504k3261df5fl692d09c6c761887e@mail.gmail.com","subject":"[PATCH] Don't crash if ai_canonname comes back as null","fromName":"Augie Fackler","fromEmail":"durin42@gmail.com","sentAt":"2009-04-29T23:04:42Z","receivedAt":"2009-04-29T23:04:42Z","isPatch":true,"sender":{"key":"durin42@gmail.com","avatar":"https://gravatar.com/avatar/72a475e7d51be65c5c8db591c22f45e0fc18dbef4df90c9f4b2ac6d67603e765?d=mp&s=160"},"body":"Fixes a weird bug where git-daemon was segfaulting\nwhen started by sh(1) because ai_canonname was null.\n---\nFixed based on feedback.\n\n  daemon.c |    2 +-\n  1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 13401f1..ae21d92 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -459,7 +459,7 @@ static void parse_extra_args(char *extra_args, int  \nbuflen)\n  \t\t\t\tinet_ntop(AF_INET, &sin_addr->sin_addr,\n  \t\t\t\t\t  addrbuf, sizeof(addrbuf));\n  \t\t\t\tfree(canon_hostname);\n-\t\t\t\tcanon_hostname = xstrdup(ai->ai_canonname);\n+\t\t\t\tcanon_hostname = ai->ai_canonname ? xstrdup(ai->ai_canonname) :  \nNULL;\n  \t\t\t\tfree(ip_address);\n  \t\t\t\tip_address = xstrdup(addrbuf);\n  \t\t\t\tbreak;\n-- \n1.6.2.GIT\n"},{"id":"112722","messageId":"7v63gn59mw.fsf@gitster.siamese.dyndns.org","threadId":"19132","inReplyTo":"C2AC0D7A-3E11-4A3A-8447-5D7582547B13@gmail.com","subject":"Re: [PATCH] Don't crash if ai_canonname comes back as null","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-29T23:21:27Z","receivedAt":"2009-04-29T23:21:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Augie Fackler <durin42@gmail.com> writes:\n\n> Fixes a weird bug where git-daemon was segfaulting\n> when started by sh(1) because ai_canonname was null.\n> ---\n> Fixed based on feedback.\n\nHmm.\n\nI've been waiting for feedback to a patch proposed earlier in the same\narea, which is <49F5BA55.3060606@googlemail.com> ($gmane/117670).  How\ndoes this new one relate to it?\n\n>  daemon.c |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index 13401f1..ae21d92 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -459,7 +459,7 @@ static void parse_extra_args(char *extra_args, int\n> buflen)\n>  \t\t\t\tinet_ntop(AF_INET, &sin_addr->sin_addr,\n>  \t\t\t\t\t  addrbuf, sizeof(addrbuf));\n>  \t\t\t\tfree(canon_hostname);\n> -\t\t\t\tcanon_hostname = xstrdup(ai->ai_canonname);\n> +\t\t\t\tcanon_hostname = ai->ai_canonname ?\n> xstrdup(ai->ai_canonname) : NULL;\n>  \t\t\t\tfree(ip_address);\n>  \t\t\t\tip_address = xstrdup(addrbuf);\n>  \t\t\t\tbreak;\n> --\n> 1.6.2.GIT\n"},{"id":"112724","messageId":"A85E96CC-CF0B-40F9-9960-00485285E6ED@gmail.com","threadId":"19132","inReplyTo":"7v63gn59mw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Don't crash if ai_canonname comes back as null","fromName":"Augie Fackler","fromEmail":"durin42@gmail.com","sentAt":"2009-04-29T23:32:02Z","receivedAt":"2009-04-29T23:32:02Z","isPatch":true,"sender":{"key":"durin42@gmail.com","avatar":"https://gravatar.com/avatar/72a475e7d51be65c5c8db591c22f45e0fc18dbef4df90c9f4b2ac6d67603e765?d=mp&s=160"},"body":"\nOn Apr 29, 2009, at 6:21 PM, Junio C Hamano wrote:\n\n> Augie Fackler <durin42@gmail.com> writes:\n>\n>> Fixes a weird bug where git-daemon was segfaulting\n>> when started by sh(1) because ai_canonname was null.\n>> ---\n>> Fixed based on feedback.\n>\n> Hmm.\n>\n> I've been waiting for feedback to a patch proposed earlier in the same\n> area, which is <49F5BA55.3060606@googlemail.com> ($gmane/117670).  How\n> does this new one relate to it?\n\nI can't comment much on the correctness of the code - my patch was the  \nminimal change to have it not crash.\n\nThe other patch also works for me to prevent the crash, and looks like  \nit might be a little more correct in terms of having a meaningful  \nhostname.\n\n>> daemon.c |    2 +-\n>> 1 files changed, 1 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/daemon.c b/daemon.c\n>> index 13401f1..ae21d92 100644\n>> --- a/daemon.c\n>> +++ b/daemon.c\n>> @@ -459,7 +459,7 @@ static void parse_extra_args(char *extra_args,  \n>> int\n>> buflen)\n>> \t\t\t\tinet_ntop(AF_INET, &sin_addr->sin_addr,\n>> \t\t\t\t\t  addrbuf, sizeof(addrbuf));\n>> \t\t\t\tfree(canon_hostname);\n>> -\t\t\t\tcanon_hostname = xstrdup(ai->ai_canonname);\n>> +\t\t\t\tcanon_hostname = ai->ai_canonname ?\n>> xstrdup(ai->ai_canonname) : NULL;\n>> \t\t\t\tfree(ip_address);\n>> \t\t\t\tip_address = xstrdup(addrbuf);\n>> \t\t\t\tbreak;\n>> --\n>> 1.6.2.GIT\n>\n"},{"id":"112766","messageId":"E1LzX1N-0003sw-2y@jdl.com","threadId":"19132","inReplyTo":"A85E96CC-CF0B-40F9-9960-00485285E6ED@gmail.com","subject":"Re: [PATCH] Don't crash if ai_canonname comes back as null","fromName":"Jon Loeliger","fromEmail":"jdl@jdl.com","sentAt":"2009-04-30T14:13:53Z","receivedAt":"2009-04-30T14:13:53Z","isPatch":true,"sender":{"key":"jdl@jdl.com","avatar":"https://gravatar.com/avatar/75ce9a10b151acd2c28ec4ab2136dba7b2ff1634530bd04b155981a749d08a64?d=mp&s=160"},"body":"> \n> On Apr 29, 2009, at 6:21 PM, Junio C Hamano wrote:\n> \n> > Augie Fackler <durin42@gmail.com> writes:\n> >\n> >> Fixes a weird bug where git-daemon was segfaulting\n> >> when started by sh(1) because ai_canonname was null.\n> >> ---\n> >> Fixed based on feedback.\n> >\n> > Hmm.\n> >\n> > I've been waiting for feedback to a patch proposed earlier in the same\n> > area, which is <49F5BA55.3060606@googlemail.com> ($gmane/117670).  How\n> > does this new one relate to it?\n> \n> I can't comment much on the correctness of the code - my patch was the  \n> minimal change to have it not crash.\n> \n> The other patch also works for me to prevent the crash, and looks like  \n> it might be a little more correct in terms of having a meaningful  \n> hostname.\n\nSo, I wasn't CC'ed on the referenced patch ($gmane/117670), but it\nseems to me that there might be value in actually looping over the\nwhole list of addrinfo results exactly in the case that it does\nreturn a null canonical name for one of its addresses?  Perhaps an\ninverse call to getnameinfo() is warranted too?\n\nSorry, I'm just not certain here.\n\njdl\n"},{"id":"112778","messageId":"7viqkm5bc0.fsf@gitster.siamese.dyndns.org","threadId":"19132","inReplyTo":"E1LzX1N-0003sw-2y@jdl.com","subject":"Re: [PATCH] Don't crash if ai_canonname comes back as null","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-30T16:57:03Z","receivedAt":"2009-04-30T16:57:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jon Loeliger <jdl@jdl.com> writes:\n\n>> > I've been waiting for feedback to a patch proposed earlier in the same\n>> > area, which is <49F5BA55.3060606@googlemail.com> ($gmane/117670).  How\n>> > does this new one relate to it?\n>> ... \n> So, I wasn't CC'ed on the referenced patch ($gmane/117670), but it\n\nYou did got CC'ed, but I got a bounce from your freescale address, so this\ntime I tried another address of yours I knew about.\n\n> seems to me that there might be value in actually looping over the\n> whole list of addrinfo results exactly in the case that it does\n> return a null canonical name for one of its addresses?\n\nThat is what I speculated when commenting on ($gmane/117670), but I think\nthe original loop was not doing any check, and instead always exited early\nduring its first iteration.  Perhaps we can re-add a loop that does\nsomething useful, but I do not know what it would be offhand.\n\n> Perhaps an\n> inverse call to getnameinfo() is warranted too?\n\nIn this case the name being looked up is _ours_; it is not like \"the\nclient claims to be frotz---does frotz reverse map to him correctly?\"\nsituation, so reverse lookup might not be so interesting.\n\nThere is one thing that could potentially be useful when the daemon runs\non a multi-homed host; git.jdl.com may have eth1 facing public and eth0\nfacing internal networks.  Depending on which address you got the request\nto, you may want to serve different contents, and if you got request to\n\"hostname\" that is not you as far as getaddrinfo() is concerned, you may\nwant to do yet another thing that is different from the two name-addr\nmapping returned by getaddrinfo().\n\nI do not see enough information to do that kind of thing is passed to the\nparse_extra_args() function in the current callchain, though.  We do have\na call to getpeername() but we do not seem to do getsockname() to learn\nabout our end of the connection.\n"}]}