segfault with exceedingly long path

segfault with exceedingly long path

[Axel Beckert]

Hi,

this is a forward of http://bugs.debian.org/418199

--->8---
Running the following on a tmpfs makes zsh segfault:

% for i in `seq 1000`; do mkdir 0123456789; cd 0123456789; done; cd ..
---8<---

I was able to reproduce this segfault with zsh 5.0.5 as currently in
Debian Testing/Unstable. Oldest version of zsh known to be affected is
4.3.2.

(Currently going through unresolved bugs against zsh in Debian which
involve segfaults.)

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: segfault with exceedingly long path

[Bart Schaefer]

On Jan 18,  1:20am, Axel Beckert wrote:
}
} this is a forward of http://bugs.debian.org/418199

This is a known issue and unlikely ever to be fixed.  Various parts
of the shell rely on system limits such as PATH_MAX which cannot be
dynamically changed.  There's a comment with some explanation around
lines 109-137 of Src/compat.c.

The upshot is that if you try hard enough you can always create a path
that will exceed some limit or other.

Re: segfault with exceedingly long path

[Axel Beckert]

Hi Bart,

On Fri, Jan 17, 2014 at 05:49:02PM -0800, Bart Schaefer wrote:
> On Jan 18,  1:20am, Axel Beckert wrote:
> } this is a forward of http://bugs.debian.org/418199
> 
> This is a known issue and unlikely ever to be fixed.

Thanks for the information. Marking as "won't fix".

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: segfault with exceedingly long path

[Peter Stephenson]

On Fri, 17 Jan 2014 17:49:02 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 18,  1:20am, Axel Beckert wrote:
> }
> } this is a forward of http://bugs.debian.org/418199
> 
> This is a known issue and unlikely ever to be fixed.  Various parts
> of the shell rely on system limits such as PATH_MAX which cannot be
> dynamically changed.  There's a comment with some explanation around
> lines 109-137 of Src/compat.c.
> 
> The upshot is that if you try hard enough you can always create a path
> that will exceed some limit or other.

I'm still in favour of reducing dependency on PATH_MAX wherever
possible, since any arbitrary limit we don't actually need we don't
actually want, but with 81 occurences in source and header files this
isn't a battle that's going to yield any instant gratification.

pws

Re: segfault with exceedingly long path

[Bart Schaefer]

On Jan 19,  7:10pm, Peter Stephenson wrote:
}
} I'm still in favour of reducing dependency on PATH_MAX wherever
} possible, since any arbitrary limit we don't actually need we don't
} actually want

In this particular case even if the shell were able to deal with an
"unlimited" directory nesting depth, system calls like chdir(2) would
begin to fail.  fchdir() sort of works around it by not passing the
path name, but you still must somehow obtain a file descriptor to the
directory, so it's just pushing the problem up a level.

Re: segfault with exceedingly long path

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

On Sun, Jan 19, 2014 at 01:35:50PM -0800, Bart Schaefer wrote:
> In this particular case even if the shell were able to deal with an
> "unlimited" directory nesting depth, system calls like chdir(2) would
> begin to fail.

PATH_MAX is the maximum relative path length, which should be
accepted by chdir(2) and similar system calls.

As long as we don't use paths longer than PATH_MAX in system
calls, it doesn't matter how "deep" the directory structure is.
It only becomes a problem if a very long absolute path is used.

So we could do that in small steps, e.g. first chdir(2) to a
directory < PATH_MAX bytes deep, then from there to the next
directory < PATH_MAX bytes deep and so on until we get down to
the real directory.

I don't think that's worth it, but it works.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: segfault with exceedingly long path

[Bart Schaefer]

On Jan 19, 11:13pm, Simon Ruderich wrote:
}
} So we could do that in small steps, e.g. first chdir(2) to a
} directory < PATH_MAX bytes deep, then from there to the next
} directory < PATH_MAX bytes deep and so on until we get down to
} the real directory.

Yes, but that doesn't even begin to address all the other places
where very long paths become an issue.  Would redirections have to
implcitly chdir along the pathname until they got "close enough"
to pass the path tail to open()?  Can you put a ridiculously long
path into $path or $fpath or $module_path?  $TMPPREFIX?  $ZDOTDIR?
What does [[ -d $longpath ]] do?  zstat $longpath?  ${(A)longpath}?

Can you even do an execve() when $#PWD is longer than PATH_MAX?

I don't think we want to let this can of worms out of Pandora's box,
or we'll be chasing geese until the cows come home to roost.

Re: segfault with exceedingly long path

[Bart Schaefer]

On Jan 19,  4:02pm, Bart Schaefer wrote:
}
} I don't think we want to let this can of worms out of Pandora's box,
} or we'll be chasing geese until the cows come home to roost.

In spite of that ... we could at least not dump core in this specific
case.  There are probably many other core dumps waiting to be exposed.

Behavior becomes undefined once the path gets too long, but:

diff --git a/Src/utils.c b/Src/utils.c
index c6d178c..705d2c4 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -725,32 +725,36 @@ xsymlinks(char *s)
     char **pp, **opp;
     char xbuf2[PATH_MAX*2], xbuf3[PATH_MAX*2];
     int t0, ret = 0;
+    zulong xbuflen = strlen(xbuf);
 
     opp = pp = slashsplit(s);
-    for (; *pp; pp++) {
-	if (!strcmp(*pp, ".")) {
-	    zsfree(*pp);
+    for (; xbuflen < sizeof(xbuf) && *pp; pp++) {
+	if (!strcmp(*pp, "."))
 	    continue;
-	}
 	if (!strcmp(*pp, "..")) {
 	    char *p;
 
-	    zsfree(*pp);
 	    if (!strcmp(xbuf, "/"))
 		continue;
 	    if (!*xbuf)
 		continue;
-	    p = xbuf + strlen(xbuf);
-	    while (*--p != '/');
+	    p = xbuf + xbuflen;
+	    while (*--p != '/')
+		xbuflen--;
 	    *p = '\0';
 	    continue;
 	}
 	sprintf(xbuf2, "%s/%s", xbuf, *pp);
 	t0 = readlink(unmeta(xbuf2), xbuf3, PATH_MAX);
 	if (t0 == -1) {
-	    strcat(xbuf, "/");
-	    strcat(xbuf, *pp);
-	    zsfree(*pp);
+	    zulong pplen = strlen(pp) + 1;
+	    if ((xbuflen += pplen) < sizeof(xbuf)) {
+		strcat(xbuf, "/");
+		strcat(xbuf, *pp);
+	    } else {
+		*xbuf = 0;
+		break;
+	    }
 	} else {
 	    ret = 1;
 	    metafy(xbuf3, t0, META_NOALLOC);
@@ -759,10 +763,9 @@ xsymlinks(char *s)
 		xsymlinks(xbuf3 + 1);
 	    } else
 		xsymlinks(xbuf3);
-	    zsfree(*pp);
 	}
     }
-    free(opp);
+    freearray(opp);
     return ret;
 }
 
@@ -779,8 +782,10 @@ xsymlink(char *s)
 	return NULL;
     *xbuf = '\0';
     xsymlinks(s + 1);
-    if (!*xbuf)
+    if (!*xbuf) {
+	zwarn("path expansion failed, using root directory");
 	return ztrdup("/");
+    }
     return ztrdup(xbuf);
 }
 

-- 
Barton E. Schaefer

Re: segfault with exceedingly long path

[Bart Schaefer]

On Jan 19,  5:10pm, Bart Schaefer wrote:
}
} In spite of that ... we could at least not dump core in this specific
} case.

Given that this is a buffer overflow, this is probably a security concern
as well, because a carefully crafted file path could cause memory to
be overwritten with a desired pattern, and a symbolic link could be used
to divert the shell into that directory.

Re: segfault with exceedingly long path

[Bart Schaefer]

On Jan 19,  5:10pm, Bart Schaefer wrote:
}
} +	    zulong pplen = strlen(pp) + 1;

That should be strlen(*pp).

Re: segfault with exceedingly long path

[Oliver Kiddle]

On 19 Jan, Bart wrote:
> -    if (!*xbuf)
> +    if (!*xbuf) {
> +	zwarn("path expansion failed, using root directory");
>  	return ztrdup("/");

As a result of this, I now get:
zsh -fw
cd /
zsh: path expansion failed, using root directory

So if the directory starts as / the !*xbuf test succeeds and it prints
the warning. I'm not sure whether it would be better to skip the whole
xsymlinks call for a path of "/" or to check the return status from it.

Oliver

Re: segfault with exceedingly long path

[Bart Schaefer]

On Feb 24,  3:38pm, Oliver Kiddle wrote:
}
} So if the directory starts as / the !*xbuf test succeeds and it prints
} the warning. I'm not sure whether it would be better to skip the whole
} xsymlinks call for a path of "/" or to check the return status from it.

Hrm.  xsymlinks() is meant to return 1 when there was a symbolic link
followed, but it is also responsible for collapsing out "../" so in
xsymlink() the return value doesn't tell us whether the input and
output paths should be the same.  Also:

torch% cd /tmp/../
zsh: path expansion failed, using root directory
torch% ln -s / /tmp/root
torch% cd /tmp/root
zsh: path expansion failed, using root directory

So neither checking the return value nor skipping xsymlinks() for "/"
will work, we actually have to differentiate paths that resolve to the
root from paths that fail to resolve.

This is going to require more analysis of xsymlinks() than I have time
to do today (possibly more than I have time to do this week).