| Message ID | 20171128210857.GC2745@madrid |
|---|---|
| State | New |
| Headers | show |
| Series | posix: if glob has a trailing slash match directories only. | expand |
(+cc: bug-gnulib@, since they share this code) Dmitry Goncharov wrote: > If pattern has a trailing slash match directories only. > This patch fixes [BZ #22513]. > > +2017-11-28 Dmitry Goncharov <dgoncharov@users.sf.net> > + > + [BZ #22513] > + * posix/glob.c (glob_in_dir): Make glob with a trailing slash match > + directores only. > + * posix/globtest.sh: Add tests. > > Tested on x86-64 linux. Thanks. Looks like a reasonable thing to do. > diff --git a/posix/glob.c b/posix/glob.c > index cb39779d07..78873d83c6 100644 > --- a/posix/glob.c > +++ b/posix/glob.c > @@ -1342,7 +1342,33 @@ glob_in_dir (const char *pattern, const char *directory, int flags, > if (flags & GLOB_ONLYDIR) > switch (readdir_result_type (d)) > { > - case DT_DIR: case DT_LNK: case DT_UNKNOWN: break; > + case DT_DIR: case DT_LNK: break; > + case DT_UNKNOWN: > + { > + int dir; > + size_t namlen = strlen (d.name); > + size_t fullsize; > + bool alloca_fullname > + = (! size_add_wrapv (dirlen + 1, namlen + 1, &fullsize) > + && glob_use_alloca (alloca_used, fullsize)); > + char *fullname; > + if (alloca_fullname) > + fullname = alloca_account (fullsize, alloca_used); > + else > + { > + fullname = malloc (fullsize); > + if (fullname == NULL) > + return GLOB_NOSPACE; > + } > + mempcpy (mempcpy (mempcpy (fullname, directory, dirlen), > + "/", 1), > + d.name, namlen + 1); > + dir = is_dir (fullname, flags, pglob); > + if (__glibc_unlikely (!alloca_fullname)) > + free (fullname); > + if (dir) > + break; > + } > default: continue; > } > If I understand correctly, this treats DT_UNKNOWN like DT_DIR when stat indicates that it is a directory. What should happen in the DT_LNK case? Should the same logic trip for it as well so we can distinguish between a symlink to a directory and other symlinks? > diff --git a/posix/globtest.sh b/posix/globtest.sh > index 73f7ae31cc..4f0c03715a 100755 > --- a/posix/globtest.sh > +++ b/posix/globtest.sh > @@ -43,13 +43,19 @@ export LC_ALL > > # Create the arena > testdir=${common_objpfx}posix/globtest-dir > +testdir2=${common_objpfx}posix/globtest-dir2 > testout=${common_objpfx}posix/globtest-out > rm -rf $testdir $testout > mkdir $testdir > +mkdir $testdir2 > +mkdir $testdir2/hello1d > +mkdir $testdir2/hello2d > +echo 1 > $testdir2/hello1f > +echo 2 > $testdir2/hello2f > > cleanup() { > chmod 777 $testdir/noread > - rm -fr $testdir $testout > + rm -fr $testdir $testdir2 $testout > } > > trap cleanup 0 HUP INT QUIT TERM > @@ -815,6 +821,41 @@ if test $failed -ne 0; then > result=1 > fi > > +# Test that if the specified glob ends with a slash then only directories are > +# matched. > +# First run with the glob that has no slash to demonstrate presence of a slash > +# makes a difference. > +failed=0 > +${test_program_prefix} \ > +${common_objpfx}posix/globtest "$testdir2" "hello*" | > +sort > $testout > +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 > +`hello1d' > +`hello1f' > +`hello2d' > +`hello2f' > +EOF > + > +if test $failed -ne 0; then > + echo "pattern+meta test failed" >> $logfile > + result=1 > +fi > + > +# The same pattern+meta test with a slash this time. > +failed=0 > +${test_program_prefix} \ > +${common_objpfx}posix/globtest "$testdir2" "hello*/" | > +sort > $testout > +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 > +`hello1d/' > +`hello2d/' > +EOF > + > +if test $failed -ne 0; then > + echo "pattern+meta+slash test failed" >> $logfile > + result=1 > +fi > + > if test $result -eq 0; then > echo "All OK." > $logfile > fi Thanks for the test. It does a good job of motivating the change. Thanks and hope that helps, Jonathan
On 11/28/2017 01:08 PM, Dmitry Goncharov wrote: > If pattern has a trailing slash match directories only. > This patch fixes [BZ #22513]. I do not observe the bug on Fedora 27, which uses glibc 2.26-16, so presumably this bug was introduced in the recent merges from Gnulib. However, I not observe the bug with Gnulib master lib/glob.c either. Could you explain what introduced the bug, e.g., by isolating the Glibc commit that introduced it? Perhaps we botched the merge, and if so we should fix the botch rather than work around the bug with some extra code.
On Tue, Nov 28, 2017 at 05:58:12PM -0800, Paul Eggert wrote: > On 11/28/2017 01:08 PM, Dmitry Goncharov wrote: > > If pattern has a trailing slash match directories only. > > This patch fixes [BZ #22513]. > > I do not observe the bug on Fedora 27, which uses glibc 2.26-16, so > presumably this bug was introduced in the recent merges from Gnulib. > However, I not observe the bug with Gnulib master lib/glob.c either. > Could you explain what introduced the bug, e.g., by isolating the Glibc > commit that introduced it? Perhaps we botched the merge, and if so we > should fix the botch rather than work around the bug with some extra code. > commit 1cab5444231a4a1fab9c3abb107d22af4eb09327 (tag: cvs/libc-ud-971031) Author: Ulrich Drepper <drepper@redhat.com> Date: Fri Oct 31 22:55:02 1997 +0000 introduced the following piece of code among others +#ifdef HAVE_D_TYPE + /* If we shall match only directories use the information + provided by the dirent if possible. */ + if ((flags & GLOB_ONLYDIR) + && d->d_type != DT_UNKNOWN && d->d_type != DT_DIR) + continue; +#endif + if (fnmatch (pattern, name, (!(flags & GLOB_PERIOD) ? FNM_PERIOD : 0) | ((flags & GLOB_NOESCAPE) ? FNM_NOESCAPE : 0) As long as your filesystem initializes d->d_type you won't see the issue. When d->d_type is DT_UNKNOWN then d may not be a directory. regards, Dmitry
Dmitry Goncharov wrote: > Author: Ulrich Drepper<drepper@redhat.com> > Date: Fri Oct 31 22:55:02 1997 +0000 Ah, so this is a longstanding bug in glob, and was not introduced by the recent merge from Gnulib. That's a relief. > As long as your filesystem initializes d->d_type you won't see the issue. > When d->d_type is DT_UNKNOWN then d may not be a directory. Thanks for clarifying. Now that I understand it better, though, I still see a problem. As noted in the Glibc manual here: https://www.gnu.org/software/libc/manual/html_node/More-Flags-for-Globbing.html#index-GLOB_005fONLYDIR GLOB_ONLYDIR is merely an efficiency flag: it means "do not return entries that can easily be shown to be non-directories". Your patch would change the semantics of GLOB_ONLYDIR so that it means "return only entries that are known to be directories", which differs from the Glibc documentation. The bug here is not in the implementation of GLOB_ONLYDIR; it is in the part of glob that calls itself recursively with GLOB_ONLYDIR, and which expects only directories (or symlinks to directories) to be matched. This issue came up in September, here: https://sourceware.org/ml/libc-alpha/2017-09/msg00888.html
On Wed, Nov 29, 2017 at 1:35 AM, Paul Eggert <eggert@cs.ucla.edu> wrote: >> >> Thanks for clarifying. Now that I understand it better, though, I still see a problem. As noted in the Glibc manual here: >> >> https://www.gnu.org/software/libc/manual/html_node/More-Flags-for-Globbing.html#index-GLOB_005fONLYDIR >> >> GLOB_ONLYDIR is merely an efficiency flag: it means "do not return entries that can easily be shown to be non-directories". Your patch would change the semantics of GLOB_ONLYDIR so that it means "return only entries that are known to be directories", which differs from the Glibc documentation. >> >> >> > You are right. There is indeed this semantics. The patch is compatible with "do not return entries that can easily be shown to be non-directories". One option is we can decide the patch is compatible and apply it. If the existing semantics has to be preserved then we can introduce another flag and use the new flag instead of GLOB_ONLYDIR in if (pattern[0] && pattern[strlen (pattern) - 1] == '/') flags |= GLOB_ONLYDIR; and everywhere else where required. regards, Dmitry
On Tue, Nov 28, 2017 at 5:04 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > (+cc: bug-gnulib@, since they share this code) > If I understand correctly, this treats DT_UNKNOWN like DT_DIR when stat > indicates that it is a directory. The patch checks if an entry is really a dir if type is DT_UNKNOWN. > > What should happen in the DT_LNK case? Should the same logic trip for > it as well so we can distinguish between a symlink to a directory and > other symlinks? You are right. i'll add a test and post another patch. regards, Dmitry
On 11/29/2017 01:50 PM, Dmitry Goncharov wrote: > One option is we can decide the patch is compatible and apply it. It's compatible only if we ignore performance. But I don't think that would be right: I expect that some callers expect GLOB_ONLYDIR to be fast (because that's what the Glibc documentation says), and that they will be disappointed by this performance regression. > If the existing semantics has to be preserved then we can introduce > another flag and use the new flag instead of GLOB_ONLYDIR in > > if (pattern[0] && pattern[strlen (pattern) - 1] == '/') > flags |= GLOB_ONLYDIR; That would be a better approach. Another possibility might be to change the last assignment to: flags |= GLOB_ONLYDIR | GLOB_MARK; and then at the end, filter out all matches that aren't marked with trailing '/'. This would avoid creating a new GLOB_XXX option and would probably be easier to implement.
On Tue, Nov 28, 2017 at 02:04:53PM -0800, Jonathan Nieder wrote: > > What should happen in the DT_LNK case? Should the same logic trip for > it as well so we can distinguish between a symlink to a directory and > other symlinks? > The filesystems that i tested on all return DT_UNKNOWN for a symlink or hardlink. Even when they return proper type for a file or directory. However, if the filesystem ever sets type to DT_LNK glob needs to treat it like DT_UNKNOWN. regards, Dmitry diff --git a/ChangeLog b/ChangeLog index ffeb208132..fb76fc1415 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2017-11-28 Dmitry Goncharov <dgoncharov@users.sf.net> + + [BZ #22513] + * posix/glob.c (glob_in_dir): Make pattern with a trailing slash + match directores only. + * posix/globtest.sh: Add tests. + 2017-11-13 Florian Weimer <fweimer@redhat.com> * support/next_to_fault.h, support/next_to_fault.c: New files. diff --git a/posix/glob.c b/posix/glob.c index cb39779d07..47e67907cd 100644 --- a/posix/glob.c +++ b/posix/glob.c @@ -1342,7 +1342,33 @@ glob_in_dir (const char *pattern, const char *directory, int flags, if (flags & GLOB_ONLYDIR) switch (readdir_result_type (d)) { - case DT_DIR: case DT_LNK: case DT_UNKNOWN: break; + case DT_DIR: break; + case DT_LNK: case DT_UNKNOWN: + { + int dir; + size_t namlen = strlen (d.name); + size_t fullsize; + bool alloca_fullname + = (! size_add_wrapv (dirlen + 1, namlen + 1, &fullsize) + && glob_use_alloca (alloca_used, fullsize)); + char *fullname; + if (alloca_fullname) + fullname = alloca_account (fullsize, alloca_used); + else + { + fullname = malloc (fullsize); + if (fullname == NULL) + return GLOB_NOSPACE; + } + mempcpy (mempcpy (mempcpy (fullname, directory, dirlen), + "/", 1), + d.name, namlen + 1); + dir = is_dir (fullname, flags, pglob); + if (__glibc_unlikely (!alloca_fullname)) + free (fullname); + if (dir) + break; + } default: continue; } diff --git a/posix/globtest.sh b/posix/globtest.sh index 73f7ae31cc..4a062ea507 100755 --- a/posix/globtest.sh +++ b/posix/globtest.sh @@ -43,13 +43,22 @@ export LC_ALL # Create the arena testdir=${common_objpfx}posix/globtest-dir +testdir2=${common_objpfx}posix/globtest-dir2 testout=${common_objpfx}posix/globtest-out rm -rf $testdir $testout mkdir $testdir +mkdir $testdir2 +mkdir $testdir2/hello1d +ln -s $testdir2/hello1d $testdir2/hello1ds +mkdir $testdir2/hello2d +echo 1 > $testdir2/hello1f +ln -s $testdir2/hello1f $testdir2/hello1fs +echo 2 > $testdir2/hello2f +ln $testdir2/hello2f $testdir2/hello2fl cleanup() { chmod 777 $testdir/noread - rm -fr $testdir $testout + rm -fr $testdir $testdir2 $testout } trap cleanup 0 HUP INT QUIT TERM @@ -815,6 +824,45 @@ if test $failed -ne 0; then result=1 fi +# Test that if the specified glob ends with a slash then only directories are +# matched. +# First run with the glob that has no slash to demonstrate presence of a slash +# makes a difference. +failed=0 +${test_program_prefix} \ +${common_objpfx}posix/globtest "$testdir2" "hello*" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`hello1d' +`hello1ds' +`hello1f' +`hello1fs' +`hello2d' +`hello2f' +`hello2fl' +EOF + +if test $failed -ne 0; then + echo "pattern+meta test failed" >> $logfile + result=1 +fi + +# The same pattern+meta test with a slash this time. +failed=0 +${test_program_prefix} \ +${common_objpfx}posix/globtest "$testdir2" "hello*/" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`hello1d/' +`hello1ds/' +`hello2d/' +EOF + +if test $failed -ne 0; then + echo "pattern+meta+slash test failed" >> $logfile + result=1 +fi + if test $result -eq 0; then echo "All OK." > $logfile fi
On Wed, Nov 29, 2017 at 03:01:46PM -0800, Paul Eggert wrote: > On 11/29/2017 01:50 PM, Dmitry Goncharov wrote: > > One option is we can decide the patch is compatible and apply it. > > It's compatible only if we ignore performance. But I don't think that > would be right: I expect that some callers expect GLOB_ONLYDIR to be > fast (because that's what the Glibc documentation says), and that they > will be disappointed by this performance regression. Paul, i am missing where you see the performance hit. The current contract makes the caller check after the library by calling stat on each entry. A stricter contract like "if GLOB_ONLDIR is set then glob returns only directories" is more efficient on the filesystems which set the type. On the filesystems which don't set the type stat'd be called once for each entry by the library rather than by the caller which keeps the performance intact. > > > If the existing semantics has to be preserved then we can introduce > > another flag and use the new flag instead of GLOB_ONLYDIR in > > > > if (pattern[0] && pattern[strlen (pattern) - 1] == '/') > > flags |= GLOB_ONLYDIR; > > That would be a better approach. Another possibility might be to change > the last assignment to: > > flags |= GLOB_ONLYDIR | GLOB_MARK; > > and then at the end, filter out all matches that aren't marked with > trailing '/'. This would avoid creating a new GLOB_XXX option and would > probably be easier to implement. Thansk for the pointer. Will have a look. regards, Dmitry
Dmitry Goncharov wrote: > Paul, i am missing where you see the performance hit. > The current contract makes the caller check after the library by calling stat > on each entry. No, these callers need not call stat. glob itself doesn't call stat, when it calls itself recursively with this flag. Instead, glob simply charges ahead and gets an ENOTDIR (or whatever( in the next level of recursion. For this sort of usage, it is a win for glob to filter out directories if that is easy (because there is then no need even to issue the syscalls that result in ENOTDIR), but it can be a loss for glob to filter out directories via stat calls.
On Wed, Nov 29, 2017 at 03:01:46PM -0800, Paul Eggert wrote: > That would be a better approach. Another possibility might be to change > the last assignment to: > > flags |= GLOB_ONLYDIR | GLOB_MARK; > > and then at the end, filter out all matches that aren't marked with > trailing '/'. This would avoid creating a new GLOB_XXX option and would > probably be easier to implement. > Please have a look at this implementation of your idea. regards, Dmitry diff --git a/ChangeLog b/ChangeLog index ffeb208132..1b7e3984d2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2017-11-28 Dmitry Goncharov <dgoncharov@users.sf.net> + + [BZ #22513] + * posix/glob.c (glob_in_dir): Make pattern with a trailing slash + match directores only. + * posix/globtest.sh: Add tests. + * posix/globtest.c: Print gl_pathv before errors. + 2017-11-13 Florian Weimer <fweimer@redhat.com> * support/next_to_fault.h, support/next_to_fault.c: New files. diff --git a/posix/glob.c b/posix/glob.c index cb39779d07..10756f1f92 100644 --- a/posix/glob.c +++ b/posix/glob.c @@ -1124,8 +1124,11 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), { /* Append slashes to directory names. */ size_t i; + int filter; + filter = 0; for (i = oldcount; i < pglob->gl_pathc + pglob->gl_offs; ++i) + { if (is_dir (pglob->gl_pathv[i], flags, pglob)) { size_t len = strlen (pglob->gl_pathv[i]) + 2; @@ -1140,6 +1143,77 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), strcpy (&new[len - 2], "/"); pglob->gl_pathv[i] = new; } + else if (flags & GLOB_ONLYDIR) + { + /* Either the caller specified GLOB_MARK and GLOB_ONLYDIR or the + caller specified the pattern that has a trailing slash. + Filter out everything, but directories. */ + free (pglob->gl_pathv[i]); + pglob->gl_pathv[i] = NULL; + filter = 1; + } + } + if (filter) + { + /* After the first pass above gl_pathv has full elements (directories) + and empty elements. + This algorithm partitions gl_pathv to have all full elements in the + beginning followed by all empty elements at the end and then sets + gl_pathc to the number of the full elements. + + Assume the case of 2 elements offsetted by the caller and 2 more matches + found by prior calls to glob with GLOB_APPEND and 3 more matches found + this time. + offs = 2, oldcount = 4, pathc = 5. + + Initial position. + gl_pathv e f + | | | + v v v + [ ][ ] [ ][ ][ ][ ][ ] + <----><--------------> + offs pathc + <-----------> + oldcount + From this position e will be advancing to the next empty element + and f will be backtracking to the last full element. At each + iteration contents of e and f will be swapped. + */ + + size_t n; + char **e, **f; + n = pglob->gl_pathc + pglob->gl_offs; + assert (oldcount < n); /* Otherwise filter'd be 0. */ + /* f points at the last element of gl_pathv. */ + f = pglob->gl_pathv + n - 1; + /* e points at gl_pathv[oldcount]. */ + e = pglob->gl_pathv + oldcount; + assert (f >= e); + assert (f >= pglob->gl_pathv); /* Otherwise filter'd be 0. */ + for (;;) + { + assert (f >= pglob->gl_pathv); + assert (e < pglob->gl_pathv + n); + /* Advance e to point at the first empty element. */ + while (*e != NULL && e < f) + ++e; + if (e >= f) + break; + /* Backtrack f to point at the last full element. */ + while (*f == NULL && f > e) + --f; + if (e >= f) + break; + assert (f > e); + *e = *f; + *f = NULL; + } + /* If there are full elements in gl_pathv then f points at the last + one. Otherwise f points at gl_pathv[oldcount]. */ + pglob->gl_pathc = f - pglob->gl_pathv - pglob->gl_offs; + if (pglob->gl_pathc == 0) + retval = GLOB_NOMATCH; + } } if (!(flags & GLOB_NOSORT)) diff --git a/posix/globtest.c b/posix/globtest.c index 878ae33044..0a94550d26 100644 --- a/posix/globtest.c +++ b/posix/globtest.c @@ -93,14 +93,6 @@ main (int argc, char *argv[]) glob_flags |= GLOB_APPEND; } - /* Was there an error? */ - if (i == GLOB_NOSPACE) - puts ("GLOB_NOSPACE"); - else if (i == GLOB_ABORTED) - puts ("GLOB_ABORTED"); - else if (i == GLOB_NOMATCH) - puts ("GLOB_NOMATCH"); - /* If we set an offset, fill in the first field. (Unless glob() has filled it in already - which is an error) */ if ((glob_flags & GLOB_DOOFFS) && g.gl_pathv[0] == NULL) @@ -109,10 +101,25 @@ main (int argc, char *argv[]) /* Print out the names. Unless otherwise specified, qoute them. */ if (g.gl_pathv) { - for (i = 0; i < g.gl_offs + g.gl_pathc; ++i) + int k; + for (k = 0; k < g.gl_offs + g.gl_pathc; ++k) printf ("%s%s%s\n", quotes ? "`" : "", - g.gl_pathv[i] ? g.gl_pathv[i] : "(null)", + g.gl_pathv[k] ? g.gl_pathv[k] : "(null)", quotes ? "'" : ""); + } + + /* Was there an error? + * Printing the error after printing g.gl_pathv simplifies the test suite, + * because when -o is used 'abc' is always printed first regardless of + * whether the pattern matches anyting. + */ + if (i == GLOB_NOSPACE) + puts ("GLOB_NOSPACE"); + else if (i == GLOB_ABORTED) + puts ("GLOB_ABORTED"); + else if (i == GLOB_NOMATCH) + puts ("GLOB_NOMATCH"); + return 0; } diff --git a/posix/globtest.sh b/posix/globtest.sh index 73f7ae31cc..bc86fcc232 100755 --- a/posix/globtest.sh +++ b/posix/globtest.sh @@ -43,13 +43,22 @@ export LC_ALL # Create the arena testdir=${common_objpfx}posix/globtest-dir +tdir2=${common_objpfx}posix/globtest-dir2 testout=${common_objpfx}posix/globtest-out -rm -rf $testdir $testout +rm -rf $testdir $tdir2 $testout mkdir $testdir +mkdir $tdir2 +mkdir $tdir2/hello1d +ln -s $tdir2/hello1d $tdir2/hello1ds +mkdir $tdir2/hello2d +echo 1 > $tdir2/hello1f +ln -s $tdir2/hello1f $tdir2/hello1fs +echo 2 > $tdir2/hello2f +ln $tdir2/hello2f $tdir2/hello2fl cleanup() { chmod 777 $testdir/noread - rm -fr $testdir $testout + rm -fr $testdir $tdir2 $testout } trap cleanup 0 HUP INT QUIT TERM @@ -74,6 +83,30 @@ echo 1_1 > $testdir/dir1/file1_1 echo 1_2 > $testdir/dir1/file1_2 ln -fs dir1 $testdir/link1 +trailing_slash_test() +{ + local dir="$1" + local pattern="$2" + local expected="$3" + failed=0 + ${test_program_prefix} \ + ${common_objpfx}posix/globtest -q "$dir" "$pattern" | sort -fd > $testout + echo -e "$expected" | $CMP - $testout >> $logfile || failed=1 + if test $failed -ne 0; then + echo "$pattern test failed" >> $logfile + result=1 + fi + + failed=0 + ${test_program_prefix} \ + ${common_objpfx}posix/globtest -qo "$dir" "$pattern" | sort -fd > $testout + echo -e "abc\n$expected" | $CMP - $testout >> $logfile || failed=1 + if test $failed -ne 0; then + echo "$pattern with dooffs test failed" >> $logfile + result=1 + fi +} + # Run some tests. result=0 rm -f $logfile @@ -692,6 +725,22 @@ if test $failed -ne 0; then result=1 fi +# Try multiple patterns (GLOB_APPEND) with offset (GLOB_DOOFFS) +# The pattern has a trailing slash which eliminates files in the +# subdirectories. +failed=0 +${test_program_prefix} \ +${common_objpfx}posix/globtest -o "$testdir" "file1" "*/*/" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`abc' +`file1' +EOF +if test $failed -ne 0; then + echo "GLOB_APPEND2 with slash test failed" >> $logfile + result=1 +fi + # Test NOCHECK with non-existing file in subdir. failed=0 ${test_program_prefix} \ @@ -815,6 +864,93 @@ if test $failed -ne 0; then result=1 fi +# This code tests the algo that filters out non directories when the pattern +# has a trailing slash. +# There are at least the following cases +# - The pattern matches files and directories. +# - The pattern matches nothing. +# - The pattern matches only files. +# - The pattern matches 1 file. +# - The pattern matches only directories. +# - The pattern matches 1 directory. +# Each test is run twice, with and without a trailing slash to demonstrate +# the differentce that the slash makes. +# Each test is also run from the target directory by doing chdir and from the +# current directory by specifying the full path in the pattern. +# Each test is also run with and without dooffs. + +# The pattern matches files and directories. +pattern='hello*' +expected="hello1d\nhello1ds\nhello1f\nhello1fs\nhello2d\nhello2f\nhello2fl" +trailing_slash_test "$tdir2" "$pattern" "$expected" + +expected="hello1d/\nhello1ds/\nhello2d/" +trailing_slash_test "$tdir2" "$pattern/" "$expected" + +pattern="$tdir2/hello*" +expected="$tdir2/hello1d\n$tdir2/hello1ds\n$tdir2/hello1f\n\ +$tdir2/hello1fs\n$tdir2/hello2d\n$tdir2/hello2f\n$tdir2/hello2fl" +trailing_slash_test . "$pattern" "$expected" + +expected="$tdir2/hello1d/\n$tdir2/hello1ds/\n$tdir2/hello2d/" +trailing_slash_test . "$pattern/" "$expected" + +# The pattern matches nothing. +pattern='nomatch*' +trailing_slash_test "$tdir2" "$pattern" GLOB_NOMATCH +trailing_slash_test "$tdir2" "$pattern/" GLOB_NOMATCH +trailing_slash_test . "$pattern" GLOB_NOMATCH +trailing_slash_test . "$pattern/" GLOB_NOMATCH + +# The pattern matches only files. +pattern='hello?f*' +expected="hello1f\nhello1fs\nhello2f\nhello2fl" +trailing_slash_test "$tdir2" "$pattern" "$expected" +trailing_slash_test "$tdir2" "$pattern/" GLOB_NOMATCH + +pattern="$tdir2/hello?f*" +expected="$tdir2/hello1f\n$tdir2/hello1fs\n$tdir2/hello2f\n$tdir2/hello2fl" +trailing_slash_test . "$pattern" "$expected" +trailing_slash_test . "$pattern/" GLOB_NOMATCH + +# The pattern matches only 1 file. +pattern='hello*fl' +expected="hello2fl" +trailing_slash_test "$tdir2" "$pattern" "$expected" +trailing_slash_test "$tdir2" "$pattern/" GLOB_NOMATCH + +pattern="$tdir2/hello*fl" +expected="$tdir2/hello2fl" +trailing_slash_test . "$pattern" "$expected" +trailing_slash_test . "$pattern/" GLOB_NOMATCH + +# The pattern matches only directores. +pattern='hello?d*' +expected="hello1d\nhello1ds\nhello2d" +trailing_slash_test "$tdir2" "$pattern" "$expected" + +expected="hello1d/\nhello1ds/\nhello2d/" +trailing_slash_test "$tdir2" "$pattern/" "$expected" + +pattern="$tdir2/hello?d*" +expected="$tdir2/hello1d\n$tdir2/hello1ds\n$tdir2/hello2d" +trailing_slash_test . "$pattern" "$expected" + +expected="$tdir2/hello1d/\n$tdir2/hello1ds/\n$tdir2/hello2d/" +trailing_slash_test . "$pattern/" "$expected" + +# The pattern matches only 1 directory. +pattern='hello*ds' +expected="hello1ds" +trailing_slash_test "$tdir2" "$pattern" "$expected" +trailing_slash_test "$tdir2" "$pattern/" "$expected/" + +pattern="$tdir2/hello*ds" +expected="$tdir2/hello1ds" +trailing_slash_test . "$pattern" "$expected" +trailing_slash_test . "$pattern/" "$expected/" + + if test $result -eq 0; then echo "All OK." > $logfile fi
Dmitry Goncharov wrote: >> flags |= GLOB_ONLYDIR | GLOB_MARK; >> >> and then at the end, filter out all matches that aren't marked with >> trailing '/'. This would avoid creating a new GLOB_XXX option and would >> probably be easier to implement. >> > Please have a look at this implementation of your idea. I'm not quite following how it's an implementation, since I don't see where it does anything like "flags |= GLOB_ONLYDIR | GLOB_MARK;". Maybe there's another part of the patch you're missing? The variable "filter" is a boolean and should be of type bool. That comment and code look over-complicated. Can't you simply copy nonnull entries in-place, in a single pass? That way, the filtered order will be the same as the original order.
On Mon, Dec 4, 2017 at 3:01 AM, Paul Eggert <eggert@cs.ucla.edu> wrote: > Dmitry Goncharov wrote: >>> >>> flags |= GLOB_ONLYDIR | GLOB_MARK; >>> >>> and then at the end, filter out all matches that aren't marked with >>> trailing '/'. This would avoid creating a new GLOB_XXX option and would >>> probably be easier to implement. >>> >> Please have a look at this implementation of your idea. > > > I'm not quite following how it's an implementation, since I don't see where > it does anything like "flags |= GLOB_ONLYDIR | GLOB_MARK;". Maybe there's > another part of the patch you're missing? glob sets GLOB_MARK when it calls itself recursively when the pattern has a trailing slash. > > The variable "filter" is a boolean and should be of type bool. sure > > That comment and code look over-complicated. i agree. Will remove the comment. > Can't you simply copy nonnull entries in-place, in a single pass? The first pass calls is_dir and frees and resets files. Do you mean to also do copying in this very pass? > That way, the filtered order will be the same as the original order. The stable algorithm results in more iterations and more copying. Are you sure? regards, Dmitry
On 12/04/2017 07:46 AM, Dmitry Goncharov wrote: >> Can't you simply copy nonnull entries in-place, in a single pass? > The first pass calls is_dir and frees and resets files. Do you mean to > also do copying in this very pass? I would think that would be easy, yes. > >> That way, the filtered order will be the same as the original order. > The stable algorithm results in more iterations and more copying. Are you sure? Sorry, I'm not seeing why an extra iteration would be needed. I expect users would prefer that this bug-fix does not alter the order of returned values.
On Mon, Dec 04, 2017 at 11:32:06AM -0800, Paul Eggert wrote: > On 12/04/2017 07:46 AM, Dmitry Goncharov wrote: > > The stable algorithm results in more iterations and more copying. Are you sure? > > Sorry, I'm not seeing why an extra iteration would be needed. I expect > users would prefer that this bug-fix does not alter the order of > returned values. i was comparing 2 pass algorithms. Please have a look at this one pass implementaion. regards, Dmitry diff --git a/ChangeLog b/ChangeLog index ffeb208132..1b7e3984d2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2017-11-28 Dmitry Goncharov <dgoncharov@users.sf.net> + + [BZ #22513] + * posix/glob.c (glob_in_dir): Make pattern with a trailing slash + match directores only. + * posix/globtest.sh: Add tests. + * posix/globtest.c: Print gl_pathv before errors. + 2017-11-13 Florian Weimer <fweimer@redhat.com> * support/next_to_fault.h, support/next_to_fault.c: New files. diff --git a/posix/glob.c b/posix/glob.c index cb39779d07..c63d628c59 100644 --- a/posix/glob.c +++ b/posix/glob.c @@ -1123,9 +1123,10 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), if (flags & GLOB_MARK) { /* Append slashes to directory names. */ - size_t i; + size_t i, e; - for (i = oldcount; i < pglob->gl_pathc + pglob->gl_offs; ++i) + for (i = e = oldcount; i < pglob->gl_pathc + pglob->gl_offs; ++i) + { if (is_dir (pglob->gl_pathv[i], flags, pglob)) { size_t len = strlen (pglob->gl_pathv[i]) + 2; @@ -1138,8 +1139,26 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int), goto out; } strcpy (&new[len - 2], "/"); + if (pglob->gl_pathv[e] == NULL) + { + pglob->gl_pathv[e++] = new; + pglob->gl_pathv[i] = NULL; + } + else pglob->gl_pathv[i] = new; } + else if (flags & GLOB_ONLYDIR) + { + free (pglob->gl_pathv[i]); + pglob->gl_pathv[i] = NULL; + if (pglob->gl_pathv[e] != NULL) + e = i; + } + } + if (pglob->gl_pathv[e] == NULL) + pglob->gl_pathc = e - pglob->gl_offs; + if (pglob->gl_pathc == 0) + retval = GLOB_NOMATCH; } if (!(flags & GLOB_NOSORT)) diff --git a/posix/globtest.c b/posix/globtest.c index 878ae33044..0a94550d26 100644 --- a/posix/globtest.c +++ b/posix/globtest.c @@ -93,14 +93,6 @@ main (int argc, char *argv[]) glob_flags |= GLOB_APPEND; } - /* Was there an error? */ - if (i == GLOB_NOSPACE) - puts ("GLOB_NOSPACE"); - else if (i == GLOB_ABORTED) - puts ("GLOB_ABORTED"); - else if (i == GLOB_NOMATCH) - puts ("GLOB_NOMATCH"); - /* If we set an offset, fill in the first field. (Unless glob() has filled it in already - which is an error) */ if ((glob_flags & GLOB_DOOFFS) && g.gl_pathv[0] == NULL) @@ -109,10 +101,25 @@ main (int argc, char *argv[]) /* Print out the names. Unless otherwise specified, qoute them. */ if (g.gl_pathv) { - for (i = 0; i < g.gl_offs + g.gl_pathc; ++i) + int k; + for (k = 0; k < g.gl_offs + g.gl_pathc; ++k) printf ("%s%s%s\n", quotes ? "`" : "", - g.gl_pathv[i] ? g.gl_pathv[i] : "(null)", + g.gl_pathv[k] ? g.gl_pathv[k] : "(null)", quotes ? "'" : ""); + } + + /* Was there an error? + * Printing the error after printing g.gl_pathv simplifies the test suite, + * because when -o is used 'abc' is always printed first regardless of + * whether the pattern matches anyting. + */ + if (i == GLOB_NOSPACE) + puts ("GLOB_NOSPACE"); + else if (i == GLOB_ABORTED) + puts ("GLOB_ABORTED"); + else if (i == GLOB_NOMATCH) + puts ("GLOB_NOMATCH"); + return 0; } diff --git a/posix/globtest.sh b/posix/globtest.sh index 73f7ae31cc..bc86fcc232 100755 --- a/posix/globtest.sh +++ b/posix/globtest.sh @@ -43,13 +43,22 @@ export LC_ALL # Create the arena testdir=${common_objpfx}posix/globtest-dir +tdir2=${common_objpfx}posix/globtest-dir2 testout=${common_objpfx}posix/globtest-out -rm -rf $testdir $testout +rm -rf $testdir $tdir2 $testout mkdir $testdir +mkdir $tdir2 +mkdir $tdir2/hello1d +ln -s $tdir2/hello1d $tdir2/hello1ds +mkdir $tdir2/hello2d +echo 1 > $tdir2/hello1f +ln -s $tdir2/hello1f $tdir2/hello1fs +echo 2 > $tdir2/hello2f +ln $tdir2/hello2f $tdir2/hello2fl cleanup() { chmod 777 $testdir/noread - rm -fr $testdir $testout + rm -fr $testdir $tdir2 $testout } trap cleanup 0 HUP INT QUIT TERM @@ -74,6 +83,30 @@ echo 1_1 > $testdir/dir1/file1_1 echo 1_2 > $testdir/dir1/file1_2 ln -fs dir1 $testdir/link1 +trailing_slash_test() +{ + local dir="$1" + local pattern="$2" + local expected="$3" + failed=0 + ${test_program_prefix} \ + ${common_objpfx}posix/globtest -q "$dir" "$pattern" | sort -fd > $testout + echo -e "$expected" | $CMP - $testout >> $logfile || failed=1 + if test $failed -ne 0; then + echo "$pattern test failed" >> $logfile + result=1 + fi + + failed=0 + ${test_program_prefix} \ + ${common_objpfx}posix/globtest -qo "$dir" "$pattern" | sort -fd > $testout + echo -e "abc\n$expected" | $CMP - $testout >> $logfile || failed=1 + if test $failed -ne 0; then + echo "$pattern with dooffs test failed" >> $logfile + result=1 + fi +} + # Run some tests. result=0 rm -f $logfile @@ -692,6 +725,22 @@ if test $failed -ne 0; then result=1 fi +# Try multiple patterns (GLOB_APPEND) with offset (GLOB_DOOFFS) +# The pattern has a trailing slash which eliminates files in the +# subdirectories. +failed=0 +${test_program_prefix} \ +${common_objpfx}posix/globtest -o "$testdir" "file1" "*/*/" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`abc' +`file1' +EOF +if test $failed -ne 0; then + echo "GLOB_APPEND2 with slash test failed" >> $logfile + result=1 +fi + # Test NOCHECK with non-existing file in subdir. failed=0 ${test_program_prefix} \ @@ -815,6 +864,93 @@ if test $failed -ne 0; then result=1 fi +# This code tests the algo that filters out non directories when the pattern +# has a trailing slash. +# There are at least the following cases +# - The pattern matches files and directories. +# - The pattern matches nothing. +# - The pattern matches only files. +# - The pattern matches 1 file. +# - The pattern matches only directories. +# - The pattern matches 1 directory. +# Each test is run twice, with and without a trailing slash to demonstrate +# the differentce that the slash makes. +# Each test is also run from the target directory by doing chdir and from the +# current directory by specifying the full path in the pattern. +# Each test is also run with and without dooffs. + +# The pattern matches files and directories. +pattern='hello*' +expected="hello1d\nhello1ds\nhello1f\nhello1fs\nhello2d\nhello2f\nhello2fl" +trailing_slash_test "$tdir2" "$pattern" "$expected" + +expected="hello1d/\nhello1ds/\nhello2d/" +trailing_slash_test "$tdir2" "$pattern/" "$expected" + +pattern="$tdir2/hello*" +expected="$tdir2/hello1d\n$tdir2/hello1ds\n$tdir2/hello1f\n\ +$tdir2/hello1fs\n$tdir2/hello2d\n$tdir2/hello2f\n$tdir2/hello2fl" +trailing_slash_test . "$pattern" "$expected" + +expected="$tdir2/hello1d/\n$tdir2/hello1ds/\n$tdir2/hello2d/" +trailing_slash_test . "$pattern/" "$expected" + +# The pattern matches nothing. +pattern='nomatch*' +trailing_slash_test "$tdir2" "$pattern" GLOB_NOMATCH +trailing_slash_test "$tdir2" "$pattern/" GLOB_NOMATCH +trailing_slash_test . "$pattern" GLOB_NOMATCH +trailing_slash_test . "$pattern/" GLOB_NOMATCH + +# The pattern matches only files. +pattern='hello?f*' +expected="hello1f\nhello1fs\nhello2f\nhello2fl" +trailing_slash_test "$tdir2" "$pattern" "$expected" +trailing_slash_test "$tdir2" "$pattern/" GLOB_NOMATCH + +pattern="$tdir2/hello?f*" +expected="$tdir2/hello1f\n$tdir2/hello1fs\n$tdir2/hello2f\n$tdir2/hello2fl" +trailing_slash_test . "$pattern" "$expected" +trailing_slash_test . "$pattern/" GLOB_NOMATCH + +# The pattern matches only 1 file. +pattern='hello*fl' +expected="hello2fl" +trailing_slash_test "$tdir2" "$pattern" "$expected" +trailing_slash_test "$tdir2" "$pattern/" GLOB_NOMATCH + +pattern="$tdir2/hello*fl" +expected="$tdir2/hello2fl" +trailing_slash_test . "$pattern" "$expected" +trailing_slash_test . "$pattern/" GLOB_NOMATCH + +# The pattern matches only directores. +pattern='hello?d*' +expected="hello1d\nhello1ds\nhello2d" +trailing_slash_test "$tdir2" "$pattern" "$expected" + +expected="hello1d/\nhello1ds/\nhello2d/" +trailing_slash_test "$tdir2" "$pattern/" "$expected" + +pattern="$tdir2/hello?d*" +expected="$tdir2/hello1d\n$tdir2/hello1ds\n$tdir2/hello2d" +trailing_slash_test . "$pattern" "$expected" + +expected="$tdir2/hello1d/\n$tdir2/hello1ds/\n$tdir2/hello2d/" +trailing_slash_test . "$pattern/" "$expected" + +# The pattern matches only 1 directory. +pattern='hello*ds' +expected="hello1ds" +trailing_slash_test "$tdir2" "$pattern" "$expected" +trailing_slash_test "$tdir2" "$pattern/" "$expected/" + +pattern="$tdir2/hello*ds" +expected="$tdir2/hello1ds" +trailing_slash_test . "$pattern" "$expected" +trailing_slash_test . "$pattern/" "$expected/" + + if test $result -eq 0; then echo "All OK." > $logfile fi
diff --git a/posix/glob.c b/posix/glob.c index cb39779d07..78873d83c6 100644 --- a/posix/glob.c +++ b/posix/glob.c @@ -1342,7 +1342,33 @@ glob_in_dir (const char *pattern, const char *directory, int flags, if (flags & GLOB_ONLYDIR) switch (readdir_result_type (d)) { - case DT_DIR: case DT_LNK: case DT_UNKNOWN: break; + case DT_DIR: case DT_LNK: break; + case DT_UNKNOWN: + { + int dir; + size_t namlen = strlen (d.name); + size_t fullsize; + bool alloca_fullname + = (! size_add_wrapv (dirlen + 1, namlen + 1, &fullsize) + && glob_use_alloca (alloca_used, fullsize)); + char *fullname; + if (alloca_fullname) + fullname = alloca_account (fullsize, alloca_used); + else + { + fullname = malloc (fullsize); + if (fullname == NULL) + return GLOB_NOSPACE; + } + mempcpy (mempcpy (mempcpy (fullname, directory, dirlen), + "/", 1), + d.name, namlen + 1); + dir = is_dir (fullname, flags, pglob); + if (__glibc_unlikely (!alloca_fullname)) + free (fullname); + if (dir) + break; + } default: continue; } diff --git a/posix/globtest.sh b/posix/globtest.sh index 73f7ae31cc..4f0c03715a 100755 --- a/posix/globtest.sh +++ b/posix/globtest.sh @@ -43,13 +43,19 @@ export LC_ALL # Create the arena testdir=${common_objpfx}posix/globtest-dir +testdir2=${common_objpfx}posix/globtest-dir2 testout=${common_objpfx}posix/globtest-out rm -rf $testdir $testout mkdir $testdir +mkdir $testdir2 +mkdir $testdir2/hello1d +mkdir $testdir2/hello2d +echo 1 > $testdir2/hello1f +echo 2 > $testdir2/hello2f cleanup() { chmod 777 $testdir/noread - rm -fr $testdir $testout + rm -fr $testdir $testdir2 $testout } trap cleanup 0 HUP INT QUIT TERM @@ -815,6 +821,41 @@ if test $failed -ne 0; then result=1 fi +# Test that if the specified glob ends with a slash then only directories are +# matched. +# First run with the glob that has no slash to demonstrate presence of a slash +# makes a difference. +failed=0 +${test_program_prefix} \ +${common_objpfx}posix/globtest "$testdir2" "hello*" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`hello1d' +`hello1f' +`hello2d' +`hello2f' +EOF + +if test $failed -ne 0; then + echo "pattern+meta test failed" >> $logfile + result=1 +fi + +# The same pattern+meta test with a slash this time. +failed=0 +${test_program_prefix} \ +${common_objpfx}posix/globtest "$testdir2" "hello*/" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`hello1d/' +`hello2d/' +EOF + +if test $failed -ne 0; then + echo "pattern+meta+slash test failed" >> $logfile + result=1 +fi + if test $result -eq 0; then echo "All OK." > $logfile fi