This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] posix: Fix glob with GLOB_NOCHECK returning modified patterns (BZ#10246)
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*.