* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port [not found] <20111022001704.3115.87464.reportbug@hejmo> @ 2012-06-10 9:05 ` Jonathan Nieder 2012-06-12 18:46 ` René Scharfe 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Nieder @ 2012-06-10 9:05 UTC (permalink / raw) To: Eduardo Trápani; +Cc: git, René Scharfe, YOSHIFUJI Hideaki 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) { ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port 2012-06-10 9:05 ` git: Wrong parsing of ssh urls with IPv6 literals ignores port Jonathan Nieder @ 2012-06-12 18:46 ` René Scharfe 2012-06-12 20:29 ` Jonathan Nieder 0 siblings, 1 reply; 9+ messages in thread From: René Scharfe @ 2012-06-12 18:46 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Eduardo Trápani, git, YOSHIFUJI Hideaki 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port 2012-06-12 18:46 ` René Scharfe @ 2012-06-12 20:29 ` Jonathan Nieder 2012-06-12 21:00 ` Jonathan Nieder 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Nieder @ 2012-06-12 20:29 UTC (permalink / raw) To: René Scharfe; +Cc: Eduardo Trápani, git, YOSHIFUJI Hideaki 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port 2012-06-12 20:29 ` Jonathan Nieder @ 2012-06-12 21:00 ` Jonathan Nieder 2012-06-13 16:33 ` René Scharfe 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Nieder @ 2012-06-12 21:00 UTC (permalink / raw) To: René Scharfe; +Cc: Eduardo Trápani, git, YOSHIFUJI Hideaki 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 ? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port 2012-06-12 21:00 ` Jonathan Nieder @ 2012-06-13 16:33 ` René Scharfe 2012-06-13 17:21 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: René Scharfe @ 2012-06-13 16:33 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Eduardo Trápani, git, YOSHIFUJI Hideaki 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é ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port 2012-06-13 16:33 ` René Scharfe @ 2012-06-13 17:21 ` Junio C Hamano 2012-06-13 19:43 ` Jonathan Nieder 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2012-06-13 17:21 UTC (permalink / raw) To: René Scharfe Cc: Jonathan Nieder, Eduardo Trápani, git, YOSHIFUJI Hideaki 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port 2012-06-13 17:21 ` Junio C Hamano @ 2012-06-13 19:43 ` Jonathan Nieder 2012-06-13 20:10 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Nieder @ 2012-06-13 19:43 UTC (permalink / raw) To: Junio C Hamano Cc: René Scharfe, Eduardo Trápani, git, YOSHIFUJI Hideaki 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port 2012-06-13 19:43 ` Jonathan Nieder @ 2012-06-13 20:10 ` Junio C Hamano 2012-06-13 20:39 ` Jonathan Nieder 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2012-06-13 20:10 UTC (permalink / raw) To: Jonathan Nieder Cc: René Scharfe, Eduardo Trápani, git, YOSHIFUJI Hideaki 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: git: Wrong parsing of ssh urls with IPv6 literals ignores port 2012-06-13 20:10 ` Junio C Hamano @ 2012-06-13 20:39 ` Jonathan Nieder 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Nieder @ 2012-06-13 20:39 UTC (permalink / raw) To: Junio C Hamano Cc: René Scharfe, Eduardo Trápani, git, YOSHIFUJI Hideaki 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-06-13 20:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20111022001704.3115.87464.reportbug@hejmo>
2012-06-10 9:05 ` git: Wrong parsing of ssh urls with IPv6 literals ignores port Jonathan Nieder
2012-06-12 18:46 ` René Scharfe
2012-06-12 20:29 ` Jonathan Nieder
2012-06-12 21:00 ` Jonathan Nieder
2012-06-13 16:33 ` René Scharfe
2012-06-13 17:21 ` Junio C Hamano
2012-06-13 19:43 ` Jonathan Nieder
2012-06-13 20:10 ` Junio C Hamano
2012-06-13 20:39 ` Jonathan Nieder
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).