Re: fts: is an O_NOFOLLOW missing in fts_build / opendirat?

[Jim Meyering]

Paul Eggert wrote:
> On 09/13/10 01:02, Jim Meyering wrote:
>> I ran this test: apply your patch, compile everything and
>> run coreutils' "make check" tests.  With this change, 4 tests fail:
>>
>>     $ grep FAIL tests/test-suite.log
>>     FAIL: chgrp/posix-H (exit: 1)
>>     FAIL: chgrp/recurse (exit: 1)
>>     FAIL: du/deref (exit: 1)
>>     FAIL: du/deref-args (exit: 1)
>
> Good thing I said that patch was "untested" :-).  I just now tried the
> patch on my RHEL 5 host, and got the du failures but not the chgrp
> failures, I guess because the openattish support is somewhat limited
> in RHEL 5?
>
>> I haven't yet had time to look carefully, but those failures suggest that
>> such a change would also have to account for the FTS_COMFOLLOW option,
>> which may be set along with FTS_PHYSICAL.
>
> Yes, that could be it.  I tried the following patch instead, and it
> fixed the du failures on RHEL 5.  Maybe it will also fix the chgrp
> failures on your host.  (I don't know offhand how to write a test case
> that will catch the race condition bug that this patch fixes.)
>
> diff --git a/lib/fts.c b/lib/fts.c
> index 4b89ee7..1daf69c 100644
> --- a/lib/fts.c
> +++ b/lib/fts.c
> @@ -290,10 +290,11 @@ fts_set_stat_required (FTSENT *p, bool required)
>  /* FIXME: if others need this function, move it into lib/openat.c */
>  static inline DIR *
>  internal_function
> -opendirat (int fd, char const *dir)
> +opendirat (int fd, char const *dir, int extra_flags)
>  {
>    int new_fd = openat (fd, dir,
> -                       O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK);
> +                       (O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK
> +                        | extra_flags));
>    DIR *dirp;
>
>    if (new_fd < 0)
> @@ -1237,7 +1238,11 @@ fts_build (register FTS *sp, int type)
>  #else
>  # define __opendir2(file, flag) \
>          ( ! ISSET(FTS_NOCHDIR) && ISSET(FTS_CWDFD) \
> -          ? opendirat(sp->fts_cwd_fd, file)        \
> +          ? opendirat(sp->fts_cwd_fd, file,        \
> +                      ((ISSET(FTS_PHYSICAL)                   \
> +                        && ! (cur->fts_level == FTS_ROOTLEVEL \
> +                              && ISSET(FTS_COMFOLLOW)))       \
> +                       ? O_NOFOLLOW : 0))                     \

That passes all tests on Fedora 13 and looks sensible.
Thanks for the fix!

I agree that writing a test for that fix is probably more
trouble than it's worth.