[PATCH] fdopendir: preserve argument fd before returning

[Paul Eggert]

This follows up on the thread earlier today.  In testing my earlier
patch, I found that fdopendir (FD) didn't do the right thing when
FD was the maximum allowed file descriptor but lower-numbered file
descriptors were available: it failed with errno == EMFILE.  I pushed
the following patch instead, which does the right thing in that
situation.  This patch also updates the comments to say "POSIX"
rather than "Solaris".  And it contains a sanity check for
race conditions in multithreaded apps (I didn't test that part,
as GNU tar isn't multithreaded).

* lib/fdopendir.c: Adjust comments to say POSIX, not Solaris.
(fdopendir_with_dup, fd_clone_opendir): New static functions.
(fdopendir): Use them, arranging for FD to be open to the same
directory that it was when it started.  (It might be temporarily
closed while fdopendir is running, so this not thread- or
signal-safe.)  Be careful to do the right thing even when file
descriptors are scarce and dup fails with errno == EMFILE.  See
<http://lists.gnu.org/archive/html/bug-gnulib/2010-09/msg00208.html>.
---
 ChangeLog       |   12 ++++++++
 lib/fdopendir.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 83 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d42e017..3d6d4c4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2010-09-13  Paul Eggert  <address@hidden>
+
+       fdopendir: preserve argument fd before returning
+       * lib/fdopendir.c: Adjust comments to say POSIX, not Solaris.
+       (fdopendir_with_dup, fd_clone_opendir): New static functions.
+       (fdopendir): Use them, arranging for FD to be open to the same
+       directory that it was when it started.  (It might be temporarily
+       closed while fdopendir is running, so this not thread- or
+       signal-safe.)  Be careful to do the right thing even when file
+       descriptors are scarce and dup fails with errno == EMFILE.  See
+       <http://lists.gnu.org/archive/html/bug-gnulib/2010-09/msg00208.html>.
+
 2010-09-10  Paolo Bonzini  <address@hidden>
 
        regex: Pass the system regex if its only problem is 32-bit regoff_t.
diff --git a/lib/fdopendir.c b/lib/fdopendir.c
index f7b29af..4adef63 100644
--- a/lib/fdopendir.c
+++ b/lib/fdopendir.c
@@ -33,25 +33,87 @@
 #  include "dirent--.h"
 # endif
 
-/* Replacement for Solaris' function by the same name.
-   <http://www.google.com/search?q=fdopendir+site:docs.sun.com>
+static DIR *fdopendir_with_dup (int, int);
+static DIR *fd_clone_opendir (int);
+
+/* Replacement for POSIX fdopendir.
+
    First, try to simulate it via opendir ("/proc/self/fd/FD").  Failing
    that, simulate it by using fchdir metadata, or by doing
    save_cwd/fchdir/opendir(".")/restore_cwd.
    If either the save_cwd or the restore_cwd fails (relatively unlikely),
    then give a diagnostic and exit nonzero.
-   Otherwise, this function works just like Solaris' fdopendir.
+
+   If successful, the resulting stream is based on FD in
+   implementations where streams are based on file descriptors and in
+   applications where no other thread or signal handler allocates or
+   frees file descriptors.  In other cases, consult dirfd on the result
+   to find out whether FD is still being used.
+
+   Otherwise, this function works just like POSIX fdopendir.
 
    W A R N I N G:
-   Unlike other fd-related functions, this one effectively consumes
-   its FD parameter.  The caller should not close or otherwise
-   manipulate FD if this function returns successfully.  Also, this
-   implementation does not guarantee that dirfd(fdopendir(n))==n;
-   the open directory stream may use a clone of FD, or have no
-   associated fd at all.  */
+
+   Unlike other fd-related functions, this one places constraints on FD.
+   If this function returns successfully, FD is under control of the
+   dirent.h system, and the caller should not close or modify the state of
+   FD other than by the dirent.h functions.  */
 DIR *
 fdopendir (int fd)
 {
+  return fdopendir_with_dup (fd, -1);
+}
+
+/* Like fdopendir, except that if OLDER_DUPFD is not -1, it is known
+   to be a dup of FD which is less than FD - 1 and which will be
+   closed by the caller and not otherwise used by the caller.  This
+   function makes sure that FD is closed and all file descriptors less
+   than FD are open, and then calls fd_clone_opendir on a dup of FD.
+   That way, barring race conditions, fd_clone_opendir returns a
+   stream whose file descriptor is FD.  */
+static DIR *
+fdopendir_with_dup (int fd, int older_dupfd)
+{
+  int dupfd = dup (fd);
+  if (dupfd < 0 && errno == EMFILE)
+    dupfd = older_dupfd;
+  if (dupfd < 0)
+    return NULL;
+  else
+    {
+      DIR *dir;
+      int saved_errno;
+      if (dupfd < fd - 1 && dupfd != older_dupfd)
+        {
+          dir = fdopendir_with_dup (fd, dupfd);
+          saved_errno = errno;
+        }
+      else
+        {
+          close (fd);
+          dir = fd_clone_opendir (dupfd);
+          saved_errno = errno;
+          if (! dir)
+            {
+              int fd1 = dup (dupfd);
+              if (fd1 != fd)
+                openat_save_fail (fd1 < 0 ? errno : EBADF);
+            }
+        }
+
+      if (dupfd != older_dupfd)
+        close (dupfd);
+      errno = saved_errno;
+      return dir;
+    }
+}
+
+/* Like fdopendir, except the result controls a clone of FD.  It is
+   the caller's responsibility both to close FD and (if the result is
+   not null) to closedir the result.  */
+static DIR *
+fd_clone_opendir (int fd)
+{
   int saved_errno;
   DIR *dir;
 
@@ -100,8 +162,6 @@ fdopendir (int fd)
 # endif /* !REPLACE_FCHDIR */
     }
 
-  if (dir)
-    close (fd);
   if (proc_file != buf)
     free (proc_file);
   errno = saved_errno;
-- 
1.7.2