[PATCH 2/2] dirent-safer: fix cloexec race

[Paul Eggert]

* lib/opendir-safer.c: Include fcntl.h instead of unistd-safer.h.
(opendir_safer): Use F_DUPFD_CLOEXEC.
* modules/dirent-safer (Depends-on): Add fcntl. Remove unistd-safer.
* tests/test-dirent-safer.c: Do not include unistd-safer.h,
as it is no longer a prerequisite. Use F_DUPFD_CLOEXEC
instead of dup_safer.
---
ChangeLog | 8 ++++++++
lib/opendir-safer.c | 4 ++--
modules/dirent-safer | 2 +-
tests/test-dirent-safer.c | 4 +---
4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c725f5d91..514b503e5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
2017-08-12 Paul Eggert <***@cs.ucla.edu>

+ dirent-safer: fix cloexec race
+ * lib/opendir-safer.c: Include fcntl.h instead of unistd-safer.h.
+ (opendir_safer): Use F_DUPFD_CLOEXEC.
+ * modules/dirent-safer (Depends-on): Add fcntl. Remove unistd-safer.
+ * tests/test-dirent-safer.c: Do not include unistd-safer.h,
+ as it is no longer a prerequisite. Use F_DUPFD_CLOEXEC
+ instead of dup_safer.
+
fts: fix cloexec races
* lib/fts.c [!_LIBC]: Do not include dirent--.h, unistd--.h, cloexec.h.
(opendirat, diropen): Use O_CLOEXEC instead of set_cloexec_flag.
diff --git a/lib/opendir-safer.c b/lib/opendir-safer.c
index b05ff549c..5f1f49e32 100644
--- a/lib/opendir-safer.c
+++ b/lib/opendir-safer.c
@@ -22,8 +22,8 @@
#include "dirent-safer.h"

#include <errno.h>
+#include <fcntl.h>
#include <unistd.h>
-#include "unistd-safer.h"

/* Like opendir, but do not clobber stdin, stdout, or stderr. */

@@ -49,7 +49,7 @@ opendir_safer (char const *name)
DIR *newdp;
int e;
#if HAVE_FDOPENDIR || GNULIB_FDOPENDIR
- int f = dup_safer (fd);
+ int f = fcntl (fd, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
if (f < 0)
{
e = errno;
diff --git a/modules/dirent-safer b/modules/dirent-safer
index b38c58e64..4ea4a2561 100644
--- a/modules/dirent-safer
+++ b/modules/dirent-safer
@@ -11,8 +11,8 @@ Depends-on:
dirent
closedir
dirfd
+fcntl
opendir
-unistd-safer

configure.ac:
gl_DIRENT_SAFER
diff --git a/tests/test-dirent-safer.c b/tests/test-dirent-safer.c
index 2f8c3cde2..8c4e4e358 100644
--- a/tests/test-dirent-safer.c
+++ b/tests/test-dirent-safer.c
@@ -25,8 +25,6 @@
#include <stdio.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. */
@@ -75,7 +73,7 @@ main (void)

#if HAVE_FDOPENDIR || GNULIB_TEST_FDOPENDIR
{
- int fd = dup_safer (dfd);
+ int fd = fcntl (dfd, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
ASSERT (STDERR_FILENO < fd);
dp = fdopendir (fd);
ASSERT (dp);

[Bruno Haible]

Hi Paul,

Post by Paul Eggert
(opendirat, diropen): Use O_CLOEXEC instead of set_cloexec_flag.

There are platforms which support O_CLOEXEC; on these platforms this commit
is an improvement (eliminate uncontrolled inheritance of fd if another thread
calls fork+exec()).

There are also platforms which don't support O_CLOEXEC; on these platforms
gnulib's fcntl.h substitute defines O_CLOEXEC to 0. So, adding it to
the open() or openat() arguments has no effect, and eliminating the
set_cloexec_flag is a regression: it introduces unwanted inheritance of fd
even in single-threaded programs.

Or did I miss something?

lib/popen-safer.c contains a helper function open_noinherit that
(a) is race-free on platformas which support O_CLOEXEC,
(b) prevents unwanted inheritance of fd in single-threaded programs.
How about
- moving this helper function to a separate module,
- adding a similar helper function for openat(),
- using these instead of unconditional uses of O_CLOEXEC ?

Alternatively, define O_CLOEXEC to a non-zero value on those platforms
that don't support it, and extend gnulib's open() and openat() wrappers
to support O_CLOEXEC. But this is more hairy because the platforms could,
at any time, introduce other O_* flags that happen to collide wit the value
we happen to pick for O_CLOEXEC.

Bruno

[Paul Eggert]

Thanks for pointing out the problem; I guess I was subconsciously hoping
that platforms lacking O_CLOEXEC were no longer a practical issue. But
that was silly.

Post by Bruno Haible
Alternatively, define O_CLOEXEC to a non-zero value on those platforms
that don't support it, and extend gnulib's open() and openat() wrappers
to support O_CLOEXEC. But this is more hairy because the platforms could,
at any time, introduce other O_* flags that happen to collide wit the value
we happen to pick for O_CLOEXEC.

Despite that disadvantage, it would be a win for users or Gnulib, who
could stop worrying about the possibility that O_CLOEXEC == 0. I
installed the attached patch to try to implement this. I don't use
MS-Windows, though, and may well have missed something.

[Bruno Haible]

Post by Paul Eggert
Despite that disadvantage, it would be a win for users or Gnulib, who
could stop worrying about the possibility that O_CLOEXEC == 0. I
installed the attached patch to try to implement this. I don't use
MS-Windows, though, and may well have missed something.

Looks quite nice. I've verified the value of O_CLOEXEC fits:

1) All systems appear to "book" the O_* bit values, starting with the
small ones.

Linux max is 0x400000
Hurd max is 0x400000
Mac OS X 10.7 max is 0x200000
FreeBSD 6.4 max is 0x10000
NetBSD 5.0 max is 0x80000
OpenBSD 3.8 max is 0x8000
MirBSD max is 0x8000
Minix 3.1.8 max is 0x1000
AIX 7.1 collision with _FKERNEL - probably harmless
HP-UX 11.31 max is 0x1000000
IRIX 6.5 max is 0x40000
OSF/1 5.1 max is 0x40000
Solaris 11 2011-11 max is 0x2000000
Cygwin max is 0x100000
mingw max is 0x40000
MSVC 14 max is 0x40000
Interix 3.5 max is 0x2000
BeOS max is 0x8000

2) The collision with _FKERNEL on AIX is probably harmless, since this flag
probably only exists in kernel code.

Here's an update of the documentation:


2017-08-14 Bruno Haible <***@clisp.org>

open, openat: Update doc about O_CLOEXEC.
* doc/posix-functions/open.texi: More concrete list of platforms.
* doc/posix-functions/openat.texi: Likewise.

diff --git a/doc/posix-functions/open.texi b/doc/posix-functions/open.texi
index f1565c3..7e9df26 100644
--- a/doc/posix-functions/open.texi
+++ b/doc/posix-functions/open.texi
@@ -10,7 +10,7 @@ Portability problems fixed by the Gnulib module open:
@itemize
@item
Some platforms do not support @code{O_CLOEXEC}:
-Solaris 10, probably many others.
+Mac OS X 10.5, FreeBSD 6.0, NetBSD 5.0, OpenBSD 3.8, Minix 3.1.8, AIX 5.1, HP-UX 11, IRIX 6.5, OSF/1 5.1, Solaris 10, Cygwin, mingw, MSVC 14, Interix 3.5.
@item
On platforms where @code{off_t} is a 32-bit type, @code{open} may not work
correctly with files larger than 2 GB. (Cf. @code{AC_SYS_LARGEFILE}.)
diff --git a/doc/posix-functions/openat.texi b/doc/posix-functions/openat.texi
index 9f7632b..4a20636 100644
--- a/doc/posix-functions/openat.texi
+++ b/doc/posix-functions/openat.texi
@@ -15,7 +15,7 @@ AIX 5.1, HP-UX 11, IRIX 6.5, OSF/1 5.1, Cygwin 1.5.x, mingw, MSVC 14, Interix 3.
But the replacement function is not safe to be used in libraries and is not multithread-safe.
@item
Some platforms do not support @code{O_CLOEXEC}:
-Solaris 10.
+AIX 7.1, Solaris 10.
@item
On platforms where @code{off_t} is a 32-bit type, @code{open} may not work
correctly with files larger than 2 GB. (Cf. @code{AC_SYS_LARGEFILE}.)