Re: need opendir_safer, dirent--.h

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Jim Meyering on 9/2/2009 12:55 AM:
>> 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.

And here's openat-safer, plus a rework of making fts safer.  With these
patches, plus a patch to findutils to consistently use the *_safer
functions, there are no more collisions with stdin in my original
findutils example (tested with both cygwin 1.5 which lacks openat and
fdopendir, and cygwin 1.7 which has both), so I'll be pushing the series
once I rebase on top of Ralf's .m4 cleanups.

Next, I'll be working on moving the declaration of openat and friends into
their respective POSIX 2008 headers (<fcntl.h>, <sys/stat.h>, <unistd.h>),
rather than requiring standard programs to use "openat.h".  And
eventually, we'll need to provide an rpl_openat even on Linux that has
openat, in order to provide O_CLOEXEC semantics on older kernels compiled
against newer glibc, as part of the suite of CLOEXEC improvements.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqewOIACgkQ84KuGfSFAYBtfwCfSVnDoro8sJo7cpVV/yx/ZMrp
5dUAn2lbmSneGHIlhWuoOqDeXw9fYCmv
=diWp
-----END PGP SIGNATURE-----

>From bdd72e423478ccb9fd93d47d136ffae9340b5384 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Wed, 2 Sep 2009 06:07:54 -0600
Subject: [PATCH 1/2] openat-safer: new module

* modules/openat-safer: New file.
* lib/openat-safer.c: Likewise.
* m4/fcntl-safer.m4 (gl_OPENAT_SAFER): New macro.
* lib/fcntl-safer.h (openat_safer): Declare.
* lib/fcntl--.h (openat): Override.
* MODULES.html.sh (File descriptor based I/O): Mention it.
* lib/openat.h: Add double-inclusion guards.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog            |   11 +++++++++++
 MODULES.html.sh      |    1 +
 lib/fcntl--.h        |    8 +++++++-
 lib/fcntl-safer.h    |    6 +++++-
 lib/openat-safer.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/openat.h         |    5 +++++
 m4/fcntl-safer.m4    |    8 +++++++-
 modules/openat-safer |   28 ++++++++++++++++++++++++++++
 8 files changed, 111 insertions(+), 3 deletions(-)
 create mode 100644 lib/openat-safer.c
 create mode 100644 modules/openat-safer

diff --git a/ChangeLog b/ChangeLog
index e9fa04a..2b56545 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2009-09-02  Eric Blake  <address@hidden>
+
+       openat-safer: new module
+       * modules/openat-safer: New file.
+       * lib/openat-safer.c: Likewise.
+       * m4/fcntl-safer.m4 (gl_OPENAT_SAFER): New macro.
+       * lib/fcntl-safer.h (openat_safer): Declare.
+       * lib/fcntl--.h (openat): Override.
+       * MODULES.html.sh (File descriptor based I/O): Mention it.
+       * lib/openat.h: Add double-inclusion guards.
+
 2009-09-01  Eric Blake  <address@hidden>

        dirent-safer: new module
diff --git a/MODULES.html.sh b/MODULES.html.sh
index bb99642..bcecc55 100755
--- a/MODULES.html.sh
+++ b/MODULES.html.sh
@@ -2509,6 +2509,7 @@ func_all_modules ()

   func_begin_table
   func_module fcntl-safer
+  func_module openat-safer
   func_module safe-read
   func_module safe-write
   func_module full-read
diff --git a/lib/fcntl--.h b/lib/fcntl--.h
index 5a6a879..9e311ce 100644
--- a/lib/fcntl--.h
+++ b/lib/fcntl--.h
@@ -1,6 +1,6 @@
 /* Like fcntl.h, but redefine some names to avoid glitches.

-   Copyright (C) 2005 Free Software Foundation, Inc.
+   Copyright (C) 2005, 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
@@ -25,3 +25,9 @@

 #undef creat
 #define creat creat_safer
+
+#if GNULIB_OPENAT_SAFER
+# include "openat.h" /* FIXME - <fcntl.h> should be sufficient.  */
+# undef openat
+# define openat openat_safer
+#endif
diff --git a/lib/fcntl-safer.h b/lib/fcntl-safer.h
index 99f3865..2b10ba6 100644
--- a/lib/fcntl-safer.h
+++ b/lib/fcntl-safer.h
@@ -1,6 +1,6 @@
 /* Invoke fcntl-like functions, but avoid some glitches.

-   Copyright (C) 2005 Free Software Foundation, Inc.
+   Copyright (C) 2005, 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
@@ -21,3 +21,7 @@

 int open_safer (char const *, int, ...);
 int creat_safer (char const *, mode_t);
+
+#if GNULIB_OPENAT_SAFER
+int openat_safer (int, char const *, int, ...);
+#endif
diff --git a/lib/openat-safer.c b/lib/openat-safer.c
new file mode 100644
index 0000000..f6977fd
--- /dev/null
+++ b/lib/openat-safer.c
@@ -0,0 +1,47 @@
+/* Invoke openat, but avoid some glitches.
+
+   Copyright (C) 2005, 2006, 2008-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 Paul Eggert for open, ported by Eric Blake for openat.  */
+
+#include <config.h>
+
+#include "fcntl-safer.h"
+
+#include <fcntl.h>
+#include "openat.h" /* FIXME - <fcntl.h> should be sufficient.  */
+#include <stdarg.h>
+#include "unistd-safer.h"
+
+int
+openat_safer (int fd, char const *file, int flags, ...)
+{
+  mode_t mode = 0;
+
+  if (flags & O_CREAT)
+    {
+      va_list ap;
+      va_start (ap, flags);
+
+      /* We have to use PROMOTED_MODE_T instead of mode_t, otherwise GCC 4
+        creates crashing code when 'mode_t' is smaller than 'int'.  */
+      mode = va_arg (ap, PROMOTED_MODE_T);
+
+      va_end (ap);
+    }
+
+  return fd_safer (openat (fd, file, flags, mode));
+}
diff --git a/lib/openat.h b/lib/openat.h
index 4072c94..df52691 100644
--- a/lib/openat.h
+++ b/lib/openat.h
@@ -16,6 +16,9 @@

 /* written by Jim Meyering */

+#ifndef _GL_HEADER_OPENAT
+#define _GL_HEADER_OPENAT
+
 #include <fcntl.h>

 #include <sys/types.h>
@@ -120,3 +123,5 @@ lchmodat (int fd, char const *file, mode_t mode)
 {
   return fchmodat (fd, file, mode, AT_SYMLINK_NOFOLLOW);
 }
+
+#endif /* _GL_HEADER_OPENAT */
diff --git a/m4/fcntl-safer.m4 b/m4/fcntl-safer.m4
index e2080df..365e221 100644
--- a/m4/fcntl-safer.m4
+++ b/m4/fcntl-safer.m4
@@ -1,4 +1,4 @@
-#serial 6
+#serial 7
 dnl Copyright (C) 2005-2007, 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,
@@ -11,3 +11,9 @@ AC_DEFUN([gl_FCNTL_SAFER],
   # Prerequisites of lib/open-safer.c.
   AC_REQUIRE([gl_PROMOTED_TYPE_MODE_T])
 ])
+
+AC_DEFUN([gl_OPENAT_SAFER],
+[
+  AC_REQUIRE([gl_FCNTL_SAFER])
+  AC_LIBOBJ([openat-safer])
+])
diff --git a/modules/openat-safer b/modules/openat-safer
new file mode 100644
index 0000000..d595883
--- /dev/null
+++ b/modules/openat-safer
@@ -0,0 +1,28 @@
+Description:
+openat function that avoids clobbering std{in,out,err}.
+
+Files:
+lib/fcntl--.h
+lib/fcntl-safer.h
+lib/openat-safer.c
+m4/fcntl-safer.m4
+
+Depends-on:
+fcntl-safer
+openat
+unistd-safer
+
+configure.ac:
+gl_OPENAT_SAFER
+gl_MODULE_INDICATOR([openat-safer])
+
+Makefile.am:
+
+Include:
+"fcntl-safer.h"
+
+License:
+GPL
+
+Maintainer:
+Eric Blake
-- 
1.6.3.3.334.g916e1


>From 0910cee25c7237002895b599b6e1acedcda773e3 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] backupfile, chdir-long, fts, savedir: make safer

* lib/backupfile.c (includes): Use "dirent--.h", since
numbered_backup can write to stderr during readdir.
* lib/savedir.c (includes): Likewise.
* lib/chdir-long.c (includes): Use "fcntl--.h", since openat
emulation can write to stderr on failure.
* lib/fts.c (includes) [!_LIBC]: Likewise for opendir and openat.
* lib/openat.c (includes): Only include "fcntl-safer.h", not
"fcntl--.h", so we can implement openat.
(openat_permissive)
* lib/getcwd.c: Document why opendir_safer is unused.
* lib/glob.c: Likewise.
* lib/scandir.c: Likewise.
* lib/openat-proc.c: Likewise, for open_safer.
* modules/backupfile (Depends-on): Add dirent-safer.
* modules/fts (Depends-on): Add dirent-safer and openat-safer.
* modules/chdir-long (Depends-on): Add openat-safer.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog          |   18 ++++++++++++++++++
 lib/backupfile.c   |    9 ++-------
 lib/chdir-long.c   |    6 +++++-
 lib/fts.c          |    2 +-
 lib/getcwd.c       |    8 ++++++--
 lib/glob.c         |    7 +++++--
 lib/openat-proc.c  |    7 +++++--
 lib/openat.c       |    8 +++++++-
 lib/savedir.c      |    7 +------
 lib/scandir.c      |    8 ++++++++
 modules/backupfile |    1 +
 modules/chdir-long |    2 +-
 modules/fts        |    3 ++-
 modules/savedir    |    1 +
 14 files changed, 63 insertions(+), 24 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2b56545..ee81a23 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,23 @@
 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.
+       * lib/savedir.c (includes): Likewise.
+       * lib/chdir-long.c (includes): Use "fcntl--.h", since openat
+       emulation can write to stderr on failure.
+       * lib/fts.c (includes) [!_LIBC]: Likewise for opendir and openat.
+       * lib/openat.c (includes): Only include "fcntl-safer.h", not
+       "fcntl--.h", so we can implement openat.
+       (openat_permissive)
+       * lib/getcwd.c: Document why opendir_safer is unused.
+       * lib/glob.c: Likewise.
+       * lib/scandir.c: Likewise.
+       * lib/openat-proc.c: Likewise, for open_safer.
+       * modules/backupfile (Depends-on): Add dirent-safer.
+       * modules/fts (Depends-on): Add dirent-safer and openat-safer.
+       * modules/chdir-long (Depends-on): Add openat-safer.
+
        openat-safer: new module
        * modules/openat-safer: New file.
        * lib/openat-safer.c: 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/chdir-long.c b/lib/chdir-long.c
index 291b58c..7840626 100644
--- a/lib/chdir-long.c
+++ b/lib/chdir-long.c
@@ -1,5 +1,5 @@
 /* provide a chdir function that tries not to fail due to ENAMETOOLONG
-   Copyright (C) 2004, 2005, 2006, 2007, 2008 Free Software Foundation, Inc.
+   Copyright (C) 2004-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
@@ -30,6 +30,10 @@

 #include "openat.h"

+#if GNULIB_OPENAT_SAFER
+# include "fcntl--.h"
+#endif
+
 #ifndef PATH_MAX
 # error "compile this file only if your system defines PATH_MAX"
 #endif
diff --git a/lib/fts.c b/lib/fts.c
index a30e38a..7616c6f 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -69,7 +69,7 @@ static char sccsid[] = "@(#)fts.c     8.6 (Berkeley) 8/14/94";

 #if ! _LIBC
 # include "fcntl--.h"
-# include "openat.h"
+# include "dirent--.h"
 # include "unistd--.h"
 # include "same-inode.h"
 #endif
diff --git a/lib/getcwd.c b/lib/getcwd.c
index b9e57d3..2da1aee 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,11 @@
 #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.
+   FIXME - if the kernel ever adds support for multi-thread safety for
+   avoiding standard fds, then we should use opendir_safer and
+   openat_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/openat-proc.c b/lib/openat-proc.c
index e84dc45..8057033 100644
--- a/lib/openat-proc.c
+++ b/lib/openat-proc.c
@@ -1,6 +1,6 @@
 /* Create /proc/self/fd-related names for subfiles of open directories.

-   Copyright (C) 2006 Free Software Foundation, Inc.
+   Copyright (C) 2006, 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
@@ -34,7 +34,10 @@
 #include "xalloc.h"

 /* The results of open() in this file are not used with 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 work in fchdir.c.
+   FIXME - if the kernel ever adds support for multi-thread safety for
+   avoiding standard fds, then we should use open_safer.  */
 #undef open
 #undef close

diff --git a/lib/openat.c b/lib/openat.c
index 7a68cba..0e2c27b 100644
--- a/lib/openat.c
+++ b/lib/openat.c
@@ -25,10 +25,16 @@
 #include <sys/stat.h>

 #include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
-#include "fcntl--.h"
 #include "openat-priv.h"
 #include "save-cwd.h"

+/* We can't use "fcntl--.h", so that openat_safer does not interfere.  */
+#if GNULIB_FCNTL_SAFER
+# include "fcntl-safer.h"
+# undef open
+# define open open_safer
+#endif
+
 /* Replacement for Solaris' openat function.
    <http://www.google.com/search?q=openat+site:docs.sun.com>
    First, try to simulate it via open ("/proc/self/fd/FD/FILE").
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/chdir-long b/modules/chdir-long
index 4025b45..cdcb9eb 100644
--- a/modules/chdir-long
+++ b/modules/chdir-long
@@ -10,7 +10,7 @@ Depends-on:
 atexit
 fchdir
 fcntl-h
-openat
+openat-safer
 memchr
 mempcpy
 memrchr
diff --git a/modules/fts b/modules/fts
index 38b2256..f80a827 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
@@ -19,7 +20,7 @@ hash
 i-ring
 lstat
 memmove
-openat
+openat-safer
 stdbool
 unistd-safer

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.3.334.g916e1