Wrong PWD with chasedots

Wrong PWD with chasedots

[Marc Finet]

Hello,

I encountered a strange behavior with chasedots. pwd is correct but PWD
not. I re-used the example given in man page. With chasedots deactivated:

    # pwd
    /foo
    # ls -l
    total 0
    lrwxrwxrwx 1 marc marc 8 Nov  7 20:07 bar -> /alt/rod
    # cd bar
    # pwd
    /foo/bar
    # pwd -P
    /alt/rod
    # echo $PWD
    /foo/bar
    # cd ..
    # pwd
    /foo
    # echo $PWD
    /foo

With chasedots activated:
    # pwd
    /foo
    # setopt chasedots
    # cd bar
    # pwd
    /foo/bar
    # pwd -P
    /alt/rod
    # echo $PWD
    /foo/bar
    # cd ..
    # pwd
    /alt
    # pwd -L
    /alt
    # pwd -P
    /alt
    # echo $PWD
    /
I expected the last output to be /alt. Since my prompt uses $PWD and not
`pwd` I have a a wrong path reported in my prompt. I quickly started
looking at code, but thought that some here have better overview
of what's going on.

Thanks,

Marc.

Re: Wrong PWD with chasedots

[Marc Finet]

On Tue, Nov 11, 2014 at 07:35:32PM -0800, Bart Schaefer wrote:
> On Nov 11, 11:35am, Marc Finet wrote:
> } Subject: Wrong PWD with chasedots
> }
> } With chasedots activated:
> }     # setopt chasedots
> }     # cd bar
> }     # cd ..
> }     # pwd
> }     /alt
> }     # echo $PWD
> }     /
> 
> I have some vague recollection of this having been mentioned before.
> I can reproduce it exactly once:  the first time I go through those
> steps in any new shell.  Thereafter it works as expected, i.e., $PWD
> has the same value as $(pwd).
I did not experienced the same behavior. I always have the problem.
 
> I've gotten as far as discovering that builtin.c:cd_new_pwd() produces
> different results on the first call as opposed to any later call (that
> is, the first call after "setopt chasedots") but have run out of time
> to keep looking.
I had some time to investigate and the problem lies in the
utils.c::xsymlinks() where there is a discrepancy between the content of
the global variable xbuf and the local value xbuflen:
 - in the case a symlink is found, xsymlinks() is called,
 - xbuf is updated but not xbuflen in the calling function
 - later, when ".." is found, it is trimmed but uses xbuf + xbuflen as
   starting point, not xbuf + its length. This leads to lose the data
   set by xsymlinks(), i.e. the name of the sub-directories.
This wrong behavior has been introduced by commit 3e06aeabd8a9e8384ebaa
"32294: prevent buffer overflow when scanning very long directory paths
for symbolic links".

So I have two patches. The first one forces the xbuflen synchronization:
--8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<--
diff --git a/Src/utils.c b/Src/utils.c
index e6eb8e6..4a13eb9 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -762,9 +762,13 @@ xsymlinks(char *s)
 		strcpy(xbuf, "");
 		if (xsymlinks(xbuf3 + 1) < 0)
 		    ret = -1;
+		else
+		    xbuflen = strlen(xbuf);
 	    } else
 		if (xsymlinks(xbuf3) < 0)
 		    ret = -1;
+		else
+		    xbuflen = strlen(xbuf);
 	}
     }
     freearray(opp);
--8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<--

The second patch changes local xbuflen into a global to follow xbuf
"localization":
--8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<--
diff --git a/Src/utils.c b/Src/utils.c
index e6eb8e6..5d67781 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -684,6 +684,7 @@ ispwd(char *s)
 }
 
 static char xbuf[PATH_MAX*2];
+static zulong xbuflen;
 
 /**/
 static char **
@@ -724,7 +725,6 @@ xsymlinks(char *s)
     char **pp, **opp;
     char xbuf2[PATH_MAX*3], xbuf3[PATH_MAX*2];
     int t0, ret = 0;
-    zulong xbuflen = strlen(xbuf);
 
     opp = pp = slashsplit(s);
     for (; xbuflen < sizeof(xbuf) && *pp; pp++) {
@@ -783,6 +783,7 @@ xsymlink(char *s)
     if (*s != '/')
 	return NULL;
     *xbuf = '\0';
+    xbuflen = 0;
     if (xsymlinks(s + 1) < 0)
 	zwarn("path expansion failed, using root directory");
     if (!*xbuf)
@@ -796,6 +797,7 @@ print_if_link(char *s)
 {
     if (*s == '/') {
 	*xbuf = '\0';
+	xbuflen = 0;
 	if (xsymlinks(s + 1) > 0)
 	    printf(" -> "), zputs(*xbuf ? xbuf : "/", stdout);
     }
--8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<--

I do not know which one do you prefer.

While reading the xsymlinks() function I think that a (ret >= 0)
condition should be added in the for() loop to exit early. Or a break
should be added after setting ret to -1 in the case of error after
symlink is found. Anyway, it's not homogeneous, but I do not know which
method is 'cleaner'.
--8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<--
diff --git a/Src/utils.c b/Src/utils.c
index e6eb8e6..c66b7ae 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -727,7 +727,7 @@ xsymlinks(char *s)
     zulong xbuflen = strlen(xbuf);
 
     opp = pp = slashsplit(s);
-    for (; xbuflen < sizeof(xbuf) && *pp; pp++) {
+    for (; xbuflen < sizeof(xbuf) && *pp && ret >= 0; pp++) {
 	if (!strcmp(*pp, "."))
 	    continue;
 	if (!strcmp(*pp, "..")) {
--8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<-8<--

Marc.

Re: Wrong PWD with chasedots

[Peter Stephenson]

On Thu, 13 Nov 2014 19:46:20 +0100
Marc Finet <m.dreadlock@gmail.com> wrote:
> I had some time to investigate and the problem lies in the
> utils.c::xsymlinks() where there is a discrepancy between the content of
> the global variable xbuf and the local value xbuflen:
>  - in the case a symlink is found, xsymlinks() is called,
>  - xbuf is updated but not xbuflen in the calling function
>  - later, when ".." is found, it is trimmed but uses xbuf + xbuflen as
>    starting point, not xbuf + its length. This leads to lose the data
>    set by xsymlinks(), i.e. the name of the sub-directories.
> This wrong behavior has been introduced by commit 3e06aeabd8a9e8384ebaa
> "32294: prevent buffer overflow when scanning very long directory paths
> for symbolic links".
>
> So I have two patches. The first one forces the xbuflen synchronization:

Thanks, I've applied this.

> While reading the xsymlinks() function I think that a (ret >= 0)
> condition should be added in the for() loop to exit early. Or a break
> should be added after setting ret to -1 in the case of error after
> symlink is found. Anyway, it's not homogeneous, but I do not know which
> method is 'cleaner'.

And this.

pws