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

9 messages from 2012-06-10 to 2012-06-13. Participants: Jonathan Nieder, René Scharfe, Junio C Hamano.
Thread: https://gitlist.dev/t/30756

## Jonathan Nieder, 2012-06-10 09:05

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <20120610090516.GA30177@burratino>
URL: https://gitlist.dev/e/20120610090516.GA30177%40burratino
In-Reply-To: <20111022001704.3115.87464.reportbug@hejmo>

```
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, 2012-06-12 18:46

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <4FD78EA0.2090306@lsrfire.ath.cx>
URL: https://gitlist.dev/e/4FD78EA0.2090306%40lsrfire.ath.cx
In-Reply-To: <20120610090516.GA30177@burratino>

```
Am 10.06.2012 11:05, schrieb Jonathan Nieder:
> 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

```

## Jonathan Nieder, 2012-06-12 20:29

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <20120612202953.GG9764@burratino>
URL: https://gitlist.dev/e/20120612202953.GG9764%40burratino
In-Reply-To: <4FD78EA0.2090306@lsrfire.ath.cx>

```
René Scharfe wrote:

> How about this instead?

Looks good to me.  Though it would be nice to see

	proto://[::1]trailing-nonsense/

diagnosed on top, too.

```

## Jonathan Nieder, 2012-06-12 21:00

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <20120612210003.GA16816@burratino>
URL: https://gitlist.dev/e/20120612210003.GA16816%40burratino
In-Reply-To: <20120612202953.GG9764@burratino>

```
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

?

```

## René Scharfe, 2012-06-13 16:33

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <4FD8C0F6.8060609@lsrfire.ath.cx>
URL: https://gitlist.dev/e/4FD8C0F6.8060609%40lsrfire.ath.cx
In-Reply-To: <20120612210003.GA16816@burratino>

```
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é

```

## Junio C Hamano, 2012-06-13 17:21

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <7v4nqfi0vz.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v4nqfi0vz.fsf%40alter.siamese.dyndns.org
In-Reply-To: <4FD8C0F6.8060609@lsrfire.ath.cx>

```
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> 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, 2012-06-13 19:43

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <20120613194347.GA7967@burratino>
URL: https://gitlist.dev/e/20120613194347.GA7967%40burratino
In-Reply-To: <7v4nqfi0vz.fsf@alter.siamese.dyndns.org>

```
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, 2012-06-13 20:10

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <7vbokngehi.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vbokngehi.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20120613194347.GA7967@burratino>

```
Jonathan Nieder <jrnieder@gmail.com> writes:

> 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, 2012-06-13 20:39

Subject: Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port
Message-ID: <20120613203959.GD7967@burratino>
URL: https://gitlist.dev/e/20120613203959.GD7967%40burratino
In-Reply-To: <7vbokngehi.fsf@alter.siamese.dyndns.org>

```
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.

```