[9/9] posix: Fix glob with GLOB_NOCHECK returning modified patterns (BZ#10246)

Message ID 1504643122-14874-10-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • posix: glob fixes and refactor
Related show

Commit Message

Adhemerval Zanella Sept. 5, 2017, 8:25 p.m.
According to POSIX glob with GLOB_NOCHECK should return a list consisting
of only of the input pattern in case of no match.  However GLIBC does not
honor in case of '//<something'.  This is due internally this is handled
and special case and prefix_array (responsable to prepend the directory
name) does not know if the input already contains a slash or not since
either '/<something>' or '//<something>' will be handle in same way.

This patch fix it by using a empty directory name for the latter (since
prefix_array already adds a slash as default for each entry).

Checked on x86_64-linux-gnu and on a build using build-many-glibcs.py
for all major architectures.

	[BZ #10246]
	* posix/glob.c (glob): Handle pattern that do not match and
	start with '/' correctly.
	* posix/globtest.sh: New tests for NOCHECK.
---
 ChangeLog         |  7 ++++++-
 posix/glob.c      | 13 +++++++------
 posix/globtest.sh | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.7.4

Comments

Paul Eggert Sept. 7, 2017, 10:14 p.m. | #1
Although this is a definite bug and the patch fixes this instance of it, I'm 
afraid other instances remain unfixed. For example:

    glob_t g; glob ("//a*b", 0, NULL, &g)

can do the wrong thing, since glob calls opendir on "/" instead of "//", and on 
some platforms "/" and "//" are different directories (POSIX allows this as a 
special exception).

A more serious example. If you do this:

   ln -s /no-such-file globlink1
   ln -s . globlink2

then:

       glob_t g;
       int res = glob ("globlink[12]/", 0, NULL, &g);
       assert (res == 0 && g.gl_pathc == 1);
       assert (strcmp (g.gl_pathv[0], "globlink2/") == 0);

fails, since glob gets confused about directories and slashes and mistakenly 
returns two results. Although this bug is seemingly unrelated, the underlying 
cause is the same: glob gets confused about whether to include or exclude 
slashes when doing its tests.

I'll take a look at it, though the fix won't be trivial.

PS. This finishes my review of this patchset. Patches 1-8 are OK to be 
installed, with the trivial changes I suggested earlier. This patch (patch 9) 
I'd like to hold off on, until we've had a chance to work out a 
more-comprehensive fix.

PPS. I'm still slowly wending my way through your original patchset. Most 
recently I looked at "[PATCH 07/18] posix: User LOGIN_NAME_MAX for all user 
names in glob" <https://sourceware.org/ml/libc-alpha/2017-08/msg00447.html>. I'm 
afraid a good fix needs to be hairier there too, as POSIX does not require 
LOGIN_NAME_MAX to be suitable for a stack-based buffer, or even to be defined. I 
have a partly-drafted patch which I hope to finish in the not-too-distant future.
Adhemerval Zanella Sept. 8, 2017, 9:15 a.m. | #2
On 08/09/2017 00:14, Paul Eggert wrote:
> Although this is a definite bug and the patch fixes this instance of

> it, I'm afraid other instances remain unfixed. For example:

>

>    glob_t g; glob ("//a*b", 0, NULL, &g)

>

> can do the wrong thing, since glob calls opendir on "/" instead of

> "//", and on some platforms "/" and "//" are different directories

> (POSIX allows this as a special exception).

>

> A more serious example. If you do this:

>

>   ln -s /no-such-file globlink1

>   ln -s . globlink2

>

> then:

>

>       glob_t g;

>       int res = glob ("globlink[12]/", 0, NULL, &g);

>       assert (res == 0 && g.gl_pathc == 1);

>       assert (strcmp (g.gl_pathv[0], "globlink2/") == 0);

>

> fails, since glob gets confused about directories and slashes and

> mistakenly returns two results. Although this bug is seemingly

> unrelated, the underlying cause is the same: glob gets confused about

> whether to include or exclude slashes when doing its tests.

>

> I'll take a look at it, though the fix won't be trivial.

>

> PS. This finishes my review of this patchset. Patches 1-8 are OK to be

> installed, with the trivial changes I suggested earlier. This patch

> (patch 9) I'd like to hold off on, until we've had a chance to work

> out a more-comprehensive fix.

Fair enough, I will hold patch 9 push and take a look at the examples you
brought up.  Thanks for the follow up.

>

> PPS. I'm still slowly wending my way through your original patchset.

> Most recently I looked at "[PATCH 07/18] posix: User LOGIN_NAME_MAX

> for all user names in glob"

> <https://sourceware.org/ml/libc-alpha/2017-08/msg00447.html>. I'm

> afraid a good fix needs to be hairier there too, as POSIX does not

> require LOGIN_NAME_MAX to be suitable for a stack-based buffer, or

> even to be defined. I have a partly-drafted patch which I hope to

> finish in the not-too-distant future.

Alright, my initial patch was to adequate it to glibc code (which does
define a actual limit suitable to stack allocation) and get rid of any
alloca usage.  My understanding, based on gnulib commit 064df0b0c,
is it should not impose a limit on user name length. 

So currently in a patchset I am intended to send after this one, user
name handling is now based on my char_array struct and thus allocates
the user_name dinamically if required.

Patch

diff --git a/posix/glob.c b/posix/glob.c
index 30a4143..25c5d24 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -272,6 +272,8 @@  glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
   size_t oldcount;
   int meta;
   int dirname_modified;
+  /* Indicate if the directory should be prepended on return values.  */
+  bool dirname_prefix = true;
   int malloc_dirname = 0;
   glob_t dirs;
   int retval = 0;
@@ -495,6 +497,10 @@  glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
       dirname = (char *) "/";
       dirlen = 1;
       ++filename;
+      /* prefix_array adds a separator for each result and DIRNAME is
+	 already '/'.  So we indicate later that we should not prepend
+	 anything for this specific case.  */
+      dirname_prefix = false;
     }
   else
     {
@@ -1086,7 +1092,7 @@  glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
       if (dirlen > 0)
 	{
 	  /* Stick the directory on the front of each name.  */
-	  if (prefix_array (dirname,
+	  if (prefix_array (dirname_prefix ? dirname : "",
 			    &pglob->gl_pathv[old_pathc + pglob->gl_offs],
 			    pglob->gl_pathc - old_pathc))
 	    {
@@ -1167,11 +1173,6 @@  prefix_array (const char *dirname, char **array, size_t n)
   size_t dirlen = strlen (dirname);
   char dirsep_char = '/';
 
-  if (dirlen == 1 && dirname[0] == '/')
-    /* DIRNAME is just "/", so normal prepending would get us "//foo".
-       We want "/foo" instead, so don't prepend any chars from DIRNAME.  */
-    dirlen = 0;
-
 #if defined __MSDOS__ || defined WINDOWS32
   if (dirlen > 1)
     {
diff --git a/posix/globtest.sh b/posix/globtest.sh
index 73f7ae3..92a8e37 100755
--- a/posix/globtest.sh
+++ b/posix/globtest.sh
@@ -242,6 +242,43 @@  if test $failed -ne 0; then
   result=1
 fi
 
+# Test NOCHECK for specific cases where the pattern used starts
+# with '/' (BZ#10246).
+failed=0
+${test_program_prefix} \
+${common_objpfx}posix/globtest -c "$testdir" "/%" |
+sort > $testout
+cat <<"EOF" | $CMP - $testout >> $logfile || failed=1
+`/%'
+EOF
+if test $failed -ne 0; then
+  echo "No check test failed" >> $logfile
+  result=1
+fi
+
+${test_program_prefix} \
+${common_objpfx}posix/globtest -c "$testdir" "//%" |
+sort > $testout
+cat <<"EOF" | $CMP - $testout >> $logfile || failed=1
+`//%'
+EOF
+if test $failed -ne 0; then
+  echo "No check test failed" >> $logfile
+  result=1
+fi
+
+${test_program_prefix} \
+${common_objpfx}posix/globtest -c "$testdir" "///%" |
+sort > $testout
+cat <<"EOF" | $CMP - $testout >> $logfile || failed=1
+`///%'
+EOF
+if test $failed -ne 0; then
+  echo "No check test failed" >> $logfile
+  result=1
+fi
+
+
 # Test NOMAGIC without magic characters
 failed=0
 ${test_program_prefix} \