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

[Paul Eggert <eggert at cs dot ucla dot edu>]

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.