{"thread":{"id":"19069","subject":"Re: [PATCH] daemon.c: fix segfault on OS X","startedAt":"2009-04-26T20:01:35Z","lastAt":"2009-04-27T15:53:25Z","messageCount":5,"participants":["Junio C Hamano","Benjamin Kramer","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"112365","messageId":"49F4BD9F.6070709@googlemail.com","threadId":"19069","inReplyTo":null,"subject":"[PATCH] daemon.c: fix segfault on OS X","fromName":"Benjamin Kramer","fromEmail":"benny.kra@googlemail.com","sentAt":"2009-04-26T20:01:35Z","receivedAt":"2009-04-26T20:01:35Z","isPatch":true,"sender":{"key":"benny.kra@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/16542?v=4"},"body":"On OS X (and maybe other unices) getaddrinfo(3) returns NULL\nin the ai_canonname field if it is called with an IP address.\n\nsteps to reproduce:\n$ git daemon --export-all\n$ git clone git://127.0.0.1/frotz\n=> git daemon's fork (silently) segfaults.\n\nRemove the pointless loop while at it.\n\nSigned-off-by: Benjamin Kramer <benny.kra@googlemail.com>\n---\n daemon.c |   20 ++++++++++----------\n 1 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 13401f1..5f455ab 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -444,27 +444,27 @@ static void parse_extra_args(char *extra_args, int buflen)\n \tif (hostname) {\n #ifndef NO_IPV6\n \t\tstruct addrinfo hints;\n-\t\tstruct addrinfo *ai, *ai0;\n+\t\tstruct addrinfo *ai;\n \t\tint gai;\n \t\tstatic char addrbuf[HOST_NAME_MAX + 1];\n \n \t\tmemset(&hints, 0, sizeof(hints));\n \t\thints.ai_flags = AI_CANONNAME;\n \n-\t\tgai = getaddrinfo(hostname, 0, &hints, &ai0);\n+\t\tgai = getaddrinfo(hostname, 0, &hints, &ai);\n \t\tif (!gai) {\n-\t\t\tfor (ai = ai0; ai; ai = ai->ai_next) {\n-\t\t\t\tstruct sockaddr_in *sin_addr = (void *)ai->ai_addr;\n+\t\t\tstruct sockaddr_in *sin_addr = (void *)ai->ai_addr;\n \n-\t\t\t\tinet_ntop(AF_INET, &sin_addr->sin_addr,\n-\t\t\t\t\t  addrbuf, sizeof(addrbuf));\n+\t\t\tinet_ntop(AF_INET, &sin_addr->sin_addr,\n+\t\t\t\t  addrbuf, sizeof(addrbuf));\n+\t\t\tfree(ip_address);\n+\t\t\tip_address = xstrdup(addrbuf);\n+\n+\t\t\tif (ai->ai_canonname) {\n \t\t\t\tfree(canon_hostname);\n \t\t\t\tcanon_hostname = xstrdup(ai->ai_canonname);\n-\t\t\t\tfree(ip_address);\n-\t\t\t\tip_address = xstrdup(addrbuf);\n-\t\t\t\tbreak;\n \t\t\t}\n-\t\t\tfreeaddrinfo(ai0);\n+\t\t\tfreeaddrinfo(ai);\n \t\t}\n #else\n \t\tstruct hostent *hent;\n-- \n1.6.3.rc2.9.g76386\n"},{"id":"112348","messageId":"7vy6tmbzj3.fsf@gitster.siamese.dyndns.org","threadId":"19069","inReplyTo":"49F4BD9F.6070709@googlemail.com","subject":"Re: [PATCH] daemon.c: fix segfault on OS X","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-27T02:28:48Z","receivedAt":"2009-04-27T02:28:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benjamin Kramer <benny.kra@googlemail.com> writes:\n\n> On OS X (and maybe other unices) getaddrinfo(3) returns NULL\n> in the ai_canonname field if it is called with an IP address.\n>\n> steps to reproduce:\n> $ git daemon --export-all\n> $ git clone git://127.0.0.1/frotz\n> => git daemon's fork (silently) segfaults.\n>\n> Remove the pointless loop while at it.\n\nHmm.  This codepath comes from dd467629, both the loop and the use of\ngetaddrinfo.\n\nI have a mild suspicion that the loop originally was meant to notice that\nan element in the addrinfo linked list is unusable and skip it to find a\nusable one in the list or something like that, but as it stands it\nprocesses the first entry (or fails to process and segfaults for you ;-)\nand breaks out, which indeed is pointless.\n\nBut the part your patch touches is about supporting virtual hosting via\npattern interpolation, and the daemon will still segfault even with your\npatch when somebody uses %CH expansion, because canon_hostname is left as\nNULL, won't it?  I suspect in such a case it might be safer to use a copy\nof the ip_address or something.\n"},{"id":"112400","messageId":"alpine.DEB.1.00.0904271103050.7741@intel-tinevez-2-302","threadId":"19069","inReplyTo":"49F4BD9F.6070709@googlemail.com","subject":"Re: [PATCH] daemon.c: fix segfault on OS X","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-27T09:09:11Z","receivedAt":"2009-04-27T09:09:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nDisclaimer: if you are offended by constructive criticism, or likely to\nanswer with insults to the comments I offer, please stop reading this mail\nnow (and please do not answer my mail, either). :-)\n\nStill with me?  Good.  Nice to meet you.\n\nJust for the record: responding to a patch is my strongest way of saying\nthat I appreciate your work.\n\nOn Sun, 26 Apr 2009, Benjamin Kramer wrote:\n\n> On OS X (and maybe other unices) getaddrinfo(3) returns NULL\n> in the ai_canonname field if it is called with an IP address.\n> \n> steps to reproduce:\n> $ git daemon --export-all\n> $ git clone git://127.0.0.1/frotz\n> => git daemon's fork (silently) segfaults.\n> \n> Remove the pointless loop while at it.\n\nWhy is it pointless?  You have to explain why there is no possiblity to \nget multiple addrinfos back.  (And come to think about it, I think it is \nperfectly possible for getaddrinfo to return multiple addresses for the \nsame hostname.)\n\nBut what is more puzzling to me is what your patch is actually trying to \ndo: fix the segfault.  I have to assume -- as you were pretty scarce with \ninformation on that -- that the ip_address is never set, and that is \ncausing the segfault.  Now, with your patch, it seems to me that the \nip_address will just be the empty string, which is hardly correct.\n\nAm I wrong?\n\nCiao,\nDscho\n"},{"id":"112416","messageId":"49F5BA55.3060606@googlemail.com","threadId":"19069","inReplyTo":"7vy6tmbzj3.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v2] daemon.c: fix segfault on OS X","fromName":"Benjamin Kramer","fromEmail":"benny.kra@googlemail.com","sentAt":"2009-04-27T13:59:49Z","receivedAt":"2009-04-27T13:59:49Z","isPatch":true,"sender":{"key":"benny.kra@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/16542?v=4"},"body":"On OS X (and maybe other unices), getaddrinfo(3) returns NULL\nin the ai_canonname field if it's called with an IP address for\nthe hostname. We'll now use the IP address for the hostname if\nai_canonname was NULL, this also matches the behaviour on Linux.\n\nsteps to reproduce:\n$ git daemon --export-all\n$ git clone git://127.0.0.1/frotz\n=> git daemon's fork (silently) segfaults.\n\nRemove the pointless loop while at it. There is only one iteration\nbecause of the break; on the last line and there are no continues.\n\nSigned-off-by: Benjamin Kramer <benny.kra@googlemail.com>\n---\n\nJunio C Hamano wrote:\n> But the part your patch touches is about supporting virtual hosting via\n> pattern interpolation, and the daemon will still segfault even with your\n> patch when somebody uses %CH expansion, because canon_hostname is left as\n> NULL, won't it?  I suspect in such a case it might be safer to use a copy\n> of the ip_address or something.\n\nIt doesn't segfault but it just assumes an empty hostname:\n\n  $ git daemon --verbose --export-all --interpolated-path=%CH/%D\n    [3251] Connection from 127.0.0.1:49423\n    [3251] Extended attributes (16 bytes) exist <host=127.0.0.1>\n    [3251] Request upload-pack for '/frotz'\n    [3251] Interpolated dir '//frotz'\n\nThis upated patch uses a copy of ip_address. I did a quick test on my\nLinux box and it looks like Linux' getaddrinfo(3) always returns the IP\naddress in ai_canonname instead of NULL when it is called with an IP\naddress hostname.\n\n  $ git daemon --verbose --export-all --interpolated-path=%CH/%D\n    [3871] Connection from 127.0.0.1:49427\n    [3871] Extended attributes (16 bytes) exist <host=127.0.0.1>\n    [3871] Request upload-pack for '/frotz'\n    [3871] Interpolated dir '127.0.0.1//frotz'\n\n\nBtw, if I connect to an IPv6 host with interpolated-path=%IP the IP\naddress gets converted to 0.0.0.0. Is this desired behaviour or yet\nanother bug?\n\n(I hope thunderbird didn't f*ck up the formatting this time)\n\n\n daemon.c |   28 ++++++++++++++--------------\n 1 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 13401f1..daa4c8e 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -444,27 +444,27 @@ static void parse_extra_args(char *extra_args, int buflen)\n \tif (hostname) {\n #ifndef NO_IPV6\n \t\tstruct addrinfo hints;\n-\t\tstruct addrinfo *ai, *ai0;\n+\t\tstruct addrinfo *ai;\n \t\tint gai;\n \t\tstatic char addrbuf[HOST_NAME_MAX + 1];\n \n \t\tmemset(&hints, 0, sizeof(hints));\n \t\thints.ai_flags = AI_CANONNAME;\n \n-\t\tgai = getaddrinfo(hostname, 0, &hints, &ai0);\n+\t\tgai = getaddrinfo(hostname, 0, &hints, &ai);\n \t\tif (!gai) {\n-\t\t\tfor (ai = ai0; ai; ai = ai->ai_next) {\n-\t\t\t\tstruct sockaddr_in *sin_addr = (void *)ai->ai_addr;\n-\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\tfree(ip_address);\n-\t\t\t\tip_address = xstrdup(addrbuf);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tfreeaddrinfo(ai0);\n+\t\t\tstruct sockaddr_in *sin_addr = (void *)ai->ai_addr;\n+\n+\t\t\tinet_ntop(AF_INET, &sin_addr->sin_addr,\n+\t\t\t\t  addrbuf, sizeof(addrbuf));\n+\t\t\tfree(ip_address);\n+\t\t\tip_address = xstrdup(addrbuf);\n+\n+\t\t\tfree(canon_hostname);\n+\t\t\tcanon_hostname = xstrdup(ai->ai_canonname ?\n+\t\t\t\t\t\t ai->ai_canonname : ip_address);\n+\n+\t\t\tfreeaddrinfo(ai);\n \t\t}\n #else\n \t\tstruct hostent *hent;\n-- \n1.6.3.rc3.1.g35108\n"},{"id":"112427","messageId":"alpine.DEB.1.00.0904271753190.7741@intel-tinevez-2-302","threadId":"19069","inReplyTo":"49F5BA55.3060606@googlemail.com","subject":"Re: [PATCH v2] daemon.c: fix segfault on OS X","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-27T15:53:25Z","receivedAt":"2009-04-27T15:53:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 27 Apr 2009, Benjamin Kramer wrote:\n\n> On OS X (and maybe other unices), getaddrinfo(3) returns NULL\n> in the ai_canonname field if it's called with an IP address for\n> the hostname. We'll now use the IP address for the hostname if\n> ai_canonname was NULL, this also matches the behaviour on Linux.\n> \n> steps to reproduce:\n> $ git daemon --export-all\n> $ git clone git://127.0.0.1/frotz\n> => git daemon's fork (silently) segfaults.\n> \n> Remove the pointless loop while at it. There is only one iteration\n> because of the break; on the last line and there are no continues.\n\nThanks, I indeed overlooked the \"break;\",\nDscho\n"}]}