Re: need opendir_safer, dirent--.h

[Eric Blake]

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

> > I think we need to implement opendir_safer, alongside all the other
> > *_safer modules.  Otherwise, opendir can end up placing an open directory
> > fd in one of the standard slots, and end up interfering with the intent of
> > all the other *_safer wrappers.
> 
> And here's at least one use case where it matters:
> 
> $ find dir -mindepth 1 -ok echo {} \; <&-
> < echo ... dir/sub > ? 
> $ echo $?
> 0

Here's the preliminary patch series, to be applied on top of my 
fchdir/fdopendir series.  However, since we are also lacking openat_safer, it 
looks like fts will STILL pollute the standard fds.  I'll have to add in 
another patch for openat-safer, then test with findutils, before calling this 
series ready for prime-time.


From: Eric Blake <address@hidden>
Date: Tue, 1 Sep 2009 07:41:28 -0600
Subject: [PATCH 1/2] dirent-safer: new module

* modules/dirent-safer: New file.
* lib/dirent--.h: Likewise.
* lib/dirent-safer.h: Likewise.
* lib/opendir-safer.c: Likewise.
* m4/dirent-safer.m4: Likewise.
* MODULES.html.sh (Enhancements for POSIX:2008): Mention it.
* modules/dirent-safer-tests: New test.
* tests/test-dirent-safer.c: New file.
* lib/fdopendir.c (includes): Ensure fdopendir is also safe.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                  |   11 +++++
 MODULES.html.sh            |    1 +
 lib/dirent--.h             |   23 ++++++++++
 lib/dirent-safer.h         |   22 +++++++++
 lib/fdopendir.c            |    4 ++
 lib/opendir-safer.c        |   68 ++++++++++++++++++++++++++++
 m4/dirent-safer.m4         |   11 +++++
 modules/dirent-safer       |   28 ++++++++++++
 modules/dirent-safer-tests |   11 +++++
 tests/test-dirent-safer.c  |  106 ++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 285 insertions(+), 0 deletions(-)
 create mode 100644 lib/dirent--.h
 create mode 100644 lib/dirent-safer.h
 create mode 100644 lib/opendir-safer.c
 create mode 100644 m4/dirent-safer.m4
 create mode 100644 modules/dirent-safer
 create mode 100644 modules/dirent-safer-tests
 create mode 100644 tests/test-dirent-safer.c

diff --git a/ChangeLog b/ChangeLog
index ed1736a..44bd320 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2009-09-01  Eric Blake  <address@hidden>

+       dirent-safer: new module
+       * modules/dirent-safer: New file.
+       * lib/dirent--.h: Likewise.
+       * lib/dirent-safer.h: Likewise.
+       * lib/opendir-safer.c: Likewise.
+       * m4/dirent-safer.m4: Likewise.
+       * MODULES.html.sh (Enhancements for POSIX:2008): Mention it.
+       * modules/dirent-safer-tests: New test.
+       * tests/test-dirent-safer.c: New file.
+       * lib/fdopendir.c (includes): Ensure fdopendir is also safe.
+
        fdopendir: optimize on mingw
        * lib/unistd.in.h (_gl_directory_name): New prototype.
        * lib/fchdir.c (_gl_directory_name): Implement it.
diff --git a/MODULES.html.sh b/MODULES.html.sh
index 027a0bc..bb99642 100755
--- a/MODULES.html.sh
+++ b/MODULES.html.sh
@@ -2394,6 +2394,7 @@ func_all_modules ()

   func_begin_table
   func_module chdir-long
+  func_module dirent-safer
   func_module dirname
   func_module getopt
   func_module iconv_open-utf
diff --git a/lib/dirent--.h b/lib/dirent--.h
new file mode 100644
index 0000000..088aafd
--- /dev/null
+++ b/lib/dirent--.h
@@ -0,0 +1,23 @@
+/* Like dirent.h, but redefine some names to avoid glitches.
+
+   Copyright (C) 2009 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Written by Eric Blake.  */
+
+#include "dirent-safer.h"
+
+#undef opendir
+#define opendir opendir_safer
diff --git a/lib/dirent-safer.h b/lib/dirent-safer.h
new file mode 100644
index 0000000..0debe26
--- /dev/null
+++ b/lib/dirent-safer.h
@@ -0,0 +1,22 @@
+/* Invoke dirent-like functions, but avoid some glitches.
+
+   Copyright (C) 2009 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Written by Eric Blake.  */
+
+#include <dirent.h>
+
+DIR *opendir_safer (const char *name);
diff --git a/lib/fdopendir.c b/lib/fdopendir.c
index 3bc13ac..14dc111 100644
--- a/lib/fdopendir.c
+++ b/lib/fdopendir.c
@@ -27,6 +27,10 @@
 #include "openat-priv.h"
 #include "save-cwd.h"

+#if GNULIB_DIRENT_SAFER
+# include "dirent--.h"
+#endif
+
 /* Replacement for Solaris' function by the same name.
    <http://www.google.com/search?q=fdopendir+site:docs.sun.com>
    First, try to simulate it via opendir ("/proc/self/fd/FD").  Failing
diff --git a/lib/opendir-safer.c b/lib/opendir-safer.c
new file mode 100644
index 0000000..8c399d0
--- /dev/null
+++ b/lib/opendir-safer.c
@@ -0,0 +1,68 @@
+/* Invoke opendir, but avoid some glitches.
+
+   Copyright (C) 2009 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Written by Eric Blake.  */
+
+#include <config.h>
+
+#include "dirent-safer.h"
+
+#include <errno.h>
+#include <unistd.h>
+#include "unistd-safer.h"
+
+/* Like opendir, but do not clobber stdin, stdout, or stderr.  */
+
+DIR *
+opendir_safer (char const *name)
+{
+  DIR *dp = opendir (name);
+
+  if (dp)
+    {
+      int fd = dirfd (dp);
+
+      if (0 <= fd && fd <= STDERR_FILENO)
+        {
+          /* If fdopendir is native (as on Linux), then it is safe to
+             assume dirfd(fdopendir(n))==n.  If we are using the
+             gnulib module fdopendir, then this guarantee is not met,
+             but fdopendir recursively calls opendir_safer up to 3
+             times to at least get a safe fd.  If fdopendir is not
+             present but dirfd is accurate (as on cygwin 1.5.x), then
+             we recurse up to 3 times ourselves.  Finally, if dirfd
+             always fails (as on mingw), then we are already safe.  */
+          DIR *newdp;
+          int e;
+#if HAVE_FDOPENDIR || GNULIB_FDOPENDIR
+          int f = dup_safer (fd);
+          newdp = fdopendir (f);
+          e = errno;
+          if (! newdp)
+            close (f);
+#else /* !FDOPENDIR */
+          newdp = opendir_safer (name);
+          e = errno;
+#endif
+          closedir (dp);
+          errno = e;
+          dp = newdp;
+        }
+    }
+
+  return dp;
+}
diff --git a/m4/dirent-safer.m4 b/m4/dirent-safer.m4
new file mode 100644
index 0000000..6e8e6c4
--- /dev/null
+++ b/m4/dirent-safer.m4
@@ -0,0 +1,11 @@
+#serial 1
+dnl Copyright (C) 2009 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_DIRENT_SAFER],
+[
+  AC_CHECK_FUNCS_ONCE([fdopendir])
+  AC_LIBOBJ([opendir-safer])
+])
diff --git a/modules/dirent-safer b/modules/dirent-safer
new file mode 100644
index 0000000..47db18e
--- /dev/null
+++ b/modules/dirent-safer
@@ -0,0 +1,28 @@
+Description:
+Directory functions that avoid clobbering STD{IN,OUT,ERR}_FILENO.
+
+Files:
+lib/dirent--.h
+lib/dirent-safer.h
+lib/opendir-safer.c
+m4/dirent-safer.m4
+
+Depends-on:
+dirent
+dirfd
+unistd-safer
+
+configure.ac:
+gl_DIRENT_SAFER
+gl_MODULE_INDICATOR([dirent-safer])
+
+Makefile.am:
+
+Include:
+"dirent-safer.h"
+
+License:
+GPL
+
+Maintainer:
+Eric Blake
diff --git a/modules/dirent-safer-tests b/modules/dirent-safer-tests
new file mode 100644
index 0000000..da87778
--- /dev/null
+++ b/modules/dirent-safer-tests
@@ -0,0 +1,11 @@
+Files:
+tests/test-dirent-safer.c
+
+Depends-on:
+dup2
+
+configure.ac:
+
+Makefile.am:
+TESTS += test-dirent-safer
+check_PROGRAMS += test-dirent-safer
diff --git a/tests/test-dirent-safer.c b/tests/test-dirent-safer.c
new file mode 100644
index 0000000..aeb8342
--- /dev/null
+++ b/tests/test-dirent-safer.c
@@ -0,0 +1,106 @@
+/* Test that directory streams leave standard fds alone.
+   Copyright (C) 2009 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Written by Eric Blake <address@hidden>, 2009.  */
+
+#include <config.h>
+
+#include "dirent--.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include "unistd-safer.h"
+
+/* This test intentionally closes stderr.  So, we arrange to have fd 10
+   (outside the range of interesting fd's during the test) set up to
+   duplicate the original stderr.  */
+
+#define BACKUP_STDERR_FILENO 10
+static FILE *myerr;
+
+#define ASSERT(expr) \
+  do                                                                         \
+    {                                                                        \
+      if (!(expr))                                                           \
+        {                                                                    \
+          fprintf (myerr, "%s:%d: assertion failed\n", __FILE__, __LINE__);  \
+          fflush (myerr);                                                    \
+          abort ();                                                          \
+        }                                                                    \
+    }                                                                        \
+  while (0)
+
+int
+main ()
+{
+  int i;
+  DIR *dp;
+  /* The dirent-safer module works without the use of fdopendir (which
+     would also pull in fchdir and openat); but if those modules were
+     also used, we ensure that they are safe.  In particular, the
+     gnulib version of fdopendir is unable to guarantee that
+     dirfd(fdopendir(fd))==fd, but we can at least guarantee that if
+     they are not equal, the fd returned by dirfd is safe.  */
+#if HAVE_FDOPENDIR || GNULIB_FDOPENDIR
+  int dfd;
+#endif
+
+  /* We close fd 2 later, so save it in fd 10.  */
+  if (dup2 (STDERR_FILENO, BACKUP_STDERR_FILENO) != BACKUP_STDERR_FILENO
+      || (myerr = fdopen (BACKUP_STDERR_FILENO, "w")) == NULL)
+    return 2;
+
+#if HAVE_FDOPENDIR || GNULIB_FDOPENDIR
+  dfd = open (".", O_RDONLY);
+  ASSERT (STDERR_FILENO < dfd);
+#endif
+
+  /* Four iterations, with progressively more standard descriptors
+     closed.  */
+  for (i = -1; i <= STDERR_FILENO; i++)
+    {
+      if (0 <= i)
+        ASSERT (close (i) == 0);
+      dp = opendir (".");
+      ASSERT (dp);
+      ASSERT (dirfd (dp) == -1 || STDERR_FILENO < dirfd (dp));
+      ASSERT (closedir (dp) == 0);
+
+#if HAVE_FDOPENDIR || GNULIB_FDOPENDIR
+      {
+        int fd = dup_safer (dfd);
+        ASSERT (STDERR_FILENO < fd);
+        dp = fdopendir (fd);
+        ASSERT (dp);
+        ASSERT (dirfd (dp) == -1 || STDERR_FILENO < dirfd (dp));
+        ASSERT (closedir (dp) == 0);
+        errno = 0;
+        ASSERT (close (fd) == -1);
+        ASSERT (errno == EBADF);
+      }
+#endif
+    }
+
+#if HAVE_FDOPENDIR || GNULIB_FDOPENDIR
+  ASSERT (close (dfd) == 0);
+#endif
+
+  return 0;
+}
-- 
1.6.3.2


>From fb12e447a0473913b4c47d804a024c15b1d85c9f Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Tue, 1 Sep 2009 12:25:01 -0600
Subject: [PATCH 2/2] dirent-safer: use it

* lib/backupfile.c (includes): Use "dirent--.h", since
numbered_backup can write to stderr during readdir.
* lib/fts.c (includes) [!_LIBC]: Likewise.
* lib/savedir.c (includes): Likewise.
* lib/getcwd.c: Document why opendir_safer is unused.
* lib/glob.c: Likewise.
* lib/scandir.c: Likewise.
* modules/backupfile (Depends-on): Add dirent-safer.
* modules/fts (Depends-on): Likewise.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog          |   11 +++++++++++
 lib/backupfile.c   |    9 ++-------
 lib/fts.c          |    1 +
 lib/getcwd.c       |    7 +++++--
 lib/glob.c         |    7 +++++--
 lib/savedir.c      |    7 +------
 lib/scandir.c      |    8 ++++++++
 modules/backupfile |    1 +
 modules/fts        |    1 +
 modules/savedir    |    1 +
 10 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 44bd320..8c7ca9a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2009-09-01  Eric Blake  <address@hidden>

+       dirent-safer: use it
+       * lib/backupfile.c (includes): Use "dirent--.h", since
+       numbered_backup can write to stderr during readdir.
+       * lib/fts.c (includes) [!_LIBC]: Likewise.
+       * lib/savedir.c (includes): Likewise.
+       * lib/getcwd.c: Document why opendir_safer is unused.
+       * lib/glob.c: Likewise.
+       * lib/scandir.c: Likewise.
+       * modules/backupfile (Depends-on): Add dirent-safer.
+       * modules/fts (Depends-on): Likewise.
+
        dirent-safer: new module
        * modules/dirent-safer: New file.
        * lib/dirent--.h: Likewise.
diff --git a/lib/backupfile.c b/lib/backupfile.c
index 1420edd..f6cf737 100644
--- a/lib/backupfile.c
+++ b/lib/backupfile.c
@@ -1,7 +1,7 @@
 /* backupfile.c -- make Emacs style backup file names

    Copyright (C) 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006 Free Software
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2009 Free Software
    Foundation, Inc.

    This program is free software: you can redistribute it and/or modify
@@ -37,7 +37,7 @@

 #include <unistd.h>

-#include <dirent.h>
+#include "dirent--.h"
 #ifndef _D_EXACT_NAMLEN
 # define _D_EXACT_NAMLEN(dp) strlen ((dp)->d_name)
 #endif
@@ -80,11 +80,6 @@
    of `digit' even when the host does not conform to POSIX.  */
 #define ISDIGIT(c) ((unsigned int) (c) - '0' <= 9)

-/* The results of opendir() in this file are not used with dirfd and fchdir,
-   therefore save some unnecessary work in fchdir.c.  */
-#undef opendir
-#undef closedir
-
 /* The extension added to file names to produce a simple (as opposed
    to numbered) backup file name. */
 char const *simple_backup_suffix = "~";
diff --git a/lib/fts.c b/lib/fts.c
index a30e38a..6373e44 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -69,6 +69,7 @@ static char sccsid[] = "@(#)fts.c     8.6 (Berkeley) 8/14/94";

 #if ! _LIBC
 # include "fcntl--.h"
+# include "dirent--.h"
 # include "openat.h"
 # include "unistd--.h"
 # include "same-inode.h"
diff --git a/lib/getcwd.c b/lib/getcwd.c
index b9e57d3..72344ba 100644
--- a/lib/getcwd.c
+++ b/lib/getcwd.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1991-1999, 2004-2008 Free Software Foundation, Inc.
+/* Copyright (C) 1991-1999, 2004-2009 Free Software Foundation, Inc.
    This file is part of the GNU C Library.

    This program is free software: you can redistribute it and/or modify
@@ -103,7 +103,10 @@
 #endif

 /* The results of opendir() in this file are not used with dirfd and fchdir,
-   therefore save some unnecessary recursion in fchdir.c.  */
+   and we do not leak fds to any single-threaded code that could use stdio,
+   therefore save some unnecessary recursion in fchdir.c and opendir_safer.c.
+   FIXME - if the kernel ever adds support for multi-thread safety for
+   avoiding standard fds, then we should use opendir_safer.  */
 #undef opendir
 #undef closedir
 
diff --git a/lib/glob.c b/lib/glob.c
index 40cc9b3..42cd39b 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1991-2002, 2003, 2004, 2005, 2006, 2007, 2008
+/* Copyright (C) 1991-2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009
    Free Software Foundation, Inc.
    This file is part of the GNU C Library.

@@ -186,7 +186,10 @@ static const char *next_brace_sub (const char *begin, int 
flags) __THROW;

 #ifndef _LIBC
 /* The results of opendir() in this file are not used with dirfd and fchdir,
-   therefore save some unnecessary work in fchdir.c.  */
+   and we do not leak fds to any single-threaded code that could use stdio,
+   therefore save some unnecessary recursion in fchdir.c and opendir_safer.c.
+   FIXME - if the kernel ever adds support for multi-thread safety for
+   avoiding standard fds, then we should use opendir_safer.  */
 # undef opendir
 # undef closedir

diff --git a/lib/savedir.c b/lib/savedir.c
index 8400145..5e69d38 100644
--- a/lib/savedir.c
+++ b/lib/savedir.c
@@ -26,7 +26,7 @@

 #include <errno.h>

-#include <dirent.h>
+#include "dirent--.h"
 #ifndef _D_EXACT_NAMLEN
 # define _D_EXACT_NAMLEN(dp)   strlen ((dp)->d_name)
 #endif
@@ -41,11 +41,6 @@
 # define NAME_SIZE_DEFAULT 512
 #endif

-/* The results of opendir() in this file are not used with dirfd and fchdir,
-   therefore save some unnecessary work in fchdir.c.  */
-#undef opendir
-#undef closedir
-
 /* Return a freshly allocated string containing the file names
    in directory DIRP, separated by '\0' characters;
    the end is marked by two '\0' characters in a row.
diff --git a/lib/scandir.c b/lib/scandir.c
index 8b34070..54a74d5 100644
--- a/lib/scandir.c
+++ b/lib/scandir.c
@@ -45,6 +45,14 @@
 # define __opendir opendir
 # define __closedir closedir
 # define __set_errno(val) errno = (val)
+
+/* The results of opendir() in this file are not used with dirfd and fchdir,
+   and we do not leak fds to any single-threaded code that could use stdio,
+   therefore save some unnecessary recursion in fchdir.c and opendir_safer.c.
+   FIXME - if the kernel ever adds support for multi-thread safety for
+   avoiding standard fds, then we should use opendir_safer.  */
+# undef opendir
+# undef closedir
 #endif

 #ifndef SCANDIR_CANCEL
diff --git a/modules/backupfile b/modules/backupfile
index 3f8ccfd..aaf20f3 100644
--- a/modules/backupfile
+++ b/modules/backupfile
@@ -11,6 +11,7 @@ m4/backupfile.m4
 Depends-on:
 argmatch
 d-ino
+dirent-safer
 dirname
 memcmp
 stdbool
diff --git a/modules/fts b/modules/fts
index 38b2256..95313a0 100644
--- a/modules/fts
+++ b/modules/fts
@@ -11,6 +11,7 @@ Depends-on:
 cycle-check
 d-ino
 d-type
+dirent-safer
 dirfd
 fchdir
 fcntl-h
diff --git a/modules/savedir b/modules/savedir
index 4171b80..6699095 100644
--- a/modules/savedir
+++ b/modules/savedir
@@ -7,6 +7,7 @@ lib/savedir.c
 m4/savedir.m4

 Depends-on:
+dirent-safer
 fdopendir
 xalloc

-- 
1.6.3.2