Bug in zsh builtin pwd when option CHASE_LINKS is set

Bug in zsh builtin pwd when option CHASE_LINKS is set

[Vincent Lefevre]

Hi,

lepuid:~> zsh -f
lepuid% setopt CHASE_LINKS
lepuid% pwd
/users/spaces/lefevre
lepuid% cd .snapshot
lepuid% pwd
.
lepuid% /bin/pwd
/users/spaces/lefevre/.snapshot
lepuid% echo $PWD
/users/spaces/lefevre/.snapshot

Note: .snapshot is a special read-only directory set up by the
NFS server and that is not seen in the directory listing.

When CHASE_LINKS (the default), I get the normal behavior.

Regards,

-- 
Vincent Lefèvre <vincent@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / SPACES project at LORIA

Re: Bug in zsh builtin pwd when option CHASE_LINKS is set

[Vincent Lefevre]

Hi,

On 2004-09-24 09:48:42 +0200, Vincent Lefevre wrote:
> lepuid:~> zsh -f
> lepuid% setopt CHASE_LINKS
> lepuid% pwd
> /users/spaces/lefevre
> lepuid% cd .snapshot
> lepuid% pwd
> .
> lepuid% /bin/pwd
> /users/spaces/lefevre/.snapshot
> lepuid% echo $PWD
> /users/spaces/lefevre/.snapshot
> 
> Note: .snapshot is a special read-only directory set up by the
> NFS server and that is not seen in the directory listing.
> 
> When CHASE_LINKS (the default), I get the normal behavior.

I have the same problem with zsh 4.3.10 when I cd a directory
with the following permissions: drwxrwxr-x. /bin/pwd and $PWD
give the correct directory, but the pwd built-in gives ".".

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / Arénaire project (LIP, ENS-Lyon)

Re: Bug in zsh builtin pwd when option CHASE_LINKS is set

[Peter Stephenson]

On Mon, 7 Jun 2010 15:13:39 +0200
Vincent Lefevre <vincent@vinc17.net> wrote:
> On 2004-09-24 09:48:42 +0200, Vincent Lefevre wrote:
> > lepuid:~> zsh -f
> > lepuid% setopt CHASE_LINKS
> > lepuid% pwd
> > /users/spaces/lefevre
> > lepuid% cd .snapshot
> > lepuid% pwd
> > .

This is happening on our Netapp too, not surprisingly.

The .snapshot isn't in the parent's directory list, so we're stuck.  We'll
just have to use a best guess. "." isn't a particularly good one.

I had to insert some comments into the function to understand it.

What are the tests for *buf doing?  That doesn't mean anything
unless we happen to have exactly PATH_MAX characters in the directory
name.  If we don't, *buf has never been set (not even to '\0').  Shouldn't
it be testing buf[pos]?  I've assumed so.


Index: Src/compat.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/compat.c,v
retrieving revision 1.19
diff -p -u -r1.19 compat.c
--- Src/compat.c	22 Feb 2010 10:12:22 -0000	1.19
+++ Src/compat.c	7 Jun 2010 16:42:33 -0000
@@ -275,11 +275,7 @@ zgetdir(struct dirsav *d)
     buf[pos] = '\0';
     strcpy(nbuf, "../");
     if (stat(".", &sbuf) < 0) {
-	if (d)
-	    return NULL;
-	buf[0] = '.';
-	buf[1] = '\0';
-	return buf;
+	return NULL;
     }
 
     /* Record the initial inode and device */
@@ -354,6 +350,11 @@ zgetdir(struct dirsav *d)
 	closedir(dir);
 	if (!de)
 	    break;		/* Not found */
+	/*
+	 * We get the "/" free just by copying from nbuf+2 instead
+	 * of nbuf+3, which is where we copied the path component.
+	 * This means buf[pos] is always a "/".
+	 */
 	len = strlen(nbuf + 2);
 	pos -= len;
 	while (pos <= 1) {
@@ -371,15 +372,22 @@ zgetdir(struct dirsav *d)
 	if (chdir(".."))
 	    break;
     }
+
+    /*
+     * Fix up the directory, if necessary.
+     * We're changing back down the hierarchy, ignore the
+     * "/" at buf[pos].
+     */
     if (d) {
 #ifndef HAVE_FCHDIR
-	if (*buf)
+	if (buf[pos])
 	    zchdir(buf + pos + 1);
 	noholdintr();
 #endif
 	return NULL;
     }
-    if (*buf)
+
+    if (buf[pos])
 	zchdir(buf + pos + 1);
     noholdintr();
 
@@ -397,16 +405,27 @@ zgetdir(struct dirsav *d)
     }
 #endif
 
-    buf[0] = '.';
-    buf[1] = '\0';
-    return buf;
+    /*
+     * Something bad happened.
+     * This has been seen when inside a special directory,
+     * such as the Netapp .snapshot directory, that doesn't
+     * appear as a directory entry in the parent directory.
+     * We'll just need our best guess.
+     *
+     * We only get here from zgetcwd(); let that fall back to pwd.
+     */
+
+    return NULL;
 }
 
 /**/
 char *
 zgetcwd(void)
 {
-    return zgetdir(NULL);
+    char *ret = xzgetdir(NULL);
+    if (!ret)
+	ret = pwd;
+    return ret;
 }
 
 /* chdir with arbitrary long pathname.  Returns 0 on success, -1 on normal *

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


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Bug in zsh builtin pwd when option CHASE_LINKS is set

[Peter Stephenson]

Looks like we need to be more careful in case zgetcwd() is being used to
set a new value of pwd.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.242
diff -p -u -r1.242 builtin.c
--- Src/builtin.c	27 May 2010 18:57:34 -0000	1.242
+++ Src/builtin.c	7 Jun 2010 17:00:13 -0000
@@ -805,6 +805,7 @@ bin_cd(char *nam, char **argv, Options o
     if (stat(unmeta(pwd), &st1) < 0) {
 	setjobpwd();
 	zsfree(pwd);
+	pwd = NULL;
 	pwd = metafy(zgetcwd(), -1, META_DUP);
     } else if (stat(".", &st2) < 0) {
 	if (chdir(unmeta(pwd)) < 0)
@@ -813,6 +814,7 @@ bin_cd(char *nam, char **argv, Options o
 	if (chasinglinks) {
 	    setjobpwd();
 	    zsfree(pwd);
+	    pwd = NULL;
 	    pwd = metafy(zgetcwd(), -1, META_DUP);
 	} else if (chdir(unmeta(pwd)) < 0)
 	    zwarn("unable to chdir(%s): %e", pwd, errno);
Index: Src/compat.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/compat.c,v
retrieving revision 1.19
diff -p -u -r1.19 compat.c
--- Src/compat.c	22 Feb 2010 10:12:22 -0000	1.19
+++ Src/compat.c	7 Jun 2010 17:00:14 -0000
@@ -275,11 +275,7 @@ zgetdir(struct dirsav *d)
     buf[pos] = '\0';
     strcpy(nbuf, "../");
     if (stat(".", &sbuf) < 0) {
-	if (d)
-	    return NULL;
-	buf[0] = '.';
-	buf[1] = '\0';
-	return buf;
+	return NULL;
     }
 
     /* Record the initial inode and device */
@@ -354,6 +350,11 @@ zgetdir(struct dirsav *d)
 	closedir(dir);
 	if (!de)
 	    break;		/* Not found */
+	/*
+	 * We get the "/" free just by copying from nbuf+2 instead
+	 * of nbuf+3, which is where we copied the path component.
+	 * This means buf[pos] is always a "/".
+	 */
 	len = strlen(nbuf + 2);
 	pos -= len;
 	while (pos <= 1) {
@@ -371,15 +372,22 @@ zgetdir(struct dirsav *d)
 	if (chdir(".."))
 	    break;
     }
+
+    /*
+     * Fix up the directory, if necessary.
+     * We're changing back down the hierarchy, ignore the
+     * "/" at buf[pos].
+     */
     if (d) {
 #ifndef HAVE_FCHDIR
-	if (*buf)
+	if (buf[pos])
 	    zchdir(buf + pos + 1);
 	noholdintr();
 #endif
 	return NULL;
     }
-    if (*buf)
+
+    if (buf[pos])
 	zchdir(buf + pos + 1);
     noholdintr();
 
@@ -397,16 +405,36 @@ zgetdir(struct dirsav *d)
     }
 #endif
 
-    buf[0] = '.';
-    buf[1] = '\0';
-    return buf;
+    /*
+     * Something bad happened.
+     * This has been seen when inside a special directory,
+     * such as the Netapp .snapshot directory, that doesn't
+     * appear as a directory entry in the parent directory.
+     * We'll just need our best guess.
+     *
+     * We only get here from zgetcwd(); let that fall back to pwd.
+     */
+
+    return NULL;
 }
 
+/*
+ * Try to find the current directory.
+ * If it fails, fall back to pwd; if zgetcwd() is being used
+ * to set pwd, pwd should be NULL and we just return ".".
+ * We could fall back to getcwd() instead.
+ */
+
 /**/
 char *
 zgetcwd(void)
 {
-    return zgetdir(NULL);
+    char *ret = zgetdir(NULL);
+    if (!ret)
+	ret = pwd;
+    if (!ret)
+	ret = dupstring(".");
+    return ret;
 }
 
 /* chdir with arbitrary long pathname.  Returns 0 on success, -1 on normal *
Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.114
diff -p -u -r1.114 init.c
--- Src/init.c	12 May 2010 10:07:01 -0000	1.114
+++ Src/init.c	7 Jun 2010 17:00:14 -0000
@@ -861,8 +861,10 @@ setupvals(void)
     else if ((ptr = zgetenv("PWD")) && (strlen(ptr) < PATH_MAX) &&
 	     (ptr = metafy(ptr, -1, META_STATIC), ispwd(ptr)))
 	pwd = ztrdup(ptr);
-    else
+    else {
+	pwd = NULL;
 	pwd = metafy(zgetcwd(), -1, META_DUP);
+    }
 
     oldpwd = ztrdup(pwd);  /* initialize `OLDPWD' = `PWD' */
 

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


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Bug in zsh builtin pwd when option CHASE_LINKS is set

[Peter Stephenson]

On Mon, 7 Jun 2010 18:00:46 +0100
Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> + * We could fall back to getcwd() instead.

I can't see how this could be worse.  It gets the right answer for
.snapshot directories, i.e. it follows symbolic links further up the path
while still locating the tail end of the path.  It takes a while, though.

Index: configure.ac
===================================================================
RCS file: /cvsroot/zsh/zsh/configure.ac,v
retrieving revision 1.129
diff -p -u -r1.129 configure.ac
--- configure.ac	18 Dec 2009 19:50:45 -0000	1.129
+++ configure.ac	9 Jun 2010 09:19:26 -0000
@@ -1170,7 +1170,7 @@ AC_CHECK_FUNCS(strftime strptime mktime 
 	       regcomp regexec regerror regfree \
 	       gdbm_open getxattr \
 	       realpath canonicalize_file_name \
-	       symlink)
+	       symlink getcwd)
 AC_FUNC_STRCOLL
 
 if test x$enable_cap = xyes; then
@@ -1898,6 +1898,34 @@ if test x$zsh_cv_use_getcwd = xyes; then
   AC_DEFINE(USE_GETCWD)
 fi
 
+dnl GNU getcwd() can allocate as much space as necessary for a
+dnl directory name, preventing guessing games.
+AH_TEMPLATE([GETCWD_CALLS_MALLOC],
+[Define to 1 if getcwd() calls malloc to allocate memory.])
+if test x$ac_cv_func_getcwd = xyes; then
+  AC_CACHE_CHECK(whether getcwd calls malloc to allocate memory,
+  zsh_cv_getcwd_malloc,
+  [AC_TRY_RUN([
+#include <unistd.h>
+#include <string.h>
+int main() {
+    char buf[1024], *ptr1, *ptr2;
+    ptr1 = getcwd(buf, 1024);
+    ptr2 = getcwd(NULL, 0);
+    if (ptr1 && ptr2 && !strcmp(ptr1, ptr2)) {
+      return 0;
+    }
+    return 1;
+}
+],
+  zsh_cv_getcwd_malloc=yes,
+  zsh_cv_getcwd_malloc=no,
+  zsh_cv_getcwd_malloc=no)])
+  if test x$zsh_cv_getcwd_malloc = xyes; then
+    AC_DEFINE(GETCWD_CALLS_MALLOC)
+  fi
+fi
+
 dnl CHECK FOR setproctitle() FOR jobs -Z / ARGV0
 AH_TEMPLATE([HAVE_SETPROCTITLE],
 [Define to 1 if the system supports `setproctitle' to change process name])
Index: Src/compat.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/compat.c,v
retrieving revision 1.20
diff -p -u -r1.20 compat.c
--- Src/compat.c	8 Jun 2010 08:51:04 -0000	1.20
+++ Src/compat.c	9 Jun 2010 09:19:26 -0000
@@ -420,9 +420,9 @@ zgetdir(struct dirsav *d)
 
 /*
  * Try to find the current directory.
+ * If we couldn't work it out internally, fall back to getcwd().
  * If it fails, fall back to pwd; if zgetcwd() is being used
  * to set pwd, pwd should be NULL and we just return ".".
- * We could fall back to getcwd() instead.
  */
 
 /**/
@@ -430,6 +430,23 @@ char *
 zgetcwd(void)
 {
     char *ret = zgetdir(NULL);
+#ifdef HAVE_GETCWD
+    if (!ret) {
+#ifdef GETCWD_CALLS_MALLOC
+	char *cwd = getcwd(NULL, 0);
+	if (cwd) {
+	    ret = dupstring(cwd);
+	    free(cwd);
+	}
+#else
+	char *cwdbuf = zalloc(PATH_MAX);
+	ret = getcwd(cwdbuf, PATH_MAX);
+	if (ret)
+	    ret = dupstring(ret);
+	free(cwdbuf);
+#endif /* GETCWD_CALLS_MALLOC */
+    }
+#endif /* HAVE_GETCWD */
     if (!ret)
 	ret = pwd;
     if (!ret)

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


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Bug in zsh builtin pwd when option CHASE_LINKS is set

[Bart Schaefer]

On Mon, Jun 7, 2010 at 9:43 AM, Peter Stephenson
<Peter.Stephenson@csr.com> wrote:
> On Mon, 7 Jun 2010 15:13:39 +0200
> Vincent Lefevre <vincent@vinc17.net> wrote:
>> On 2004-09-24 09:48:42 +0200, Vincent Lefevre wrote:
>> > lepuid:~> zsh -f
>> > lepuid% setopt CHASE_LINKS
>> > lepuid% pwd
>> > /users/spaces/lefevre
>> > lepuid% cd .snapshot
>> > lepuid% pwd
>> > .
>
> This is happening on our Netapp too, not surprisingly.
>
> The .snapshot isn't in the parent's directory list, so we're stuck.  We'll
> just have to use a best guess. "." isn't a particularly good one.

There was just recently a lot of discussion about behavior of cd /
$PWD / $OLDPWD on the austin-group list.  I'm rather short on time
this week so rather than try to research it I'll just ask whether
anyone thinks there are relevant bits there, and hope someone else can
follow up.

(Sorry.)

Re: Bug in zsh builtin pwd when option CHASE_LINKS is set

[Vincent Lefevre]

On 2010-06-07 15:13:39 +0200, Vincent Lefevre wrote:
> I have the same problem with zsh 4.3.10 when I cd a directory
> with the following permissions: drwxrwxr-x. /bin/pwd and $PWD
> give the correct directory, but the pwd built-in gives ".".

FYI, the problem was not due to the permissions, but I've just noticed
that the parent directory was not readable (it just has the execution
bit for the group).

I can reproduce the bug on other machines, where I haven't applied
the patch:

xvii% setopt CHASE_LINKS
xvii% mkdir -p dir1/dir2
xvii% chmod 100 dir1
xvii% cd dir1/dir2
xvii% /bin/pwd
/home/vinc17/dir1/dir2
xvii% pwd
.
xvii% 

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / Arénaire project (LIP, ENS-Lyon)