Re: cd -s symlink hangs (sometimes?)

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

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