Re: [PATCH] Re: `pwd -P` with systemd-homed causes inconsistent cwd state

[Kyle Laker <kyle@laker.email>]

On 11/3/23 00:28, Bart Schaefer wrote:> Somehow getcwd() might bypass 
whatever restriction caused this to
> fail?  In all other cases we've chdir'd away from the current
> directory and can't get back.  I'm tempted to say we should just
> delete that entire fallback, but then we never use getcwd() at all.

If that fallback gets deleted, it seems like HAVE_GETCWD in its entirety 
isn't particularly useful anymore; this is the only `#ifdef` for it. It 
might simplify the logic a bit; if USE_GETCWD then do so, otherwise, 
rely on fallbacks (especially since zgetdir() itself may be calling 
getcwd()).

> Maybe we should be checking errno (for what?) as well as ret before
> falling back to getcwd()?  Or maybe when USE_GETCWD, we should use it
> preferentially, and zgetdir() should be the fallback?

I think zgetdir() does use getcwd() preferentially when USE_GETCWD. It's 
awkwardly at the bottom of zgetdir(). When USE_GETCWD, all the internal 
pathwalking logic of zgetdir() is skipped.

> Or maybe zgetdir() itself should USE_GETCWD there at line 364?

That feels possibly the cleanest. That would allow removing all the 
calls to getcwd() from within zgetcwd() itself.

I've attached a patch that builds on yours. Testing it in my use case, 
it correctly works when HAVE_GETCWD && USE_GETCWD (eagerly invokes 
getcwd()), HAVE_GETCWD && !USE_GETCWD (zgetdir() returns null and the 
pwd fallback in zgetcwd() is used), and !HAVE_GETCWD && !USE_GETCWD 
(same as when HAVE && !USE). The patch also removes 
`GETCWD_CALLS_MALLOC` because it was only used in that fallback case. 
Running `make check` for me still passes.
diff --git a/Src/compat.c b/Src/compat.c
index 817bb4aaf..23ad703f4 100644
--- a/Src/compat.c
+++ b/Src/compat.c
@@ -359,9 +359,40 @@ zgetdir(struct dirsav *d)
     buf = zhalloc(bufsiz = PATH_MAX+1);
     pos = bufsiz - 1;
     buf[pos] = '\0';
+#if defined(USE_GETCWD) || defined(__CYGWIN__)
+    if (!getcwd(buf, bufsiz)) {
+	if (d) {
+	    return NULL;
+	}
+    } else {
+	if (d) {
+	    return d->dirname = ztrdup(buf);
+	}
+	return buf;
+    }
+#else
     strcpy(nbuf, "../");
     if (stat(".", &sbuf) < 0) {
+	/*
+	 * Fallback to getcwd() since it may be able to overcome
+	 * the failure in stat(). We haven't changed directories
+	 * to walk the path yet, so this should return the current
+	 * directory.
+	 */
+#ifdef HAVE_GETCWD
+	if (!getcwd(buf, bufsiz)) {
+	    if (d) {
+		return NULL;
+	    }
+	} else {
+	    if (d) {
+		return d->dirname = ztrdup(buf);
+	    }
+	    return buf;
+	}
+#else
 	return NULL;
+#endif
     }
 
     /* Record the initial inode and device */
@@ -369,7 +400,6 @@ zgetdir(struct dirsav *d)
     pdev = sbuf.st_dev;
     if (d)
 	d->ino = pino, d->dev = pdev;
-#if !defined(__CYGWIN__) && !defined(USE_GETCWD)
 #ifdef HAVE_FCHDIR
     else
 #endif
@@ -403,12 +433,25 @@ zgetdir(struct dirsav *d)
 		buf[--pos] = '/';
 	    if (d) {
 #ifndef HAVE_FCHDIR
-		zchdir(buf + pos);
+		if (zchdir(buf + pos) < 0) {
+		    /*
+		     * The reason for all this chdir-ing is to get the
+		     * absolute name of the current directory, so if we
+		     * cannot chdir to that name we have to assume our
+		     * directory is lost.  See "something bad" below.
+		     */
+		    noholdintr();
+		    return NULL;
+		}
 		noholdintr();
 #endif
 		return d->dirname = ztrdup(buf + pos);
 	    }
-	    zchdir(buf + pos);
+	    if (zchdir(buf + pos) < 0) {
+		/* Current directory lost */
+		noholdintr();
+		return NULL;
+	    }
 	    noholdintr();
 	    return buf + pos;
 	}
@@ -477,28 +520,23 @@ zgetdir(struct dirsav *d)
     if (d) {
 #ifndef HAVE_FCHDIR
 	if (buf[pos])
-	    zchdir(buf + pos + 1);
+	    if (zchdir(buf + pos + 1) < 0) {
+		/* Current directory lost */
+		noholdintr();
+		return NULL;
+	    }
 	noholdintr();
 #endif
 	return NULL;
     }
 
     if (buf[pos])
-	zchdir(buf + pos + 1);
-    noholdintr();
-
-#else  /* __CYGWIN__, USE_GETCWD cases */
-
-    if (!getcwd(buf, bufsiz)) {
-	if (d) {
+	if (zchdir(buf + pos + 1) < 0) {
+	    /* Current directory lost */
+	    noholdintr();
 	    return NULL;
 	}
-    } else {
-	if (d) {
-	    return d->dirname = ztrdup(buf);
-	}
-	return buf;
-    }
+    noholdintr();
 #endif
 
     /*
@@ -526,25 +564,12 @@ mod_export 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+1);
-	ret = getcwd(cwdbuf, PATH_MAX);
-	if (ret)
-	    ret = dupstring(ret);
-	zfree(cwdbuf, PATH_MAX+1);
-#endif /* GETCWD_CALLS_MALLOC */
-    }
-#endif /* HAVE_GETCWD */
     if (!ret)
-	ret = unmeta(pwd);
+	if (chdir(ret = unmeta(pwd)) < 0) {
+	    zwarn("%e: current directory %s lost, using /",
+		  errno, ret);
+	    chdir(ret = dupstring("/"));
+	}
     if (!ret || *ret == '\0')
 	ret = dupstring(".");
     return ret;
diff --git a/configure.ac b/configure.ac
index c5263035e..5dd6c7892 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2068,31 +2068,6 @@ 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_RUN_IFELSE([AC_LANG_SOURCE([[
-#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])