| Message ID | 20180325083800.GA5496@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On 3/25/18 10:38 AM, Herbert Xu wrote: > The native version of expandmeta allocates a buffer that may be > overrun for two reasons. First of all the size is 1 byte too small > but this is normally hidden because the minimum size is rounded > up to 2048 bytes. Secondly, if the directory level is deep enough, > any buffer can be overrun. > > This patch fixes both problems by calling realloc when necessary. This was already the case before your patch, but on systems that flat out reject paths longer than PATH_MAX (that includes GNU/Linux), it seems weird that expansion can produce paths which then don't work. Test case: cd /tmp x=xxxxxxxxxxxxxxxxx x=$x$x$x$x$x$x$x$x$x$x$x$x$x$x$x ln -s . $x set -- $x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x* case $1 in *"*") echo "no match" ;; *) if test -e "$1" then echo "match and exists" else echo "match but does not exist" fi ;; esac rm $x I'm seeing "no match" from bash/mksh/pdksh/posh/yash/zsh, but "match but does not exist" from dash/bosh/ksh93. Other commands would naturally print the "File name too long" error. Should the buffer perhaps simply be limited to PATH_MAX, taking care to just give up if something longer is found? That could avoid the need to handle reallocations and result in somewhat smaller and simpler code. The downside would be that if other systems do support paths longer than PATH_MAX, globbing would no longer work for those paths. Cheers, Harald van Dijk -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 26, 2018 at 03:03:38AM +0200, Harald van Dijk wrote: > > This was already the case before your patch, but on systems that flat out > reject paths longer than PATH_MAX (that includes GNU/Linux), it seems weird > that expansion can produce paths which then don't work. PATH_MAX only applies in certain cases. Besides, you can always cd into the directories one level at a time to circumvent it. > Test case: > > cd /tmp > x=xxxxxxxxxxxxxxxxx > x=$x$x$x$x$x$x$x$x$x$x$x$x$x$x$x > ln -s . $x > set -- $x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x/$x* > case $1 in > *"*") echo "no match" ;; > *) if test -e "$1" > then echo "match and exists" > else echo "match but does not exist" > fi ;; > esac > rm $x > > I'm seeing "no match" from bash/mksh/pdksh/posh/yash/zsh, but "match but > does not exist" from dash/bosh/ksh93. Other commands would naturally print > the "File name too long" error. > > Should the buffer perhaps simply be limited to PATH_MAX, taking care to just > give up if something longer is found? That could avoid the need to handle > reallocations and result in somewhat smaller and simpler code. > > The downside would be that if other systems do support paths longer than > PATH_MAX, globbing would no longer work for those paths. Well the fact is that all systems allow paths to exist which are longer than PATH_MAX. Besides, even if we did impose a limit here we'd still have to check that limit at every recursion level which means that the code won't be that much simpler. Cheers,
diff --git a/src/expand.c b/src/expand.c index 705fef7..dadac56 100644 --- a/src/expand.c +++ b/src/expand.c @@ -124,7 +124,7 @@ STATIC void expandmeta(struct strlist *, int); #ifdef HAVE_GLOB STATIC void addglob(const glob_t *); #else -STATIC void expmeta(char *, char *); +STATIC void expmeta(char *, unsigned, unsigned); STATIC struct strlist *expsort(struct strlist *); STATIC struct strlist *msort(struct strlist *, int); #endif @@ -1229,6 +1229,7 @@ addglob(pglob) #else /* HAVE_GLOB */ STATIC char *expdir; +STATIC unsigned expdir_max; STATIC void @@ -1243,6 +1244,7 @@ expandmeta(struct strlist *str, int flag) struct strlist **savelastp; struct strlist *sp; char *p; + unsigned len; if (fflag) goto nometa; @@ -1252,12 +1254,11 @@ expandmeta(struct strlist *str, int flag) INTOFF; p = preglob(str->text, RMESCAPE_ALLOC | RMESCAPE_HEAP); - { - int i = strlen(str->text); - expdir = ckmalloc(i < 2048 ? 2048 : i); /* XXX */ - } + len = strlen(p); + expdir_max = len + PATH_MAX; + expdir = ckmalloc(expdir_max); - expmeta(expdir, p); + expmeta(p, len, 0); ckfree(expdir); if (p != str->text) ckfree(p); @@ -1287,8 +1288,9 @@ nometa: */ STATIC void -expmeta(char *enddir, char *name) +expmeta(char *name, unsigned name_len, unsigned expdir_len) { + char *enddir = expdir + expdir_len; char *p; const char *cp; char *start; @@ -1331,15 +1333,15 @@ expmeta(char *enddir, char *name) } } if (metaflag == 0) { /* we've reached the end of the file name */ - if (enddir != expdir) - metaflag++; + if (!expdir_len) + return; p = name; do { if (*p == '\\') p++; *enddir++ = *p; } while (*p++); - if (metaflag == 0 || lstat64(expdir, &statb) >= 0) + if (lstat64(expdir, &statb) >= 0) addfname(expdir); return; } @@ -1352,18 +1354,13 @@ expmeta(char *enddir, char *name) *enddir++ = *p++; } while (p < start); } - if (enddir == expdir) { + *enddir = 0; + cp = expdir; + expdir_len = enddir - cp; + if (!expdir_len) cp = "."; - } else if (enddir == expdir + 1 && *expdir == '/') { - cp = "/"; - } else { - cp = expdir; - enddir[-1] = '\0'; - } if ((dirp = opendir(cp)) == NULL) return; - if (enddir != expdir) - enddir[-1] = '/'; if (*endname == 0) { atend = 1; } else { @@ -1371,6 +1368,7 @@ expmeta(char *enddir, char *name) *endname = '\0'; endname += esc + 1; } + name_len -= endname - name; matchdot = 0; p = start; if (*p == '\\') @@ -1385,11 +1383,22 @@ expmeta(char *enddir, char *name) scopy(dp->d_name, enddir); addfname(expdir); } else { - for (p = enddir, cp = dp->d_name; - (*p++ = *cp++) != '\0';) - continue; - p[-1] = '/'; - expmeta(p, endname); + unsigned offset; + unsigned len; + + p = stpcpy(enddir, dp->d_name); + *p = '/'; + + offset = p - expdir + 1; + len = offset + name_len + NAME_MAX; + if (len > expdir_max) { + len += PATH_MAX; + expdir = ckrealloc(expdir, len); + expdir_max = len; + } + + expmeta(endname, name_len, offset); + enddir = expdir + expdir_len; } } }
The native version of expandmeta allocates a buffer that may be overrun for two reasons. First of all the size is 1 byte too small but this is normally hidden because the minimum size is rounded up to 2048 bytes. Secondly, if the directory level is deep enough, any buffer can be overrun. This patch fixes both problems by calling realloc when necessary. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>