{"thread":{"id":"30756","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","startedAt":"2012-06-10T09:05:16Z","lastAt":"2012-06-13T20:39:59Z","messageCount":9,"participants":["Jonathan Nieder","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"193237","messageId":"20120610090516.GA30177@burratino","threadId":"30756","inReplyTo":"20111022001704.3115.87464.reportbug@hejmo","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-10T09:05:16Z","receivedAt":"2012-06-10T09:05:16Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Eduardo,\n\nEduardo Trápani wrote[1]:\n\n> git clone ssh://[2001:0:53aa:64c:1845:430c:4179:d71f]:3333/deponejo/unua\n>\n> Will try to connect to port 22 and not 3333. The port number seems to be\n> ignored.\n\nTrue. How about something like this (untested)?\n\nSigned-off-by: Jonathan Nieder \n---\n[1] http://bugs.debian.org/646178\n\n connect.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git i/connect.c w/connect.c\nindex 912cddee..7ec1b258 100644\n--- i/connect.c\n+++ w/connect.c\n@@ -494,11 +494,19 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \tif (host[0] == '[') {\n \t\tend = strchr(host + 1, ']');\n \t\tif (end) {\n-\t\t\tif (protocol != PROTO_GIT) {\n+\t\t\tif (protocol == PROTO_GIT)\n+\t\t\t\tend++;\n+\t\t\telse {\n \t\t\t\t*end = 0;\n \t\t\t\thost++;\n+\t\t\t\tend++;\n+\t\t\t\tif (!*end || *end == c)\n+\t\t\t\t\t; /* good */\n+\t\t\t\telse if (protocol == PROTO_SSH && c != ':' && *end == ':')\n+\t\t\t\t\tport = end + 1;\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"garbage after [] string in URL\");\n \t\t\t}\n-\t\t\tend++;\n \t\t} else\n \t\t\tend = host;\n \t} else\n@@ -535,7 +543,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t/*\n \t * Add support for ssh port: ssh://host.xy:/...\n \t */\n-\tif (protocol == PROTO_SSH && host != url)\n+\tif (protocol == PROTO_SSH && host != url && !port)\n \t\tport = get_port(host);\n \n \tif (protocol == PROTO_GIT) {\n"},{"id":"193481","messageId":"4FD78EA0.2090306@lsrfire.ath.cx","threadId":"30756","inReplyTo":"20120610090516.GA30177@burratino","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-06-12T18:46:56Z","receivedAt":"2012-06-12T18:46:56Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.06.2012 11:05, schrieb Jonathan Nieder:\n> Hi Eduardo,\n> \n> Eduardo Trápani wrote[1]:\n> \n>> git clone ssh://[2001:0:53aa:64c:1845:430c:4179:d71f]:3333/deponejo/unua\n>>\n>> Will try to connect to port 22 and not 3333. The port number seems to be\n>> ignored.\n> \n> True. How about something like this (untested)?\n> \n> Signed-off-by: Jonathan Nieder \n> ---\n> [1] http://bugs.debian.org/646178\n> \n> connect.c | 14 +++++++++++---\n> 1 file changed, 11 insertions(+), 3 deletions(-)\n\nHow about this instead? Except perhaps with a commit message that is vaguely\nunderstandable?\n\n-- >8 --\nIf we encounter an address part shaped like \"[HOST]:PORT\", we skip the opening\nbracket and replace the closing one with a NUL. The variable host then points\nto HOST and we've cut off the PORT part. Thus, when we go looking for it using\nhost a bit later, we can't find it. Start at end instead, which either points\nto the colon, if present, or is equal to host.\n\nSigned-off-by: Rene Scharfe \n---\nWe have similar code in daemon.c. Can we share more? And make it testable?\n\n connect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex 912cdde..41b7400 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -536,7 +536,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t * Add support for ssh port: ssh://host.xy:/...\n \t */\n \tif (protocol == PROTO_SSH && host != url)\n-\t\tport = get_port(host);\n+\t\tport = get_port(end);\n \n \tif (protocol == PROTO_GIT) {\n \t\t/* These underlying connection commands die() if they\n-- \n1.7.10.2\n"},{"id":"193499","messageId":"20120612202953.GG9764@burratino","threadId":"30756","inReplyTo":"4FD78EA0.2090306@lsrfire.ath.cx","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-12T20:29:53Z","receivedAt":"2012-06-12T20:29:53Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"René Scharfe wrote:\n\n> How about this instead?\n\nLooks good to me. Though it would be nice to see\n\n\tproto://[::1]trailing-nonsense/\n\ndiagnosed on top, too.\n"},{"id":"193503","messageId":"20120612210003.GA16816@burratino","threadId":"30756","inReplyTo":"20120612202953.GG9764@burratino","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-12T21:00:03Z","receivedAt":"2012-06-12T21:00:03Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> René Scharfe wrote:\n\n>> How about this instead?\n>\n> Looks good to me.\n\nOh, hold on a second. Won't this get confused by\n\n\tssh://[::1]/foo/bar/baz:80/qux\n\n?\n"},{"id":"193545","messageId":"4FD8C0F6.8060609@lsrfire.ath.cx","threadId":"30756","inReplyTo":"20120612210003.GA16816@burratino","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-06-13T16:33:58Z","receivedAt":"2012-06-13T16:33:58Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.06.2012 23:00, schrieb Jonathan Nieder:\n> Jonathan Nieder wrote:\n>> René Scharfe wrote:\n>\n>>> How about this instead?\n>>\n>> Looks good to me.\n>\n> Oh, hold on a second. Won't this get confused by\n>\n> \tssh://[::1]/foo/bar/baz:80/qux\n>\n> ?\n\nIt shouldn't, because the host part is NUL-terminated before get_port() \nis called. Let's see (with the patch):\n\n\t$ git clone ssh://[::1]/foo/bar/baz:80/qux\n\tCloning into 'qux'...\n\tssh: connect to host ::1 port 22: Connection refused\n\tfatal: The remote end hung up unexpectedly\n\nRené\n"},{"id":"193555","messageId":"7v4nqfi0vz.fsf@alter.siamese.dyndns.org","threadId":"30756","inReplyTo":"4FD8C0F6.8060609@lsrfire.ath.cx","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-13T17:21:04Z","receivedAt":"2012-06-13T17:21:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 12.06.2012 23:00, schrieb Jonathan Nieder:\n>> Jonathan Nieder wrote:\n>>> René Scharfe wrote:\n>>\n>>>> How about this instead?\n>>>\n>>> Looks good to me.\n>>\n>> Oh, hold on a second. Won't this get confused by\n>>\n>> \tssh://[::1]/foo/bar/baz:80/qux\n>>\n>> ?\n>\n> It shouldn't, because the host part is NUL-terminated before\n> get_port() is called. Let's see (with the patch):\n>\n> \t$ git clone ssh://[::1]/foo/bar/baz:80/qux\n> \tCloning into 'qux'...\n> \tssh: connect to host ::1 port 22: Connection refused\n> \tfatal: The remote end hung up unexpectedly\n>\n> René\n\nYeah, I was wondering how that would get confused myself. Jonathan,\nack again?\n"},{"id":"193586","messageId":"20120613194347.GA7967@burratino","threadId":"30756","inReplyTo":"7v4nqfi0vz.fsf@alter.siamese.dyndns.org","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-13T19:43:47Z","receivedAt":"2012-06-13T19:43:47Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Wed, Jun 13, 2012 at 10:21:04AM -0700, Junio C Hamano wrote:\n> René Scharfe writes:\n>> Am 12.06.2012 23:00, schrieb Jonathan Nieder:\n\n>>> Oh, hold on a second. Won't this get confused by\n>>>\n>>> \tssh://[::1]/foo/bar/baz:80/qux\n[...]\n>> It shouldn't, because the host part is NUL-terminated before\n>> get_port() is called. Let's see (with the patch):\n[...]\n> Yeah, I was wondering how that would get confused myself. Jonathan,\n> ack again?\n\nYeah. I had missed that when proto == PROTO_SSH that means the proto\n!= PROTO_LOCAL branch has been taken and the port is NUL-terminated.\n\nSo\n\nReviewed-by: Jonathan Nieder \n\nIt seems like a good fix given the current code structure. Sorry for\nthe false alarm.\n"},{"id":"193588","messageId":"7vbokngehi.fsf@alter.siamese.dyndns.org","threadId":"30756","inReplyTo":"20120613194347.GA7967@burratino","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-13T20:10:17Z","receivedAt":"2012-06-13T20:10:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> On Wed, Jun 13, 2012 at 10:21:04AM -0700, Junio C Hamano wrote:\n>> René Scharfe writes:\n>>> Am 12.06.2012 23:00, schrieb Jonathan Nieder:\n>\n>>>> Oh, hold on a second. Won't this get confused by\n>>>>\n>>>> \tssh://[::1]/foo/bar/baz:80/qux\n> [...]\n>>> It shouldn't, because the host part is NUL-terminated before\n>>> get_port() is called. Let's see (with the patch):\n> [...]\n>> Yeah, I was wondering how that would get confused myself. Jonathan,\n>> ack again?\n>\n> Yeah. I had missed that when proto == PROTO_SSH that means the proto\n> != PROTO_LOCAL branch has been taken and the port is NUL-terminated.\n>\n> So\n>\n> Reviewed-by: Jonathan Nieder \n>\n> It seems like a good fix given the current code structure. Sorry for\n> the false alarm.\n\nThanks.\n\nBy the way, it seems that Debian/Ubuntu are still carrying a bit\nmore changes and rewrites in the connection code as local\npatches. Mind upstreaming them for the next cycle?\n"},{"id":"193590","messageId":"20120613203959.GD7967@burratino","threadId":"30756","inReplyTo":"7vbokngehi.fsf@alter.siamese.dyndns.org","subject":"Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-13T20:39:59Z","receivedAt":"2012-06-13T20:39:59Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> By the way, it seems that Debian/Ubuntu are still carrying a bit\n> more changes and rewrites in the connection code as local\n> patches. Mind upstreaming them for the next cycle?\n\nSure --- based on Erik's advice I think the right thing to do is to\nmake a simple compatibility wrapper that acts like normal getaddrinfo,\nso no one has to learn a new API. It's a little more expensive (some\nmalloc() / free() noise) but probably not enough so to be noticeable.\n\nDebian has these patches because the SRV patch is on top of them.\n"}]}