conmon: store open FDs and close only them

now that we use a delay to call the cleanup program, we might end up
in a race where the event fd used by glib is close'd and it causes the
glib event handler to keep polling the closed file descriptor in a
tight loop.

To avoid closing files that are handled by glib, store what FDs are
opened when conmon first started and close only them.

Closes: #221

Signed-off-by: Giuseppe Scrivano <[email protected]>

Makefile

@@ -6,7 +6,7 @@ GO ?= go
 PROJECT := github.com/containers/conmon
 PKG_CONFIG ?= pkg-config
 HEADERS := $(wildcard src/*.h)
-OBJS := src/conmon.o src/cmsg.o src/ctr_logging.o src/utils.o src/cli.o src/globals.o src/cgroup.o src/conn_sock.o src/oom.o src/ctrl.o src/ctr_stdio.o src/parent_pipe_fd.o src/ctr_exit.o src/runtime_args.o
+OBJS := src/conmon.o src/cmsg.o src/ctr_logging.o src/utils.o src/cli.o src/globals.o src/cgroup.o src/conn_sock.o src/oom.o src/ctrl.o src/ctr_stdio.o src/parent_pipe_fd.o src/ctr_exit.o src/runtime_args.o src/close_fds.o
 DEBUGTAG ?=
 ifneq (,$(findstring enable_debug,$(DEBUGTAG)))
 	DEBUGFLAG=-g

meson.build

@@ -58,6 +58,8 @@ executable('conmon',
             'src/ctr_stdio.h',
             'src/globals.c',
             'src/globals.h',
+            'src/close_fds.c',
+            'src/close_fds.h',
             'src/oom.c',
             'src/oom.h',
             'src/parent_pipe_fd.c',

src/close_fds.c

@@ -0,0 +1,83 @@
+#define _GNU_SOURCE
+#if __STDC_VERSION__ >= 199901L
+/* C99 or later */
+#else
+#error conmon.c requires C99 or later
+#endif
+
+#include "utils.h"
+#include "ctr_logging.h"
+#include "cgroup.h"
+#include "cli.h"
+#include "globals.h"
+#include "oom.h"
+#include "conn_sock.h"
+#include "ctrl.h"
+#include "ctr_stdio.h"
+#include "config.h"
+#include "parent_pipe_fd.h"
+#include "ctr_exit.h"
+#include "close_fds.h"
+#include "runtime_args.h"
+
+#include <sys/prctl.h>
+#include <sys/stat.h>
+
+static int open_files_max_fd;
+static fd_set *open_files_set;
+
+static void __attribute__((constructor)) init()
+{
+	struct dirent *ent;
+	ssize_t size = 0;
+	DIR *d;
+
+	/* Store how many FDs were open before the Go runtime kicked in.  */
+	d = opendir("/proc/self/fd");
+	if (!d)
+		return;
+
+	for (ent = readdir(d); ent; ent = readdir(d)) {
+		int fd;
+
+		if (ent->d_name[0] == '.')
+			continue;
+
+		fd = atoi(ent->d_name);
+		if (fd == dirfd(d))
+			continue;
+
+		if (fd >= size * FD_SETSIZE) {
+			int i;
+			ssize_t new_size;
+
+			new_size = (fd / FD_SETSIZE) + 1;
+			open_files_set = realloc(open_files_set, new_size * sizeof(fd_set));
+			if (open_files_set == NULL)
+				_exit(EXIT_FAILURE);
+
+			for (i = size; i < new_size; i++)
+				FD_ZERO(&(open_files_set[i]));
+
+			size = new_size;
+		}
+
+		if (fd > open_files_max_fd)
+			open_files_max_fd = fd;
+
+		FD_SET(fd % FD_SETSIZE, &(open_files_set[fd / FD_SETSIZE]));
+	}
+	closedir(d);
+}
+
+void close_other_fds()
+{
+	int fd;
+
+	for (fd = 3; fd < open_files_max_fd; fd++) {
+		if (open_files_set == NULL || FD_ISSET(fd % FD_SETSIZE, &(open_files_set[fd / FD_SETSIZE])))
+			if (fd == sync_pipe_fd || fd == attach_pipe_fd || fd == dev_null_r || fd == dev_null_w || fd == oom_cgroup_fd
+			    || fd == oom_event_fd)
+				close(fd);
+	}
+}

src/close_fds.h

@@ -0,0 +1 @@
+void close_other_fds();

src/conmon.c

@@ -17,6 +17,7 @@
 #include "config.h"
 #include "parent_pipe_fd.h"
 #include "ctr_exit.h"
+#include "close_fds.h"
 #include "runtime_args.h"
 
 #include <sys/prctl.h>
@@ -27,8 +28,8 @@ int main(int argc, char *argv[])
 	_cleanup_gerror_ GError *err = NULL;
 	char buf[BUF_SIZE];
 	int num_read;
-	_cleanup_close_ int dev_null_r = -1;
-	_cleanup_close_ int dev_null_w = -1;
+	_cleanup_close_ int dev_null_r_cleanup = -1;
+	_cleanup_close_ int dev_null_w_cleanup = -1;
 	_cleanup_close_ int dummyfd = -1;
 
 	int initialize_ec = initialize_cli(argc, argv);
@@ -58,11 +59,11 @@ int main(int argc, char *argv[])
 			close(start_pipe_fd);
 	}
 
-	dev_null_r = open("/dev/null", O_RDONLY | O_CLOEXEC);
+	dev_null_r_cleanup = dev_null_r = open("/dev/null", O_RDONLY | O_CLOEXEC);
 	if (dev_null_r < 0)
 		pexit("Failed to open /dev/null");
 
-	dev_null_w = open("/dev/null", O_WRONLY | O_CLOEXEC);
+	dev_null_w_cleanup = dev_null_w = open("/dev/null", O_WRONLY | O_CLOEXEC);
 	if (dev_null_w < 0)
 		pexit("Failed to open /dev/null");
 
@@ -97,7 +98,6 @@ int main(int argc, char *argv[])
 	/* Environment variables */
 	sync_pipe_fd = get_pipe_fd_from_env("_OCI_SYNCPIPE");
 
-	int attach_pipe_fd = -1;
 	if (opt_attach) {
 		attach_pipe_fd = get_pipe_fd_from_env("_OCI_ATTACHPIPE");
 		if (attach_pipe_fd < 0) {
@@ -465,24 +465,8 @@ int main(int argc, char *argv[])
 	 * the container runs.  Close them before we notify the container exited, so that they can be
 	 * reused immediately.
 	 */
-	DIR *fdsdir = opendir("/proc/self/fd");
-	if (fdsdir != NULL) {
-		int fd;
-		int dfd = dirfd(fdsdir);
-		struct dirent *next;
-
-		for (next = readdir(fdsdir); next; next = readdir(fdsdir)) {
-			const char *name = next->d_name;
-			if (name[0] == '.')
-				continue;
-
-			fd = strtoll(name, NULL, 10);
-			if (fd == dfd || fd == sync_pipe_fd || fd == attach_pipe_fd || fd == dev_null_r || fd == dev_null_w)
-				continue;
-			close(fd);
-		}
-		closedir(fdsdir);
-	}
+	close_other_fds();
+	close_all_readers();
 
 	_cleanup_free_ char *status_str = g_strdup_printf("%d", exit_status);
 

src/globals.c

@@ -13,6 +13,9 @@ int terminal_ctrl_fd = -1;
 int inotify_fd = -1;
 int winsz_fd_w = -1;
 int winsz_fd_r = -1;
+int attach_pipe_fd = -1;
+int dev_null_r = -1;
+int dev_null_w = -1;
 
 gboolean timed_out = FALSE;
 

src/globals.h

@@ -18,6 +18,9 @@ extern int terminal_ctrl_fd;
 extern int inotify_fd;
 extern int winsz_fd_w;
 extern int winsz_fd_r;
+extern int attach_pipe_fd;
+extern int dev_null_r;
+extern int dev_null_w;
 
 extern gboolean timed_out;