Re: fts: slightly more robust

[Eric Blake]

Jim Meyering <jim <at> meyering.net> writes:

> > While we're visiting fts, how about this patch?  POSIX 2008 is clear that
> > opendir should use O_DIRECTORY, so opendirat should probably do likewise.
> 
> There is no need for O_DIRECTORY in that function, since the only thing
> it does with the resulting file descriptor is to apply fdopendir, and
> fdopendir will fail with ENOTDIR when fd refers to a non-directory.

Yes, fdopendir will reject non-directories, but the point of using O_DIRECTORY 
up front is to avoid blocking on a FIFO prior to reaching the fdopendir.

> 
> >  Do we want to obey the FIXME and make opendirat an independent module?
> 
> Sure, if there is another user.
> Is there?

Not so far, so I won't bother.

> > O_DIRECTORY|O_NOFOLLOW on
> > a symlink must produce failure, although POSIX is ambiguous whether the 
failure
> > will be ELOOP or ENOTDIR)

I stand corrected.  The POSIX wording is that O_DIRECTORY fails if "path does 
not name a directory", but elsewhere in POSIX, that same phrase applies to 
symlinks that point to directories.  In other words:

open("link-to-dir",O_RDONLY|O_DIRECTORY) -> passes
open("link-to-file",O_RDONLY|O_DIRECTORY) -> fails with ENOTDIR
open("link-to-dir",O_RDONLY|O_DIRECTORY|O_NOFOLLOW) -> fails with ELOOP
open("link-to-file",O_RDONLY|O_DIRECTORY|O_NOFOLLOW) -> fails with either ELOOP 
or ENOTDIR

> You might as well add all four,
>   O_DIRECTORY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW
> on the off-chance that O_DIRECTORY makes the earlier openat
> fail, and it fails with a more useful errno value than fdopendir.

So blindly adding O_NOFOLLOW for opendirat is wrong.

Here's the latest draft of my patch.

>From 0145db62dc0f4fc77ed98119e40d256d95aa21cb Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Tue, 1 Sep 2009 14:06:37 -0600
Subject: [PATCH] fts: make directory fds more robust

* lib/fts.c (O_DIRECTORY): Let <fcntl.h> take care of this.
(opendirat): Specify O_DIRECTORY, and add fallbacks for safety.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog |    6 ++++++
 lib/fts.c |    7 ++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ee81a23..58decf8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2009-09-02  Eric Blake  <address@hidden>

+       fts: make directory fds more robust
+       * lib/fts.c (O_DIRECTORY): Let <fcntl.h> take care of this.
+       (opendirat): Specify O_DIRECTORY, and add fallbacks for safety.
+
+2009-09-02  Eric Blake  <address@hidden>
+
        backupfile, chdir-long, fts, savedir: make safer
        * lib/backupfile.c (includes): Use "dirent--.h", since
        numbered_backup can write to stderr during readdir.
diff --git a/lib/fts.c b/lib/fts.c
index 7616c6f..ebf66fc 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -228,10 +228,6 @@ static void free_dir (FTS *fts) {}
 # define SIZE_MAX ((size_t) -1)
 #endif

-#ifndef O_DIRECTORY
-# define O_DIRECTORY 0
-#endif
-
 #define ISDOT(a)       (a[0] == '.' && (!a[1] || (a[1] == '.' && !a[2])))
 #define STREQ(a, b)    (strcmp ((a), (b)) == 0)

@@ -309,7 +305,8 @@ static inline DIR *
 internal_function
 opendirat (int fd, char const *dir)
 {
-  int new_fd = openat (fd, dir, O_RDONLY);
+  int new_fd = openat (fd, dir,
+                      O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK);
   DIR *dirp;

   if (new_fd < 0)
-- 
1.6.3.2