Re: [PATCH 1/2] fts: fix cloexec races
Paul Eggert wrote:
> 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 0x40
Hurdmax is 0x40
Mac OS X 10.7 max is 0x20
FreeBSD 6.4 max is 0x1
NetBSD 5.0 max is 0x8
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 0x100
IRIX 6.5max is 0x4
OSF/1 5.1 max is 0x4
Solaris 11 2011-11 max is 0x200
Cygwin max is 0x10
mingw max is 0x4
MSVC 14 max is 0x4
Interix 3.5 max is 0x2000
BeOSmax 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
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}.)
Re: [PATCH 1/2] fts: fix cloexec races
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. On 08/14/2017 10:18 AM, Bruno Haible wrote: 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. >From 9830b8cbec084211d3169bbb074317053cb1088e Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 14 Aug 2017 13:04:46 -0700 Subject: [PATCH] open: support O_CLOEXEC * NEWS, doc/posix-functions/open.texi: * doc/posix-functions/openat.texi: Document this. * lib/fcntl.in.h (O_CLOEXEC): Default to a nonzero value. (GNULIB_defined_O_CLOEXEC): New symbol. * lib/open.c: Include cloexec.h. (open): Support O_CLOEXEC. * lib/openat.c: Include cloexec.h. (rpl_openat): Support O_CLOEXEC. * lib/popen-safer.c: Do not include cloexec.h. (open_noinherit): Remove. (popen_safer): Use O_CLOEXEC instead of set_cloexec_flag. * lib/save-cwd.c: Do not include cloexec.h. (save_cwd): Use O_CLOEXEC instead of set_cloexec_flag. * m4/open-cloexec.m4: New file. * m4/open.m4 (gl_FUNC_OPEN): Require gl_PREPROC_O_CLOEXEC. Replace 'open' if O_CLOEXEC is not present. * m4/openat.m4 (gl_FUNC_OPENAT): Require gl_PREPROC_O_CLOEXEC. Replace 'openat' if O_CLOEXEC is not present. * modules/freopen (Depends-on): Depend on 'open' if replacing freopen. * modules/open (Files): Add m4/open-cloexec.m4. (Depends-on): Depend on cloexec if replacing 'open'. * modules/openat (Files): Add m4/open-cloexec.m4. (Depends-on): Depend on cloexec if replacing openat. * modules/popen-safer (Depends-on): Remove cloexec. * modules/save-cwd (Depends-on): Remove cloexec, and add fd-safer-flag and 'open'. --- ChangeLog | 31 +++ NEWS| 4 doc/posix-functions/open.texi | 6 ++ doc/posix-functions/openat.texi | 6 ++ lib/fcntl.in.h | 5 - lib/open.c | 29 - lib/openat.c| 32 ++-- lib/popen-safer.c | 35 +-- lib/save-cwd.c | 6 ++ m4/open-cloexec.m4 | 21 + m4/open.m4 | 6 +- m4/openat.m4| 8 +--- modules/freopen | 1 + modules/open| 2 ++ modules/openat | 2 ++ modules/popen-safer | 1 - modules/save-cwd| 5 +++-- 17 files changed, 151 insertions(+), 49 deletions(-) create mode 100644 m4/open-cloexec.m4 diff --git a/ChangeLog b/ChangeLog index a07e17251..7f1a5c2e9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,34 @@ +2017-08-14 Paul Eggert + + + open: support O_CLOEXEC + * NEWS, doc/posix-functions/open.texi: + * doc/posix-functions/openat.texi: Document this. + * lib/fcntl.in.h (O_CLOEXEC): Default to a nonzero value. + (GNULIB_defined_O_CLOEXEC): New symbol. + * lib/open.c: Include cloexec.h. + (open): Support O_CLOEXEC. + * lib/openat.c: Include cloexec.h. + (rpl_openat): Support O_CLOEXEC. + * lib/popen-safer.c: Do not include cloexec.h. + (open_noinherit): Remove. + (popen_safer): Use O_CLOEXEC instead of set_cloexec_flag. + * lib/save-cwd.c: Do not include cloexec.h. + (save_cwd): Use O_CLOEXEC instead of set_cloexec_flag. + * m4/open-cloexec.m4: New file. + * m4/open.m4 (gl_FUNC_OPEN): Require gl_PREPROC_O_CLOEXEC. + Replace 'open' if O_CLOEXEC is not present. + * m4/openat.m4 (gl_FUNC_OPENAT): Require gl_PREPROC_O_CLOEXEC. + Replace 'openat' if O_CLOEXEC is not present. + * modules/freopen (Depends-on): Depend on 'open' if replacing freopen. + * modules/open (Files): Add m4/open-cloexec.m4. + (Depends-on): Depend on cloexec if replacing 'open'. + * modules/openat (Files): Add m4/open-cloexec.m4. + (Depends-on): Depend on cloexec if replacing openat. + * modules/popen-safer (Depends-on): Remove cloexec. + * modules/save-cwd (Depends-on): Remove cloexec, and add + fd-safer-flag and 'open'. + 2017-08-13 Paul Eggert reallocarray: minor fixes diff --git a/NEWS b/NEWS index 8fb2c54dd..d3a300a30 100644 --- a/NEWS +++ b/NEWS @@ -42,6 +42,10 @@ User visible incompatible changes DateModules Changes +2017-08-14 fcntl-h This module now defaults O_CLOEXEC to a nonzero +value instead of to 0, as the 'open' and +'openat' modules now emulate O_C
Re: [PATCH 1/2] fts: fix cloexec races
Hi Paul, > (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
[PATCH 1/2] 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.
(fts_build): Use F_DUPD_CLOEXEC rinstad of set_cloexec_flag.
(fd_ring_check): Set cloexec flag on new file descriptors.
(fts_build, fd_ring_check): While we’re at it, make sure the
resulting file descriptor is not 0, 1, or 2, since that is easy.
* modules/fts (Depends-on): Remove cloexec, dirent-safer, dup,
fcntl-safer, unistd-safer. Add fcntl.
---
ChangeLog | 10 ++
lib/fts.c | 27 ---
modules/fts | 6 +-
3 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 4b475437d..c725f5d91 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2017-08-12 Paul Eggert
+
+ 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.
+ (fts_build): Use F_DUPD_CLOEXEC rinstad of set_cloexec_flag.
+ (fd_ring_check): Set cloexec flag on new file descriptors.
+ (fts_build, fd_ring_check): While we’re at it, make sure the
+ resulting file descriptor is not 0, 1, or 2, since that is easy.
+
2017-08-11 Bruno Haible
fts tests: Fix link error.
diff --git a/lib/fts.c b/lib/fts.c
index bf8edf821..ffa41130a 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -71,11 +71,6 @@ static char sccsid[] = "@(#)fts.c 8.6 (Berkeley)
8/14/94";
#if ! _LIBC
# 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 "flexmember.h"
# include "openat.h"
# include "same-inode.h"
@@ -305,14 +300,13 @@ static DIR *
internal_function
opendirat (int fd, char const *dir, int extra_flags, int *pdir_fd)
{
- int new_fd = openat (fd, dir,
- (O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK
-| extra_flags));
+ int open_flags = (O_RDONLY | O_CLOEXEC | O_DIRECTORY | O_NOCTTY
+| O_NONBLOCK | extra_flags);
+ int new_fd = openat (fd, dir, open_flags);
DIR *dirp;
if (new_fd < 0)
return NULL;
- set_cloexec_flag (new_fd, true);
dirp = fdopendir (new_fd);
if (dirp)
*pdir_fd = new_fd;
@@ -375,15 +369,13 @@ static int
internal_function
diropen (FTS const *sp, char const *dir)
{
- int open_flags = (O_SEARCH | O_DIRECTORY | O_NOCTTY | O_NONBLOCK
+ int open_flags = (O_SEARCH | O_CLOEXEC | O_DIRECTORY | O_NOCTTY | O_NONBLOCK
| (ISSET (FTS_PHYSICAL) ? O_NOFOLLOW : 0)
| (ISSET (FTS_NOATIME) ? O_NOATIME : 0));
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;
}
@@ -1435,11 +1427,7 @@ fts_build (register FTS *sp, int type)
if (descend || type == BREAD)
{
if (ISSET(FTS_CWDFD))
- {
-dir_fd = dup (dir_fd);
-if (0 <= dir_fd)
- set_cloexec_flag (dir_fd, true);
- }
+ dir_fd = fcntl (dir_fd, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
if (dir_fd < 0 || fts_safe_changedir(sp, cur, dir_fd, NULL)) {
if (descend && type == BREAD)
cur->fts_errno = errno;
@@ -1781,7 +1769,7 @@ fd_ring_check (FTS const *sp)
I_ring fd_w = sp->fts_fd_ring;
int cwd_fd = sp->fts_cwd_fd;
- cwd_fd = dup (cwd_fd);
+ cwd_fd = fcntl (cwd_fd, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
char *dot = getcwdat (cwd_fd, NULL, 0);
error (0, 0, "= check = cwd: %s", dot);
free (dot);
@@ -1790,7 +1778,8 @@ fd_ring_check (FTS const *sp)
int fd = i_ring_pop (&fd_w);
if (0 <= fd)
{
- int parent_fd = openat (cwd_fd, "..", O_SEARCH | O_NOATIME);
+ int open_flags = O_SEARCH | O_CLOEXEC | O_NOATIME;
+ int parent_fd = openat (cwd_fd, "..", open_flags);
if (parent_fd < 0)
{
// Warn?
diff --git a/modules/fts b/modules/fts
index 55c09e7c9..ecbca70b5 100644
--- a/modules/fts
+++ b/modules/fts
@@ -8,16 +8,13 @@ lib/fts-cycle.c
m4/fts.m4
Depends-on:
-cloexec
closedir
cycle-check
d-ino
d-type
-dirent-safer
-dup
fchdir
+fcntl
fcntl-h
-fcntl-safer
fdopendir
flexmember
fstat
@@ -32,7 +29,6 @@ readdir
stdalign
stdbool
stddef
-unistd-safer
configure.ac:
gl_FUNC_FTS
--
2.13.4
