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

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

[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(-)

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) {

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

[René Scharfe]

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

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

[Jonathan Nieder]

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.

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

[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

?

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

[René Scharfe]

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é

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

[Junio C Hamano]

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?

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

[Jonathan Nieder]

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.

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

[Junio C Hamano]

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?

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

[Jonathan Nieder]

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.