Re: cwd unintentionally changed

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Thu, 2019-04-25 at 14:03 +0200, Yannic Schröder wrote:
> Hi *,
> 
> I got myself into a situation where zsh changed my working directory to
> "/" without further notice. Unfortunately, the next command I issued was
> "sudo rm -rf *", which did not end well with cwd being "/" :-(
> 
> My colleagues and me started to track down the bug. Our efforts are
> documented here:
> https://github.com/grml/grml-etc-core/issues/76
> 
> (We initially though it was a bug with grml zsh config.)
> 
> A minimal example that triggers the bug looks like this:
> 
> $ mkdir /tmp/tmp.0HnyRZ1iXv/
> $ cd /tmp/tmp.0HnyRZ1iXv/
> $ mkdir a
> $ sudo zsh -f -c 'mount -t tmpfs tmpfs a; cd a; touch x; ls; umount
> --lazy ../a; ls; foo=.; echo ${foo:a}; ls; realpath .; echo $PWD;'

Thanks, easy to follow.

That's certainly not pleasant as it's silent.

The source of the problem is in zgetdir().  We attempt to climb the
directory hierarchy until we get to /.  In this case we appear to get
there immediately.  Then, fatally, we call zchdir() to the place where we
think we are, but actually we aren't.

The patch below fixes up this case so we make quite sure we're in /.
This looks safe, but I'm not sure it's necessarily the limit of
what we can do to make things as safe as possible...

I'm not quite sure why we need to zchdir().  It seems to be a side
effect rather than a basic part of the call.  In particular I don't see
why it's a good idea in this case --- maybe we just add an argument
saying "not actually changing directory"?

Or should we be using getcwd() nowadays?  Presumably this is fine on most
modern systems?

pws

diff --git a/Src/compat.c b/Src/compat.c
index 7b5c4411c..7131d91a4 100644
--- a/Src/compat.c
+++ b/Src/compat.c
@@ -361,8 +361,18 @@ zgetdir(struct dirsav *d)
 	pino = sbuf.st_ino;
 	pdev = sbuf.st_dev;
 
-	/* If they're the same, we've reached the root directory. */
+	/* If they're the same, we've reached the root directory... */
 	if (ino == pino && dev == pdev) {
+	    /*
+	     * ...well, probably.  If this was an orphaned . after
+	     * an unmount, or something such, we could be in trouble...
+	     */
+	    if (stat("/", &sbuf) < 0 ||
+		sbuf.st_ino != ino ||
+		sbuf.st_dev != dev) {
+		zerr("Failed to get current directory: path invalid");
+		return NULL;
+	    }
 	    if (!buf[pos])
 		buf[--pos] = '/';
 	    if (d) {