Re: fts: slightly more robust

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> Here's the latest draft of my patch.

While we're at it, I noticed via findutils that fts leaks fds into child 
processes.  This plugs the fts leak (but completely fixing find also requires a 
patch to findutils).

Hmm - POSIX states that fdopendir can, but not must, set the cloexec flag, as 
part of consuming the fd.  Maybe the gnulib fdopendir module should guarantee 
that the cloexec flag is set as part of creating a directory stream, rendering 
the first of the three hunks to fts.c redundant?  Or maybe keep the fdopendir 
module as-is, but create a new module fdopendir-gnu, which goes further and 
gives the additional GNU semantics that: fd is changed to cloexec, and dirfd 
gives the same fd back (requires tweaking rpl_opendir on mingw to open an fd up 
front, and for all other platforms lacking fdopendir it requires writing into 
the member variable read by dirfd).


From: Eric Blake <address@hidden>
Date: Wed, 2 Sep 2009 14:44:51 -0600
Subject: [PATCH] fts: avoid leaking fds

* modules/fts (Depends-on): Add cloexec.
* lib/fts.c (opendirat, diropen, fts_build): Set close-on-exec
flag.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog   |    5 +++++
 lib/fts.c   |   18 ++++++++++++++----
 modules/fts |    1 +
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e63e020..3933600 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2009-09-02  Eric Blake  <address@hidden>

+       fts: avoid leaking fds
+       * modules/fts (Depends-on): Add cloexec.
+       * lib/fts.c (opendirat, diropen, fts_build): Set close-on-exec
+       flag.
+
        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.
diff --git a/lib/fts.c b/lib/fts.c
index ebf66fc..c05eb8b 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -71,6 +71,9 @@ static char sccsid[] = "@(#)fts.c     8.6 (Berkeley) 8/14/94";
 # include "fcntl--.h"
 # include "dirent--.h"
 # include "unistd--.h"
+/* FIXME - use fcntl(F_DUPFD_CLOEXEC)/openat(O_CLOEXEC) once they are
+   supported.  */
+# include "cloexec.h"
 # include "same-inode.h"
 #endif

@@ -311,6 +314,7 @@ opendirat (int fd, char const *dir)

   if (new_fd < 0)
     return NULL;
+  set_cloexec_flag (new_fd, true);
   dirp = fdopendir (new_fd);
   if (dirp == NULL)
     {
@@ -362,9 +366,12 @@ diropen (FTS const *sp, char const *dir)
   int open_flags = (O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK
                    | (ISSET (FTS_PHYSICAL) ? O_NOFOLLOW : 0));

-  return (ISSET (FTS_CWDFD)
-         ? openat (sp->fts_cwd_fd, dir, open_flags)
-         : open (dir, open_flags));
+  int fd = (ISSET (FTS_CWDFD)
+            ? openat (sp->fts_cwd_fd, dir, open_flags)
+            : open (dir, open_flags));
+  if (0 <= fd)
+    set_cloexec_flag (fd, true);
+  return fd;
 }

 FTS *
@@ -1305,7 +1312,10 @@ fts_build (register FTS *sp, int type)
        if (nlinks || type == BREAD) {
                int dir_fd = dirfd(dirp);
                if (ISSET(FTS_CWDFD) && 0 <= dir_fd)
-                 dir_fd = dup (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;
diff --git a/modules/fts b/modules/fts
index f80a827..9509557 100644
--- a/modules/fts
+++ b/modules/fts
@@ -8,6 +8,7 @@ lib/fts-cycle.c
 m4/fts.m4

 Depends-on:
+cloexec
 cycle-check
 d-ino
 d-type
-- 
1.6.3.2