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

This is a follow up of my previous fix of BZ#10246 [1] which addresses
the issues raised by Paul Eggert.  The issue is for glob calls which
internally enabled GLOB_ONLYDIR (essentinally pattern ending with '/')
required for dangling symlinks to actually check if the directory is
accessible.  An extra testcase is added on tst-glob_symlinks.

---

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.

	* posix/glob (glob_lstat, glob_opendir, glob_readdir,
	glob_closedir): New function: wrapper for the lstat, opendir,
	readdir, and closedir respectively.
	(convert_dirent, convert_dirent64): Remove function.
	(glob, glob_in_dir):  Handle pattern that do not match and
	start with '/' correctly.
	* posix/globtest.sh: New tests for NOCHECK.
	* posix/tst-glob_symlinks.c: Add tests for dangling directory.

[1] https://sourceware.org/ml/libc-alpha/2017-09/msg00315.html

---
 ChangeLog                 |  11 ++++
 posix/glob.c              | 124 +++++++++++++++++++++++++---------------------
 posix/globtest.sh         |  37 ++++++++++++++
 posix/tst-glob_symlinks.c |   6 +++
 4 files changed, 121 insertions(+), 57 deletions(-)

-- 
2.7.4

Adhemerval Zanella wrote:

> +  return (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)


Use __glibc_unlikely instead of __builtin_expect with 0, here and elsewhere.

> +	 ? (*pglob->gl_lstat) (fname, &ust->st)


No need for the "*" or for the initial pair of parens, both here and elsewhere.

> +	 : __lstat64 (fname, &ust->st64));


The ? and : should be indented one more column than the (.

> +}

> +

> +static void *

> +glob_opendir (glob_t *pglob, int flags, const char *directory)

> +{

> +  return (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)

> +	  ? (*pglob->gl_opendir) (directory)

> +	: opendir (directory));

>   }


The indenting is not right there: ? and : should have the same indentation.

> @@ -1332,6 +1334,17 @@ glob_in_dir (const char *pattern, const char *directory, int flags,

>   

>   	      if (fnmatch (pattern, d.name, fnm_flags) == 0)

>   		{

> +		  /* We need to certify that the link is a directory.  */

> +		  if (flags & GLOB_ONLYDIR && readdir_result_type (d) == DT_LNK > +		    {

> +		      void *ss = glob_opendir (pglob, flags, d.name);


I'm afraid there are several problems here. GLOB_ONLYDIR does not mean, "return 
only directories". It means, "the caller is interested only in directories, so 
if you can easily determine that the result is not a directory, don't bother to 
return it. However, if it is expensive to check then it is OK to return the 
result anyway, as it is the caller's responsibility to check that returned 
strings are directories." So the comment is wrong here, and the code looks wrong 
too: calling glob_opendir is expensive, so it should not be done here.

Also, why use glob_opendir to test whether d.name is a directory, when the rest 
of the code uses is_dir to do that?

Also, if readdir_result_type (d) is DT_UNKNOWN, d.name might be a directory, so 
the code should check in that case too.

Also, d.name is just a file name component: it should be absolute, or relative 
to the working directory.

Also, the code mistakenly treats leading "//" as if it were "/"; this is not 
correct, and does not work on some platforms. POSIX allows implementations where 
"//" is distinct from "/".

Also, it's not clear to me that the code treats strings of one or more "/"s 
correctly, in the pattern.

On 23/09/2017 03:46, Paul Eggert wrote:
> Adhemerval Zanella wrote:

> 

>> +  return (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)

> 

> Use __glibc_unlikely instead of __builtin_expect with 0, here and elsewhere.


Ack.

> 

>> +     ? (*pglob->gl_lstat) (fname, &ust->st)

> 

> No need for the "*" or for the initial pair of parens, both here and elsewhere.


Ack.

> 

>> +     : __lstat64 (fname, &ust->st64));

> 

> The ? and : should be indented one more column than the (.


Ack.

> 

>> +}

>> +

>> +static void *

>> +glob_opendir (glob_t *pglob, int flags, const char *directory)

>> +{

>> +  return (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)

>> +      ? (*pglob->gl_opendir) (directory)

>> +    : opendir (directory));

>>   }

> 

> The indenting is not right there: ? and : should have the same indentation.


Ack.

> 

>> @@ -1332,6 +1334,17 @@ glob_in_dir (const char *pattern, const char *directory, int flags,

>>               if (fnmatch (pattern, d.name, fnm_flags) == 0)

>>           {

>> +          /* We need to certify that the link is a directory.  */

>> +          if (flags & GLOB_ONLYDIR && readdir_result_type (d) == DT_LNK > +            {

>> +              void *ss = glob_opendir (pglob, flags, d.name);

> 

> I'm afraid there are several problems here. GLOB_ONLYDIR does not mean, "return only directories". It means, "the caller is interested only in directories, so if you can easily determine that the result is not a directory, don't bother to return it. However, if it is expensive to check then it is OK to return the result anyway, as it is the caller's responsibility to check that returned strings are directories." So the comment is wrong here, and the code looks wrong too: calling glob_opendir is expensive, so it should not be done here.


I tried to defer the call that check if the link is actually pointing to a directory
as late as possible.  Without actually following the possible directory the obtained 
matched name the example you provided:

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

Will be matched only for globlink1.

> 

> Also, why use glob_opendir to test whether d.name is a directory, when the rest of the code uses is_dir to do that?


Good point, it can be simplified to use is_dir indeed.

> 

> Also, if readdir_result_type (d) is DT_UNKNOWN, d.name might be a directory, so the code should check in that case too.


I excluded DT_UNKNOWN mainly because posix/tst-gnuglob.c uses a for the synthetic
directory to tests some entries as DT_UNKNOWN and it does not expect them to 
be matches.  If we change to also match DT_UNKNOWN for possible directories
entries it would meant change slight current glob semantic.

> 

> Also, d.name is just a file name component: it should be absolute, or relative to the working directory.


Not really sure what this may lead to issue in current patch, care to
elaborate?

> 

> Also, the code mistakenly treats leading "//" as if it were "/"; this is not correct, and does not work on some platforms. POSIX allows implementations where "//" is distinct from "/".


Indeed, although for *glibc* supported platforms this is not an issue. I don't
consider this a block for glibc inclusion, but I am open to suggestion on how 
to handle correctly on platforms with handle '//' distinct from '/'.

> 

> Also, it's not clear to me that the code treats strings of one or more "/"s correctly, in the pattern.


Same as previous remark, using the same testcase:

  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);

Using either 'globlink[12]//' or 'globlink[12]///' will handle the same
results as 'globlink[12]/'.  This still might be not the correct solution for
platform that  handle '//' distinct from '/', but again I do not think it is
block for *glibc*.

Adhemerval Zanella wrote:

> Without actually following the possible directory the obtained

> matched name the example you provided:

> 

>    ln -s /no-such-file globlink1

>    ln -s . globlink2

> 

> Will be matched only for globlink1.


You're correct that there's a bug here, that needs to be fixed. But the proposed 
patch changes the semantics of GLOB_ONLYDIR contrary to its intent. GLOB_ONLYDIR 
is supposed to be an efficiency flag, to let callers run faster. By making 
GLOB_ONLYDIR run slower, the patch is contrary to this expectation.

The bug needs to be fixed, but not this way. It's the caller's responsibility to 
check that the returned value is indeed a directory, and that's how the fix 
should be written: by fixing the caller to check that the result is indeed a 
directory, not by slowing down the callee.

>> Also, if readdir_result_type (d) is DT_UNKNOWN, d.name might be a directory, so the code should check in that case too.

> 

> I excluded DT_UNKNOWN mainly because posix/tst-gnuglob.c uses a for the synthetic

> directory to tests some entries as DT_UNKNOWN and it does not expect them to

> be matches.


That sounds like a bug in the test case, then. DT_UNKNOWN is returned for file 
systems that do not have the file type in the inode. A DT_UNKNOWN entry might be 
a directory, or a symlink, or anything else.

> If we change to also match DT_UNKNOWN for possible directories

> entries it would meant change slight current glob semantic.


How so? DT_UNKNOWN merely means that glob can't do some optimizations. This 
should not change the semantics, only performance.

>> Also, d.name is just a file name component: it should be absolute, or relative to the working directory.

> 

> Not really sure what this may lead to issue in current patch, care to

> elaborate?


As I recall, d.name is relative to the parent directory: it has no slashes. The 
argument to is_dir is supposed to be the full filename, either absolute or 
relative to the current working directory.

>> Also, the code mistakenly treats leading "//" as if it were "/"; this is not correct, and does not work on some platforms. POSIX allows implementations where "//" is distinct from "/".

> 

> Indeed, although for *glibc* supported platforms this is not an issue.


Fair enough, we can fix that separately.

> Using either 'globlink[12]//' or 'globlink[12]///' will handle the same

> results as 'globlink[12]/'.


Every result returned by globlink[12]/// should end in three slashes. The number 
of  slashes in the result should equal the number of slashes in the pattern. 
This general issue differs from the issue about // vs / at the start of the file 
name, as the trailing slashes are portable to all filesystems.