[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [Libevent-users] [PATCH] Make evutil_read_file() close-on-exec safe.



On 11/02/2012 18:48, Nick Mathewson wrote:
> On Sat, Feb 11, 2012 at 10:34 AM, Ross Lagerwall
> <rosslagerwall@xxxxxxxxx> 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 <rosslagerwall@xxxxxxxxx>
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