On 11/02/2012 18:48, Nick Mathewson wrote:
> On Sat, Feb 11, 2012 at 10:34 AM, Ross Lagerwall
> <rosslagerw...@gmail.com> wrote:
>> In a multi-process/threaded environment, ev_util_read_file()
>> could leak fds to child processes when not using O_CLOEXEC/FD_CLOEXEC.
>
> Hm. I'm not sure I trust "#ifdef O_CLOEXEC" as a test for whether
> open() supports O_CLOEXEC. Generally, I'd prefer a solution that
> falls back gracefully if Libevent is built with headers from a newer
> libc/kernel and then run on an older kernel. (This is a real concern:
> it seems to happen to RHEL/Centos users pretty often.)
>
> Unfortunately, it seems we can't just do a pattern of
> if ((fd = open(path, flags|O_CLOEXEC)) == -1) {
> fd = open(path, flags);
> if (fd == -1) return -1;
> fcntl(fd,...)
> }
> because (if my tests are right) at least some implementations of
> open() ignore unrecognized flags.
>
> So given that, maybe the solution you suggest is the best we can do?
If we can't trust the compile-time value and we can't detect the runtime
behavior then I think the solution I suggested is a reasonable idea.
>
>
> Additionally, I see 3 other open()s: two in arc4random.c and one in
> devpoll.c. Maybe this means we want an evutil_* wrapper function to
> set CLOEXEC on fds.
>
> thoughts?
Attached is a patch which provides a new utility function,
evutil_open_closeonexec which is used instead in place of the various
open() calls.
>From 77afe122702fa8a667ab9b5fbc3095d58196a4b2 Mon Sep 17 00:00:00 2001
From: Ross Lagerwall <rosslagerw...@gmail.com>
Date: Sat, 11 Feb 2012 17:23:17 +0200
Subject: [PATCH] Make uses of open() close-on-exec safe by introducing
evutil_open_closeonexec.
In a multi-process/threaded environment, opening fds internally
without the close-on-exec flag could leak fds to child processes.
---
arc4random.c | 4 ++--
devpoll.c | 2 +-
evutil.c | 23 ++++++++++++++++++++++-
util-internal.h | 2 ++
4 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/arc4random.c b/arc4random.c
index 31f3842..fe0234c 100644
--- a/arc4random.c
+++ b/arc4random.c
@@ -261,7 +261,7 @@ arc4_seed_proc_sys_kernel_random_uuid(void)
unsigned char entropy[64];
int bytes, n, i, nybbles;
for (bytes = 0; bytes<ADD_ENTROPY; ) {
- fd = open("/proc/sys/kernel/random/uuid", O_RDONLY, 0);
+ fd = evutil_open_closeonexec("/proc/sys/kernel/random/uuid", O_RDONLY, 0);
if (fd < 0)
return -1;
n = read(fd, buf, sizeof(buf));
@@ -305,7 +305,7 @@ arc4_seed_urandom(void)
size_t n;
for (i = 0; filenames[i]; ++i) {
- fd = open(filenames[i], O_RDONLY, 0);
+ fd = evutil_open_closeonexec(filenames[i], O_RDONLY, 0);
if (fd<0)
continue;
n = read_all(fd, buf, sizeof(buf));
diff --git a/devpoll.c b/devpoll.c
index 6b46fc3..4be46e4 100644
--- a/devpoll.c
+++ b/devpoll.c
@@ -132,7 +132,7 @@ devpoll_init(struct event_base *base)
nfiles = rl.rlim_cur;
/* Initialize the kernel queue */
- if ((dpfd = open("/dev/poll", O_RDWR)) == -1) {
+ if ((dpfd = evutil_open_closeonexec("/dev/poll", O_RDWR, 0)) == -1) {
event_warn("open: /dev/poll");
mm_free(devpollop);
return (NULL);
diff --git a/evutil.c b/evutil.c
index 306f037..c51a191 100644
--- a/evutil.c
+++ b/evutil.c
@@ -96,6 +96,27 @@
#define stat _stati64
#endif
+int
+evutil_open_closeonexec(const char *pathname, int flags, mode_t mode)
+{
+ int fd;
+
+#ifdef O_CLOEXEC
+ flags |= O_CLOEXEC;
+#endif
+
+ fd = open(pathname, flags, mode);
+ if (fd < 0)
+ return -1;
+
+#if !defined(O_CLOEXEC) && defined(FD_CLOEXEC)
+ if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0)
+ return -1;
+#endif
+
+ return fd;
+}
+
/**
Read the contents of 'filename' into a newly allocated NUL-terminated
string. Set *content_out to hold this string, and *len_out to hold its
@@ -126,7 +147,7 @@ evutil_read_file(const char *filename, char **content_out, size_t *len_out,
mode |= O_BINARY;
#endif
- fd = open(filename, mode);
+ fd = evutil_open_closeonexec(filename, mode, 0);
if (fd < 0)
return -1;
if (fstat(fd, &st) || st.st_size < 0 ||
diff --git a/util-internal.h b/util-internal.h
index 52f61f2..0f91a01 100644
--- a/util-internal.h
+++ b/util-internal.h
@@ -163,6 +163,8 @@ char EVUTIL_TOLOWER(char c);
#define EVUTIL_UPCAST(ptr, type, field) \
((type *)(((char*)(ptr)) - evutil_offsetof(type, field)))
+int evutil_open_closeonexec(const char *pathname, int flags, mode_t mode);
+
int evutil_read_file(const char *filename, char **content_out, size_t *len_out,
int is_binary);
--
1.7.5.4
