Re: [PATCH] posix: if glob has a trailing slash match directories only.

[Jonathan Nieder]

(+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  <address@hidden>
> +
> + [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