Re: unsafe use of dirfd in fts.c, getcwd.c, glob.c?

[Jim Meyering]

Paul Eggert wrote:
> I ran into a problem with dirfd when coding up recent changes to GNU
> tar, to fix some race conditions when traversing directories.  Can
> dirfd really return -1 on real platforms, when its argument is a valid
> open directory stream?  POSIX allows that behavior, but I don't know
> of any implementations that do it.

Hi Paul,

Nor do I.

> If dirfd can return -1, it seems to me that fts.c, getcwd.c, and
> glob.c need to be fixed, as a cursory look at them suggests that they
> assume that dirfd never returns -1 in this situation.

I see that getcwd.c and glob.c do assume that.
E.g., here's getcwd.c's sole use of dirfd:

      fd = dirfd (dirstream);
      ...
            entry_status = fstatat (fd, d->d_name, &st, AT_SYMLINK_NOFOLLOW);

but fts.c uses dirfd like this:

                int dir_fd = dirfd(dirp);
                if (ISSET(FTS_CWDFD) && 0 <= dir_fd)
                  {
                    dir_fd = dup (dir_fd);
                    set_cloexec_flag (dir_fd, true);
                  }
                if (dir_fd < 0 || fts_safe_changedir(sp, cur, dir_fd, NULL)) {
                        if (nlinks && type == BREAD)
                                cur->fts_errno = errno;
                        cur->fts_flags |= FTS_DONTCHDIR;
                        descend = false;
                        closedir(dirp);
                        if (ISSET(FTS_CWDFD) && 0 <= dir_fd)
                                close (dir_fd);
                        dirp = NULL;
                } else
                        descend = true;

and while that code may call set_cloexec_flag on a negative DIR_FD
(upon failed "dup" call -- I'll fix that, mainly for consistency),
I don't see how it assumes dirfd never returns -1.

> Below is pseudocode for what I had to do in GNU tar to work around the
> possibility of dirfd returning -1.  The "if (fd2 < 0) { ... }" code
> can be removed if we know -1 isn't possible.  That code is pretty
> annoying, not to mention less-safe in the presence of race conditions,
> and almost inherently untestable.  I'd rather rip it out if it's not
> needed.

I'd rip it out.
And not even bother to assert (0 <= fd2).

We have two cases:
  - new POSIX-conforming dirfd(S) implementation returns -1 for valid S
  - some existing implementation does

I think neither is possible.

The POSIX wording allowing dirfd to fail on a valid stream is far older
than the *at functions, for which dirfd can now be essential.  Given this
relatively new constraint, the odds of any new, useful system providing
a stream interface without a backing file descriptor are nil.

If there were an existing system for which dirfd could always
fail, we would have known about it long before now.

Not that showing lots of offenses means it's ok, but
there are quite a few:

    http://www.google.com/codesearch?hl=en&start=10&sa=N&q=\%28dirfd\+*\%28

including this from coreutils-4.5.5/lib/ftw.c (inherited from glibc):

     415:   if (dir->stream != NULL)
     416:     if (__fchdir (dirfd (dir->stream)) == 0)
     417:       done = 1;