cd -s symlink hangs (sometimes?)

cd -s symlink hangs (sometimes?)

[Mikael Magnusson]

Hi,

zsh -f
% mkdir a
% ln -s a b
% cd -s b
cd: not a directory: b
% cd -s b
# kill -9 in another terminal
zsh: killed     zsh -f

strace output of a rerun where it happened the first time ($PWD is /tmp/x)
write(10, "\10cd"..., 3)                = 3
read(10, " "..., 1)                     = 1
write(10, " "..., 1)                    = 1
read(10, "-"..., 1)                     = 1
write(10, "-"..., 1)                    = 1
read(10, "s"..., 1)                     = 1
write(10, "s"..., 1)                    = 1
read(10, " "..., 1)                     = 1
write(10, " "..., 1)                    = 1
read(10, "b"..., 1)                     = 1
write(10, "b"..., 1)                    = 1
read(10, "\n"..., 1)                    = 1
write(10, "\r\n"..., 2)                 = 2
alarm(0)                                = 0
ioctl(10, SNDCTL_TMR_STOP or TCSETSW, {B38400 opost isig icanon echo ...}) = 0
ioctl(10, SNDCTL_TMR_STOP or TCSETSW, {B38400 opost isig icanon echo ...}) = 0
time(NULL)                              = 1237583357
rt_sigprocmask(SIG_BLOCK, [CHLD], [], 8) = 0
open(".", O_RDONLY|O_NOCTTY|O_LARGEFILE) = 3
chdir("/")                              = 0
lstat64("tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=1040, ...}) = 0
chdir("tmp")                            = 0
lstat64(".", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=1040, ...}) = 0
lstat64("x", {st_mode=S_IFDIR|0755, st_size=80, ...}) = 0
chdir("x")                              = 0
lstat64(".", {st_mode=S_IFDIR|0755, st_size=80, ...}) = 0
lstat64("b", {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
fchdir(3)                               = 0
close(3)                                = 0
stat64(".", {st_mode=S_IFDIR|0755, st_size=80, ...}) = 0
lstat64("b", {st_mode=S_IFLNK|0777, st_size=1, ...}) = 0
chdir("../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../..")
= 0
...
[chdir repeated infinitely]

-- 
Mikael Magnusson

Re: cd -s symlink hangs (sometimes?)

[Peter Stephenson]

On Fri, 20 Mar 2009 22:12:30 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> zsh -f
> % mkdir a
> % ln -s a b
> % cd -s b
> cd: not a directory: b
> % cd -s b
> # kill -9 in another terminal
> zsh: killed     zsh -f

The following is reproducible (note I didn't actually create the
directory a):

% zsh -f
% cd ~
% pwd
/home/pws
% rm -f a b
% ln -s a b
% cd -s b
cd: not a directory: b
% /bin/pwd
/

The -s does seem to be the crucial factor---which figures, since cd is
very heavily used so this would otherwise have turned up long ago.  This
is related to the lchdir() / zgetdir() code I was looking at the other
day and thinking was rather hairy (but the bug goes back to the 4.2 line
and presumably much further).

I can't be sure the fix below doesn't introduce another subtle problem,
though I think the test "d->dirfd < 0" should protect us from the most
likely side effects.  The underlying problem here is that the code for
HAVE_FCHDIR appears to have been kludged in without fixing the code
structure to do it properly.  I really don't like that complicated
expression I've moved down to fix the problem: in case we couldn't open
the current directory, investigate what directory it is, check it's not
absolute and if so open the parent directory instead... what is that
palaver supposed to do?

Still, I suspect it's better with rather than without the change.

Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.215
diff -u -r1.215 utils.c
--- Src/utils.c	20 Mar 2009 20:52:13 -0000	1.215
+++ Src/utils.c	20 Mar 2009 22:41:43 -0000
@@ -5388,11 +5388,7 @@
     if (*path == '/') {
 #endif
 	level = -1;
-#ifdef HAVE_FCHDIR
-	if (d->dirfd < 0 && (d->dirfd = open(".", O_RDONLY | O_NOCTTY)) < 0 &&
-	    zgetdir(d) && *d->dirname != '/')
-	    d->dirfd = open("..", O_RDONLY | O_NOCTTY);
-#else
+#ifndef HAVE_FCHDIR
 	if (!d->dirname)
 	    zgetdir(d);
 #endif
@@ -5404,6 +5400,11 @@
 	    d->ino = st1.st_ino;
 	}
     }
+#ifdef HAVE_FCHDIR
+    if (d->dirfd < 0 && (d->dirfd = open(".", O_RDONLY | O_NOCTTY)) < 0 &&
+	zgetdir(d) && *d->dirname != '/')
+	d->dirfd = open("..", O_RDONLY | O_NOCTTY);
+#endif
 
 #ifdef HAVE_LSTAT
     if (!hard)
Index: Test/B01cd.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/B01cd.ztst,v
retrieving revision 1.3
diff -u -r1.3 B01cd.ztst
--- Test/B01cd.ztst	5 Aug 2002 13:10:02 -0000	1.3
+++ Test/B01cd.ztst	20 Mar 2009 22:41:43 -0000
@@ -109,6 +109,14 @@
 >$mydir/cdtst.tmp/real
 >$mydir/cdtst.tmp/real
 
+ ln -s nonexistent link_to_nonexistent
+ pwd1=$(pwd -P)
+ cd -s link_to_nonexistent
+ pwd2=$(pwd -P)
+ [[ $pwd1 = $pwd2 ]] || print "Ooops, changed to directory '$pwd2'"
+0:
+?(eval):cd:3: not a directory: link_to_nonexistent
+
 %clean
 # This optional section cleans up after the test, if necessary,
 # e.g. killing processes etc.  This is in addition to the removal of *.tmp


-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: cd -s symlink hangs (sometimes?)

[Mikael Magnusson]

2009/3/20 Peter Stephenson <p.w.stephenson@ntlworld.com>:
> On Fri, 20 Mar 2009 22:12:30 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> zsh -f
>> % mkdir a
>> % ln -s a b
>> % cd -s b
>> cd: not a directory: b
>> % cd -s b
>> # kill -9 in another terminal
>> zsh: killed     zsh -f
>
> The following is reproducible (note I didn't actually create the
> directory a):
>
> % zsh -f
> % cd ~
> % pwd
> /home/pws
> % rm -f a b
> % ln -s a b
> % cd -s b
> cd: not a directory: b
> % /bin/pwd
> /

At first I thought you misunderstood my problem, since the above seems
like a different problem, but the patch does fix my infinite loop.

Just for fun I tried mkdir a; cd a; rmdir ../a; cd -s ../symlink, and
it didn't break either.

[5 minutes later]

For some more fun I tried this, and it does still break:
mkdir a
ln -s a b
cd a
chmod -x .
cd -s $PWD:h/b
#infinite loop here still
chdir("../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../..")
= 0

I think this is related to .. not being accessible when . is -x? I
didn't study the code very closely (ie, I only read the patch
context). I should also note that this is not something I tend to do
very often, I was just thinking up ways in which the code might still
fail.

-- 
Mikael Magnusson

Re: cd -s symlink hangs (sometimes?)

[Peter Stephenson]

On Sat, 21 Mar 2009 00:15:24 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> For some more fun I tried this, and it does still break:
> mkdir a
> ln -s a b
> cd a
> chmod -x .
> cd -s $PWD:h/b
> #infinite loop here still

This is basically the same problem.  I'm not sure why when the directory
is restored wrongly and you end up in "/" you are seeing this infinite
loop and I'm not, but it may just be a side effect.

The problem is still the code I was scratching my head over before:

#ifdef HAVE_FCHDIR
    if (d->dirfd < 0 && (d->dirfd = open(".", O_RDONLY | O_NOCTTY)) < 0 &&
	zgetdir(d) && *d->dirname != '/')
	d->dirfd = open("..", O_RDONLY | O_NOCTTY);
#endif

It's now being executed, but we can't open "." any more.  So it's
failing to save a fchdir-able directory for restoring, so after the "cd
-s" fails we end up still in / again.

I'm not sure if there's a full fix for this.  It can probably be handled
better than it is at the moment, but apart from warning the user and
making sure the shell knows what directory it's actually in I'm
don't really know how.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: cd -s symlink hangs (sometimes?)

[Mikael Magnusson]

2009/3/22 Peter Stephenson <p.w.stephenson@ntlworld.com>:
> On Sat, 21 Mar 2009 00:15:24 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> For some more fun I tried this, and it does still break:
>> mkdir a
>> ln -s a b
>> cd a
>> chmod -x .
>> cd -s $PWD:h/b
>> #infinite loop here still
>
> This is basically the same problem.  I'm not sure why when the directory
> is restored wrongly and you end up in "/" you are seeing this infinite
> loop and I'm not, but it may just be a side effect.
>
> The problem is still the code I was scratching my head over before:
>
> #ifdef HAVE_FCHDIR
>    if (d->dirfd < 0 && (d->dirfd = open(".", O_RDONLY | O_NOCTTY)) < 0 &&
>        zgetdir(d) && *d->dirname != '/')
>        d->dirfd = open("..", O_RDONLY | O_NOCTTY);
> #endif
>
> It's now being executed, but we can't open "." any more.  So it's
> failing to save a fchdir-able directory for restoring, so after the "cd
> -s" fails we end up still in / again.
>
> I'm not sure if there's a full fix for this.  It can probably be handled
> better than it is at the moment, but apart from warning the user and
> making sure the shell knows what directory it's actually in I'm
> don't really know how.

I just noticed (ls -l /proc/$$/fd) that with this last commit, zsh now keeps
all my old directories open, even when I don't use -s or anything special.

zsh -f
% cd /var
% cd log
% cd /var/tmp
% cd /
% cd tmp
% ls -g /proc/$$/fd
total 0
[...]
lr-x------ 1 users 64 2009-03-23 00:03 4 -> /tmp #this is from cwd in
the first cd,
                                                 #not the final cd tmp
lr-x------ 1 users 64 2009-03-23 00:03 5 -> /var
lr-x------ 1 users 64 2009-03-23 00:03 6 -> /var/log
lr-x------ 1 users 64 2009-03-23 00:03 7 -> /var/tmp
lr-x------ 1 users 64 2009-03-23 00:03 8 -> /


-- 
Mikael Magnusson

Re: cd -s symlink hangs (sometimes?)

[Peter Stephenson]

On Mon, 23 Mar 2009 00:05:10 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> I just noticed (ls -l /proc/$$/fd) that with this last commit, zsh now keeps
> all my old directories open, even when I don't use -s or anything special.

Lucky you noticed.

This is more overload-things-in-a-complicated-fashion-without-documentation
fallout.

The following should be safer, although the bug you noticed was, I think,
just because we didn't need to open the directory at all if allowed to
follow links because zchdir doesn't use restoredir().  So close_dir
may be redundant, but it's hard to tell.

Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.216
diff -u -r1.216 utils.c
--- Src/utils.c	20 Mar 2009 22:52:56 -0000	1.216
+++ Src/utils.c	23 Mar 2009 10:46:11 -0000
@@ -5374,6 +5374,9 @@
     int err;
     struct stat st2;
 #endif
+#ifdef HAVE_FCHDIR
+    int close_dir = 0;
+#endif
 
     if (!d) {
 	ds.ino = ds.dev = 0;
@@ -5400,11 +5403,6 @@
 	    d->ino = st1.st_ino;
 	}
     }
-#ifdef HAVE_FCHDIR
-    if (d->dirfd < 0 && (d->dirfd = open(".", O_RDONLY | O_NOCTTY)) < 0 &&
-	zgetdir(d) && *d->dirname != '/')
-	d->dirfd = open("..", O_RDONLY | O_NOCTTY);
-#endif
 
 #ifdef HAVE_LSTAT
     if (!hard)
@@ -5420,7 +5418,16 @@
 	}
 	return zchdir((char *) path);
     }
+
 #ifdef HAVE_LSTAT
+#ifdef HAVE_FCHDIR
+    if (d->dirfd < 0) {
+	close_dir = 1;
+        if ((d->dirfd = open(".", O_RDONLY | O_NOCTTY) < 0) &&
+	    zgetdir(d) && *d->dirname != '/')
+	    d->dirfd = open("..", O_RDONLY | O_NOCTTY);
+    }
+#endif
     if (*path == '/')
 	if (chdir("/") < 0)
 	    zwarn("failed to chdir(/): %e", errno);
@@ -5428,12 +5435,16 @@
 	while(*path == '/')
 	    path++;
 	if(!*path) {
-	    if (d == &ds) {
+	    if (d == &ds)
 		zsfree(ds.dirname);
-		if (ds.dirfd >=0)
-		    close(ds.dirfd);
-	    } else
+	    else
 		d->level = level;
+#ifdef HAVE_FCHDIR
+	    if (d->dirfd >=0 && close_dir) {
+		close(d->dirfd);
+		d->dirfd = -1;
+	    }
+#endif
 	    return 0;
 	}
 	for(pptr = path; *++pptr && *pptr != '/'; ) ;
@@ -5468,19 +5479,25 @@
 	}
     }
     if (restoredir(d)) {
-	if (d == &ds) {
+	if (d == &ds)
 	    zsfree(ds.dirname);
-	    if (ds.dirfd >=0)
-		close(ds.dirfd);
+#ifdef HAVE_FCHDIR
+	if (d->dirfd >=0 && close_dir) {
+	    close(d->dirfd);
+	    d->dirfd = -1;
 	}
+#endif
 	errno = err;
 	return -2;
     }
-    if (d == &ds) {
+    if (d == &ds)
 	zsfree(ds.dirname);
-	if (ds.dirfd >=0)
-	    close(ds.dirfd);
+#ifdef HAVE_FCHDIR
+    if (d->dirfd >=0 && close_dir) {
+	close(d->dirfd);
+	d->dirfd = -1;
     }
+#endif
     errno = err;
     return -1;
 #endif /* HAVE_LSTAT */


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: cd -s symlink hangs (sometimes?)

[Mikael Magnusson]

2009/3/23 Peter Stephenson <pws@csr.com>:
> On Mon, 23 Mar 2009 00:05:10 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> I just noticed (ls -l /proc/$$/fd) that with this last commit, zsh now keeps
>> all my old directories open, even when I don't use -s or anything special.
>
> Lucky you noticed.

I started noticing I couldn't unmount my ipod and memory cards, it
took a few tries until I made the connection with the change in zsh
though.

Trying the patch now and it does stop the leak... but you didn't think
this adventure was over yet, did you?

zsh -f
% chmod -x .
% cd -s /nonexisting
cd: no such file or directory: /nonexisting
% ls
ls: write error: Bad file descriptor

Oops, stdout is closed now. I can only get it to happen with -s and
the unexecutable dir though, so it's not really worse than the loop
:).

I looked at the code, and while I don't really have any idea what's
going on, the only place that closes d->dirfd without checking
close_dir seems to be restoredir()? I don't know if that is the guilty
party though.

-- 
Mikael Magnusson

Re: cd -s symlink hangs (sometimes?)

[Peter Stephenson]

On Mon, 23 Mar 2009 12:46:10 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> 2009/3/23 Peter Stephenson <pws@csr.com>:
> Trying the patch now and it does stop the leak... but you didn't think
> this adventure was over yet, did you?

No, I definitely want to fix the diagnostics and at the least the internal
setting of pwd when a cd fails, but I'm not sure what a neat way is.

> zsh -f
> % chmod -x .
> % cd -s /nonexisting
> cd: no such file or directory: /nonexisting
> % ls
> ls: write error: Bad file descriptor

That was finger trouble:  parentheses in the wrong place.

Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.217
diff -u -r1.217 utils.c
--- Src/utils.c	23 Mar 2009 10:58:57 -0000	1.217
+++ Src/utils.c	23 Mar 2009 12:21:25 -0000
@@ -5423,7 +5423,7 @@
 #ifdef HAVE_FCHDIR
     if (d->dirfd < 0) {
 	close_dir = 1;
-        if ((d->dirfd = open(".", O_RDONLY | O_NOCTTY) < 0) &&
+        if ((d->dirfd = open(".", O_RDONLY | O_NOCTTY)) < 0 &&
 	    zgetdir(d) && *d->dirname != '/')
 	    d->dirfd = open("..", O_RDONLY | O_NOCTTY);
     }

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: cd -s symlink hangs (sometimes?)

[Bart Schaefer]

On Mar 22, 12:54pm, Peter Stephenson wrote:
}
} The problem is still the code I was scratching my head over before:
} 
} #ifdef HAVE_FCHDIR
}     if (d->dirfd < 0 && (d->dirfd = open(".", O_RDONLY | O_NOCTTY)) < 0 &&
} 	zgetdir(d) && *d->dirname != '/')
} 	d->dirfd = open("..", O_RDONLY | O_NOCTTY);
} #endif
} 
} It's now being executed, but we can't open "." any more.  So it's
} failing to save a fchdir-able directory for restoring, so after the "cd
} -s" fails we end up still in / again.

I did a quick search of the zsh-workers archives and that code appears
to predate the earliest messages from the mailing list that are there,
or at least to predate the habit of mailing patches to the lists.  So
if it was hacked in without fixing the rest of the structure, it was
hacked in a very long time ago.

Anyway, as best I can tell the reason for the open("..") is to cause
an error to be returned from restoredir() later.  At the code above,
if open(".") fails but zgetdir() succeeds, we can stat(".") but not
read it.  So the code attempts to arrange that restoredir() will cd
into a directory that we CAN read (".." in this case), but leaves
mismatched ino,dev information in the dirsave struct so restoredir()
will return nonzero, causing lchdir() to return -2.

In the course of looking at this I think I've spotted another problem,
which possibly only affects filesystems where a double slash at the
root means something different (mainly Cygwin at this point).  Have a
look at zchdir() and consider the consequences of passing it a file
name consisting entirely of "/" characters repeated PATH_MAX+N times.

Re: cd -s symlink hangs (sometimes?)

[Peter Stephenson]

Bart Schaefer wrote:
> Anyway, as best I can tell the reason for the open("..") is to cause
> an error to be returned from restoredir() later.  At the code above,
> if open(".") fails but zgetdir() succeeds, we can stat(".") but not
> read it.  So the code attempts to arrange that restoredir() will cd
> into a directory that we CAN read (".." in this case)

I don't think that's likely to work anyway, though...  if we can't
open(".") because of *directory* permissions, that means we won't be
able to open("..") either.  They're both just entries in the current
directory (that happen to have special meanings).  (It's because we can
only track up the hard-linked layout by laboriously opening the file
".."  in the current directory that it's all so complicated.)

> but leaves
> mismatched ino,dev information in the dirsave struct so restoredir()
> will return nonzero, causing lchdir() to return -2.

Presumably with similar effects to what we're seeing now,
i.e. mismatched diretories.  Sounds like another good reason for
completely rewriting the ("..")  thing.

> In the course of looking at this I think I've spotted another problem,
> which possibly only affects filesystems where a double slash at the
> root means something different (mainly Cygwin at this point).  Have a
> look at zchdir() and consider the consequences of passing it a file
> name consisting entirely of "/" characters repeated PATH_MAX+N times.

So if the initial chdir() fails, shouldn't we rewind the pointer to the
start (dir), then break out of the loop and return failure?  Or what
else?

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: cd -s symlink hangs (sometimes?)

[Peter Stephenson]

On Mon, 23 Mar 2009 12:27:14 +0000
Peter Stephenson <pws@csr.com> wrote:
> On Mon, 23 Mar 2009 12:46:10 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
> > 2009/3/23 Peter Stephenson <pws@csr.com>:
> > Trying the patch now and it does stop the leak... but you didn't think
> > this adventure was over yet, did you?
> 
> No, I definitely want to fix the diagnostics and at the least the internal
> setting of pwd when a cd fails, but I'm not sure what a neat way is.

On top of this, I've found another pre-existing bug down to poorly thought
out code structure, which may be something to do with the side effect
Mikael was seeing:  on failure, restoredir() could call upchdir()
repeatedly because it thought it was doing a recursive glob since the "level"
element wasn't initialised.  I only got this sometimes with an optimised
compilation; before, I was using a debug build which didn't show it up at
all.  I've now put the initialisation for a "struct dirsav" in a
subroutine.

For the diagnostics and pwd, I've just borrowed what the recursive handling
in zsh/files does, which is cd to /, set pwd consistently, and report the
error.  This is at least much better; an algorithm for fixing up the
current directory even better is a good deal more complicated and given you
can't cd to the correct directory in this case it's not clear how useful it
is.  So I'm tempted to leave it at this.

In addition to "cd -s", this may address theoretical problems with recursive
globs under similar circumstances.

Index: Src/glob.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/glob.c,v
retrieving revision 1.69
diff -u -r1.69 glob.c
--- Src/glob.c	27 Jan 2009 09:55:34 -0000	1.69
+++ Src/glob.c	24 Mar 2009 12:37:12 -0000
@@ -492,9 +492,7 @@
     int errssofar = errsfound;
     struct dirsav ds;
 
-    ds.ino = ds.dev = 0;
-    ds.dirname = NULL;
-    ds.dirfd = ds.level = -1;
+    init_dirsav(&ds);
     if (!q)
 	return;
 
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.218
diff -u -r1.218 utils.c
--- Src/utils.c	24 Mar 2009 12:14:36 -0000	1.218
+++ Src/utils.c	24 Mar 2009 12:37:12 -0000
@@ -5356,6 +5356,21 @@
     return 0;
 }
 
+/*
+ * Initialize a "struct dirsav".
+ * The structure will be set to the directory we want to save
+ * the first time we change to a different directory.
+ */
+
+/**/
+mod_export void
+init_dirsav(Dirsav d)
+{
+    d->ino = d->dev = 0;
+    d->dirname = NULL;
+    d->dirfd = d->level = -1;
+}
+
 /* Change directory, without following symlinks.  Returns 0 on success, -1 *
  * on failure.  Sets errno to ENOTDIR if any symlinks are encountered.  If *
  * fchdir() fails, or the current directory is unreadable, we might end up *
@@ -5379,9 +5394,7 @@
 #endif
 
     if (!d) {
-	ds.ino = ds.dev = 0;
-	ds.dirname = NULL;
-	ds.dirfd = -1;
+	init_dirsav(&ds);
 	d = &ds;
     }
 #ifdef HAVE_LSTAT
@@ -5479,6 +5492,17 @@
 	}
     }
     if (restoredir(d)) {
+	int restoreerr = errno;
+	/*
+	 * Failed to restore the directory.
+	 * Just be definite, cd to root and report the result.
+	 */
+	zsfree(pwd);
+	pwd = ztrdup("/");
+	if (chdir(pwd) < 0)
+	    zerr("lost current directory, failed to cd to /: %e", errno);
+	else
+	    zerr("lost current directory: %e: changed to /", restoreerr);
 	if (d == &ds)
 	    zsfree(ds.dirname);
 #ifdef HAVE_FCHDIR
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.154
diff -u -r1.154 zsh.h
--- Src/zsh.h	3 Mar 2009 17:26:08 -0000	1.154
+++ Src/zsh.h	24 Mar 2009 12:37:12 -0000
@@ -382,6 +382,7 @@
 typedef struct cmdnam    *Cmdnam;
 typedef struct complist  *Complist;
 typedef struct conddef   *Conddef;
+typedef struct dirsav    *Dirsav;
 typedef struct features  *Features;
 typedef struct feature_enables  *Feature_enables;
 typedef struct funcstack *Funcstack;
Index: Src/Modules/files.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/files.c,v
retrieving revision 1.20
diff -u -r1.20 files.c
--- Src/Modules/files.c	15 Mar 2009 01:04:50 -0000	1.20
+++ Src/Modules/files.c	24 Mar 2009 12:37:12 -0000
@@ -382,9 +382,7 @@
     reccmd.dirpost_func = dirpost_func;
     reccmd.leaf_func = leaf_func;
     reccmd.magic = magic;
-    ds.ino = ds.dev = 0;
-    ds.dirname = NULL;
-    ds.dirfd = ds.level = -1;
+    init_dirsav(&ds);
     if (opt_recurse || opt_safe) {
 	if ((ds.dirfd = open(".", O_RDONLY|O_NOCTTY)) < 0 &&
 	    zgetdir(&ds) && *ds.dirname != '/')
@@ -476,9 +474,7 @@
     }
     err = err1;
 
-    dsav.ino = dsav.dev = 0;
-    dsav.dirname = NULL;
-    dsav.dirfd = dsav.level = -1;
+    init_dirsav(&dsav);
     d = opendir(".");
     if(!d) {
 	if(!reccmd->opt_noerr)



-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: cd -s symlink hangs (sometimes?)

[Bart Schaefer]

On Mar 24, 12:46pm, Peter Stephenson wrote:
}
} For the diagnostics and pwd, I've just borrowed what the recursive
} handling in zsh/files does, which is cd to /, set pwd consistently,
} and report the error.

This is calling zerr(), which sets errflag, which means a script that
encounters this problem would abort at that point, correct?  (That
seems to be the behavior in a few simple tests I did.)

I'd be worried that if the only error is that chdir returns nonzero,
changing into the root directory creates a security problem (what if
the user is privileged and the next command is "rm -rf *" etc.).

Maybe it would be better to first attempt chdir($HOME) and only fall
back on "/" if that also fails.  Also, should $OLDPWD be getting updated?
I'm undecided on that.  We did change directories, even though not to the
desired destination.

Re: cd -s symlink hangs (sometimes?)

[Peter Stephenson]

On Tue, 24 Mar 2009 08:15:38 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Mar 24, 12:46pm, Peter Stephenson wrote:
> }
> } For the diagnostics and pwd, I've just borrowed what the recursive
> } handling in zsh/files does, which is cd to /, set pwd consistently,
> } and report the error.
> 
> This is calling zerr(), which sets errflag, which means a script that
> encounters this problem would abort at that point, correct?  (That
> seems to be the behavior in a few simple tests I did.)

Yes, this seems serious enough.

> I'd be worried that if the only error is that chdir returns nonzero,
> changing into the root directory creates a security problem (what if
> the user is privileged and the next command is "rm -rf *" etc.).
> 
> Maybe it would be better to first attempt chdir($HOME) and only fall
> back on "/" if that also fails.

That makes sense, that is what "cd" on its own does, after all.

> Also, should $OLDPWD be getting updated?
> I'm undecided on that.  We did change directories, even though not to the
> desired destination.

Codewise, we're a little bit too low down to deal with this easily and
correctly.  I think the only consistent way of doing this would be to call
cd_new_pd(), which we currently only do on a successful cd or pushd.
Anything else looks to me like an inconsistent half measure.  We would then
be executing chpwd (etc.); this might be correct (fixing up a terminal
header, for example), or it might do stuff the user didn't want.  It's hard
to decide.

Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.219
diff -u -r1.219 utils.c
--- Src/utils.c	24 Mar 2009 12:52:07 -0000	1.219
+++ Src/utils.c	24 Mar 2009 15:58:30 -0000
@@ -5493,16 +5493,30 @@
     }
     if (restoredir(d)) {
 	int restoreerr = errno;
+	int i;
 	/*
 	 * Failed to restore the directory.
 	 * Just be definite, cd to root and report the result.
 	 */
-	zsfree(pwd);
-	pwd = ztrdup("/");
-	if (chdir(pwd) < 0)
+	for (i = 0; i < 2; i++) {
+	    const char *cdest;
+	    if (i)
+		cdest = "/";
+	    else {
+		if (!home)
+		    continue;
+		cdest = home;
+	    }
+	    zsfree(pwd);
+	    pwd = ztrdup(cdest);
+	    if (chdir(pwd) == 0)
+		break;
+	}
+	if (i == 2)
 	    zerr("lost current directory, failed to cd to /: %e", errno);
 	else
-	    zerr("lost current directory: %e: changed to /", restoreerr);
+	    zerr("lost current directory: %e: changed to `%s'", restoreerr,
+		pwd);
 	if (d == &ds)
 	    zsfree(ds.dirname);
 #ifdef HAVE_FCHDIR


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: cd -s symlink hangs (sometimes?)

[Mikael Magnusson]

2009/3/24 Peter Stephenson <pws@csr.com>:
> On Mon, 23 Mar 2009 12:27:14 +0000
> Peter Stephenson <pws@csr.com> wrote:
>> On Mon, 23 Mar 2009 12:46:10 +0100
>> Mikael Magnusson <mikachu@gmail.com> wrote:
>> > 2009/3/23 Peter Stephenson <pws@csr.com>:
>> > Trying the patch now and it does stop the leak... but you didn't think
>> > this adventure was over yet, did you?
>>
>> No, I definitely want to fix the diagnostics and at the least the internal
>> setting of pwd when a cd fails, but I'm not sure what a neat way is.
>
> On top of this, I've found another pre-existing bug down to poorly thought
> out code structure, which may be something to do with the side effect
> Mikael was seeing:  on failure, restoredir() could call upchdir()
> repeatedly because it thought it was doing a recursive glob since the "level"
> element wasn't initialised.  I only got this sometimes with an optimised
> compilation; before, I was using a debug build which didn't show it up at
> all.  I've now put the initialisation for a "struct dirsav" in a
> subroutine.
>
> For the diagnostics and pwd, I've just borrowed what the recursive handling
> in zsh/files does, which is cd to /, set pwd consistently, and report the
> error.  This is at least much better; an algorithm for fixing up the
> current directory even better is a good deal more complicated and given you
> can't cd to the correct directory in this case it's not clear how useful it
> is.  So I'm tempted to leave it at this.
>
> In addition to "cd -s", this may address theoretical problems with recursive
> globs under similar circumstances.

I wasn't able to make it happen, but is it possible that under some
circumstance, a command of the form
/unimportant/dir% ls **/*; rm -rf *
could now remove everything in my home directory?

-- 
Mikael Magnusson

Re: cd -s symlink hangs (sometimes?)

[Peter Stephenson]

Mikael Magnusson wrote:
> I wasn't able to make it happen, but is it possible that under some
> circumstance, a command of the form
> /unimportant/dir% ls **/*; rm -rf *
> could now remove everything in my home directory?

It will now abort the current set of commands if restoring the directory
fails and in any case ** doesn't follow symlinks (unlike ***).  In theory.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070