threads / discuss / 30756

Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port

## tl;dr

9 messages between Jun 10, 2012 and Jun 13, 2012.

replies: 8people: 3as markdown or json

Jonathan Nieder· Jun 10, 2012, 09:05 UTC · lore
Hi Eduardo,
Eduardo Trápani wrote[1]:
> git clone ssh://[2001:0:53aa:64c:1845:430c:4179:d71f]:3333/deponejo/unua
>
> Will try to connect to port 22 and not 3333.  The port number seems to be
> ignored.
True.  How about something like this (untested)?
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
[1] http://bugs.debian.org/646178
 connect.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git i/connect.c w/connect.c
index 912cddee..7ec1b258 100644
--- i/connect.c
+++ w/connect.c
@@ -494,11 +494,19 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	if (host[0] == '[') {
 		end = strchr(host + 1, ']');
 		if (end) {
-			if (protocol != PROTO_GIT) {
+			if (protocol == PROTO_GIT)
+				end++;
+			else {
 				*end = 0;
 				host++;
+				end++;
+				if (!*end || *end == c)
+					; /* good */
+				else if (protocol == PROTO_SSH && c != ':' && *end == ':')
+					port = end + 1;
+				else
+					die("garbage after [] string in URL");
 			}
-			end++;
 		} else
 			end = host;
 	} else
@@ -535,7 +543,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	/*
 	 * Add support for ssh port: ssh://host.xy:<port>/...
 	 */
-	if (protocol == PROTO_SSH && host != url)
+	if (protocol == PROTO_SSH && host != url && !port)
 		port = get_port(host);
 
 	if (protocol == PROTO_GIT) {
René Scharfe· Jun 12, 2012, 18:46 UTC · re: Jonathan Nieder · lore
Am 10.06.2012 11:05, schrieb Jonathan Nieder:
Show 17 quoted lines
> Hi Eduardo,
> 
> Eduardo Trápani wrote[1]:
> 
>> git clone ssh://[2001:0:53aa:64c:1845:430c:4179:d71f]:3333/deponejo/unua
>>
>> Will try to connect to port 22 and not 3333.  The port number seems to be
>> ignored.
> 
> True.  How about something like this (untested)?
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> [1] http://bugs.debian.org/646178
> 
>   connect.c |   14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)

How about this instead? Except perhaps with a commit message that is vaguely understandable?

-- >8 -- If we encounter an address part shaped like "[HOST]:PORT", we skip the opening bracket and replace the closing one with a NUL. The variable host then points to HOST and we've cut off the PORT part. Thus, when we go looking for it using host a bit later, we can't find it. Start at end instead, which either points to the colon, if present, or is equal to host.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
We have similar code in daemon.c.  Can we share more?  And make it testable?
 connect.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/connect.c b/connect.c
index 912cdde..41b7400 100644
--- a/connect.c
+++ b/connect.c
@@ -536,7 +536,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	 * Add support for ssh port: ssh://host.xy:<port>/...
 	 */
 	if (protocol == PROTO_SSH && host != url)
-		port = get_port(host);
+		port = get_port(end);
 
 	if (protocol == PROTO_GIT) {
 		/* These underlying connection commands die() if they
-- 
1.7.10.2
René Scharfe· Jun 13, 2012, 16:33 UTC · re: Jonathan Nieder · lore
Am 12.06.2012 23:00, schrieb Jonathan Nieder:
Show 12 quoted lines
> Jonathan Nieder wrote:
>> René Scharfe wrote:
>
>>> How about this instead?
>>
>> Looks good to me.
>
> Oh, hold on a second.  Won't this get confused by
>
> 	ssh://[::1]/foo/bar/baz:80/qux
>
> ?

It shouldn't, because the host part is NUL-terminated before get_port() is called. Let's see (with the patch):

	$ git clone ssh://[::1]/foo/bar/baz:80/qux
	Cloning into 'qux'...
	ssh: connect to host ::1 port 22: Connection refused
	fatal: The remote end hung up unexpectedly
René
Junio C Hamano· Jun 13, 2012, 17:21 UTC · re: René Scharfe · lore
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 23 quoted lines
> Am 12.06.2012 23:00, schrieb Jonathan Nieder:
>> Jonathan Nieder wrote:
>>> René Scharfe wrote:
>>
>>>> How about this instead?
>>>
>>> Looks good to me.
>>
>> Oh, hold on a second.  Won't this get confused by
>>
>> 	ssh://[::1]/foo/bar/baz:80/qux
>>
>> ?
>
> It shouldn't, because the host part is NUL-terminated before
> get_port() is called.  Let's see (with the patch):
>
> 	$ git clone ssh://[::1]/foo/bar/baz:80/qux
> 	Cloning into 'qux'...
> 	ssh: connect to host ::1 port 22: Connection refused
> 	fatal: The remote end hung up unexpectedly
>
> René

Yeah, I was wondering how that would get confused myself. Jonathan, ack again?

Jonathan Nieder· Jun 13, 2012, 19:43 UTC · re: Junio C Hamano · lore
On Wed, Jun 13, 2012 at 10:21:04AM -0700, Junio C Hamano wrote:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>> Am 12.06.2012 23:00, schrieb Jonathan Nieder:
>>> Oh, hold on a second.  Won't this get confused by
>>>
>>> 	ssh://[::1]/foo/bar/baz:80/qux
[...]
>> It shouldn't, because the host part is NUL-terminated before
>> get_port() is called.  Let's see (with the patch):
[...]
> Yeah, I was wondering how that would get confused myself.  Jonathan,
> ack again?

Yeah. I had missed that when proto == PROTO_SSH that means the proto != PROTO_LOCAL branch has been taken and the port is NUL-terminated.

So
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

It seems like a good fix given the current code structure. Sorry for the false alarm.

Junio C Hamano· Jun 13, 2012, 20:10 UTC · re: Jonathan Nieder · lore
Jonathan Nieder <jrnieder@gmail.com> writes:
Show 23 quoted lines
> On Wed, Jun 13, 2012 at 10:21:04AM -0700, Junio C Hamano wrote:
>> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>>> Am 12.06.2012 23:00, schrieb Jonathan Nieder:
>
>>>> Oh, hold on a second.  Won't this get confused by
>>>>
>>>> 	ssh://[::1]/foo/bar/baz:80/qux
> [...]
>>> It shouldn't, because the host part is NUL-terminated before
>>> get_port() is called.  Let's see (with the patch):
> [...]
>> Yeah, I was wondering how that would get confused myself.  Jonathan,
>> ack again?
>
> Yeah.  I had missed that when proto == PROTO_SSH that means the proto
> != PROTO_LOCAL branch has been taken and the port is NUL-terminated.
>
> So
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> It seems like a good fix given the current code structure.  Sorry for
> the false alarm.
Thanks.

By the way, it seems that Debian/Ubuntu are still carrying a bit more changes and rewrites in the connection code as local patches. Mind upstreaming them for the next cycle?

Jonathan Nieder· Jun 13, 2012, 20:39 UTC · re: Junio C Hamano · lore
Junio C Hamano wrote:
> By the way, it seems that Debian/Ubuntu are still carrying a bit
> more changes and rewrites in the connection code as local
> patches. Mind upstreaming them for the next cycle?

Sure --- based on Erik's advice I think the right thing to do is to make a simple compatibility wrapper that acts like normal getaddrinfo, so no one has to learn a new API. It's a little more expensive (some malloc() / free() noise) but probably not enough so to be noticeable.

Debian has these patches because the SRV patch is on top of them.

← back to recent threads