[BUG] cdpath=(.) breaks cd from / on Cygwin

[BUG] cdpath=(.) breaks cd from / on Cygwin

[Andrew Janke]

Hi, zsh workers,

This is my first time sending to this list. Sorry if I've got any of the 
conventions wrong.

I think I've found a bug in zsh's handling of path normalization on 
Cygwin when there's an explicit '.' in $cdpath. When $cdpath contains ., 
and your pwd is /, doing a cd using a relative path to an immediate 
subdir like `cd bin` or `cd tmp` errors out, and slowly.

Reproduction
====================

In zsh on Cygwin, do this:

cd /
cdpath=(.)
cd bin
cd lib
cdpath=()
cd bin

Symptoms
====================

On my system (Windows 7, zsh 5.0.6 (x86_64-unknown-cygwin), running 
inside MinTTY, inside a vmware VM on Mac OS X 10.9 on a MacBook Pro), if 
I have '.' in $cdpath and try to do a cd from / to an immediate subdir, 
the command sits there for up to several seconds and then errors out 
with a "no such dir" error.

Here's an example run. This is with an empty ~/.zshrc file so I'd have a 
default zsh configuration. gwydion is my win7 machine's hostname.

gwydion% cd /
gwydion% cdpath=(.)
gwydion% cd bin
cd: no such file or directory: bin
gwydion% cd lib
cd: no such file or directory: lib
gwydion% pwd
/
gwydion% cdpath=()
gwydion% cd bin
gwydion% pwd
/bin
gwydion%

I would expect the behavior with `cdpath=(.)` to be identical to the 
behavior with `cdpath=()`, and to have all these cd commands succeed.


Analysis
===================

On OS X 10.9 and Debian Linux 7, doing a cd from / with cdpath=(.) works 
fine for me, so I think this is Cygwin-specific.

In Src/builtin.c, function cd_try_chdir(), lines 1080-1093, there's some 
special case handling for Cygwin and the prefix '/'. But when the prefix 
is '.', it goes down a different code path. It constructs the path 
starting from pwd (which is '/' in this case), and then goes through the 
path normalization which elides '.' segments. But it doesn't have a 
special case check for Cygwin to check for leading '/' again. And I 
think the elision of the first '.' segment will result in a computed 
path starting with '//' but not further escaped.

I think what's happening is in this case, the path is ending up with 
'//' at the beginning and it's treating it as a UNC path and ends up 
looking for a machine named 'bin' or 'lib' or whatever.

I'm not good enough with C and path normalization to try my hand at 
making a patch for this.

References
===================

I found this while working with oh-my-zsh, where another Cygwin user ran 
in to this behavior. https://github.com/robbyrussell/oh-my-zsh/issues/3550

(When I tested this, I had oh-my-zsh disabled, and an empty .zshrc, so I 
don't think oh-my-zsh interaction is relevant.)

Cheers,
Andrew Janke

Re: [BUG] cdpath=(.) breaks cd from / on Cygwin

[Bart Schaefer]

On Feb 15,  3:18pm, Andrew Janke wrote:
}
} In Src/builtin.c, function cd_try_chdir(), lines 1080-1093, there's some 
} special case handling for Cygwin and the prefix '/'. But when the prefix 
} is '.', it goes down a different code path. It constructs the path 
} starting from pwd (which is '/' in this case), and then goes through the 
} path normalization which elides '.' segments. But it doesn't have a 
} special case check for Cygwin to check for leading '/' again. And I 
} think the elision of the first '.' segment will result in a computed 
} path starting with '//' but not further escaped.

That's a pretty close analysis.  The real problem is that when pwd is
"/", cd_try_chdir() unnecessarily inserts yet another "/" into the
path before appending a cdpath segment (the "pfix").  So the path has
already become "//./" before normalization even begins.  This is a
waste of effort when pwd is "/" on any platform.

I'm not sure why we bother compiling different branches for cygwin and
otherwise in cd_try_chdir().  Doesn't appear the cygwin variation would
be wrong anywhere else.  But I won't mess with that here.

Try this patch:


diff --git a/Src/builtin.c b/Src/builtin.c
index c4e4b94..614b17d 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -1093,9 +1093,11 @@ cd_try_chdir(char *pfix, char *dest, int hard)
 	} else {
 	    int pfl = strlen(pfix);
 	    dlen = strlen(pwd);
-
+	    if (dlen == 1 && *pwd == '/')
+		dlen = 0;
 	    buf = zalloc(dlen + pfl + strlen(dest) + 3);
-	    strcpy(buf, pwd);
+	    if (dlen)
+		strcpy(buf, pwd);
 	    buf[dlen] = '/';
 	    strcpy(buf + dlen + 1, pfix);
 	    buf[dlen + 1 + pfl] = '/';

Re: [BUG] cdpath=(.) breaks cd from / on Cygwin

[Andrew Janke]

Looks good to me. When I build with this patch, cd from / on Cygwin 
works as expected, even with an explicit '.' in $cdpath.

Thanks!

Andrew


On 2/15/15 5:57 PM, Bart Schaefer wrote:
> On Feb 15,  3:18pm, Andrew Janke wrote:
> }
> } In Src/builtin.c, function cd_try_chdir(), lines 1080-1093, there's some
> } special case handling for Cygwin and the prefix '/'. But when the prefix
> } is '.', it goes down a different code path. It constructs the path
> } starting from pwd (which is '/' in this case), and then goes through the
> } path normalization which elides '.' segments. But it doesn't have a
> } special case check for Cygwin to check for leading '/' again. And I
> } think the elision of the first '.' segment will result in a computed
> } path starting with '//' but not further escaped.
>
> That's a pretty close analysis.  The real problem is that when pwd is
> "/", cd_try_chdir() unnecessarily inserts yet another "/" into the
> path before appending a cdpath segment (the "pfix").  So the path has
> already become "//./" before normalization even begins.  This is a
> waste of effort when pwd is "/" on any platform.
>
> I'm not sure why we bother compiling different branches for cygwin and
> otherwise in cd_try_chdir().  Doesn't appear the cygwin variation would
> be wrong anywhere else.  But I won't mess with that here.
>
> Try this patch:
>
>
> diff --git a/Src/builtin.c b/Src/builtin.c
> index c4e4b94..614b17d 100644
> --- a/Src/builtin.c
> +++ b/Src/builtin.c
> @@ -1093,9 +1093,11 @@ cd_try_chdir(char *pfix, char *dest, int hard)
>   	} else {
>   	    int pfl = strlen(pfix);
>   	    dlen = strlen(pwd);
> -
> +	    if (dlen == 1 && *pwd == '/')
> +		dlen = 0;
>   	    buf = zalloc(dlen + pfl + strlen(dest) + 3);
> -	    strcpy(buf, pwd);
> +	    if (dlen)
> +		strcpy(buf, pwd);
>   	    buf[dlen] = '/';
>   	    strcpy(buf + dlen + 1, pfix);
>   	    buf[dlen + 1 + pfl] = '/';