[lxc-devel] [PATCH 1/1] close-all-fds: fix behavior

[Serge Hallyn]

We want to close all inherited fds in three cases - one, if a container
is daemonized.  Two, if the user specifies -C on the lxc-start command
line.  Three, in src/lxc/monitor.c.  The presence of -C is passed in the
lxc_conf may not always exist.

One call to lxc_check_inherited was being done from lxc_start(), which
doesn't know whether we are daemonized.  Move that call to its caller,
lxcapi_start(), which does know.

Pass an explicit closeall boolean as second argument to lxc_check_inherited.
If it is true, then all fds are closed.  If it is false, then we check
the lxc_conf->close_all_fds.

With this, all tests pass, and the logic appears correct.

Note that when -C is not true, then we only warn about inherited fds,
but we do not abort the container start.  This appears to have ben the case
since commit 92c7f6295518 in 2011.  Unfortunately the referenced URL with
the justification is no longer valid.  We may want to consider becoming
stricter about this again.  (Note that the commit did say "for now")

Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
---
 src/lxc/execute.c      |  2 +-
 src/lxc/lxccontainer.c | 11 +++++++++--
 src/lxc/monitor.c      |  2 +-
 src/lxc/start.c        | 19 ++++++++++++++-----
 src/lxc/start.h        |  3 ++-
 5 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/src/lxc/execute.c b/src/lxc/execute.c
index b78bcbf..a0f7ff1 100644
--- a/src/lxc/execute.c
+++ b/src/lxc/execute.c
@@ -118,7 +118,7 @@ int lxc_execute(const char *name, char *const argv[], int quiet,
 		.quiet = quiet
 	};
 
-	if (lxc_check_inherited(conf, -1))
+	if (lxc_check_inherited(conf, false, -1))
 		return -1;
 
 	conf->is_execute = 1;
diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 0d36687..7ed8717 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -606,7 +606,6 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
 	* while container is running...
 	*/
 	if (daemonize) {
-		conf->close_all_fds = 1;
 		lxc_monitord_spawn(c->config_path);
 
 		pid_t pid = fork();
@@ -634,7 +633,7 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
 			SYSERROR("Error chdir()ing to /.");
 			return false;
 		}
-		lxc_check_inherited(conf, -1);
+		lxc_check_inherited(conf, true, -1);
 		close(0);
 		close(1);
 		close(2);
@@ -673,6 +672,13 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
 
 reboot:
 	conf->reboot = 0;
+
+	if (lxc_check_inherited(conf, daemonize, -1)) {
+		ERROR("Inherited fds found");
+		ret = 1;
+		goto out;
+	}
+
 	ret = lxc_start(c->name, argv, conf, c->config_path);
 	c->error_num = ret;
 
@@ -682,6 +688,7 @@ reboot:
 		goto reboot;
 	}
 
+out:
 	if (c->pidfile) {
 		unlink(c->pidfile);
 		free(c->pidfile);
diff --git a/src/lxc/monitor.c b/src/lxc/monitor.c
index f6d36a9..1e1c094 100644
--- a/src/lxc/monitor.c
+++ b/src/lxc/monitor.c
@@ -331,7 +331,7 @@ int lxc_monitord_spawn(const char *lxcpath)
 		SYSERROR("failed to setsid");
 		exit(EXIT_FAILURE);
 	}
-	lxc_check_inherited(NULL, pipefd[1]);
+	lxc_check_inherited(NULL, true, pipefd[1]);
 	close(0);
 	close(1);
 	close(2);
diff --git a/src/lxc/start.c b/src/lxc/start.c
index cd78665..f9bff51 100644
--- a/src/lxc/start.c
+++ b/src/lxc/start.c
@@ -170,12 +170,24 @@ static int match_fd(int fd)
 	return (fd == 0 || fd == 1 || fd == 2);
 }
 
-int lxc_check_inherited(struct lxc_conf *conf, int fd_to_ignore)
+/*
+ * Check for any fds we need to close
+ * * if fd_to_ignore != -1, then if we find that fd open we will ignore it.
+ * * By default we warn about open fds we find.
+ * * If closeall is true, we will close open fds.
+ * * If lxc-start was passed "-C", then conf->close_all_fds will be true,
+ *     in which case we also close all open fds.
+ * * A daemonized container will always pass closeall=true.
+ */
+int lxc_check_inherited(struct lxc_conf *conf, bool closeall, int fd_to_ignore)
 {
 	struct dirent dirent, *direntp;
 	int fd, fddir;
 	DIR *dir;
 
+	if (conf && conf->close_all_fds)
+		closeall = true;
+
 restart:
 	dir = opendir("/proc/self/fd");
 	if (!dir) {
@@ -203,7 +215,7 @@ restart:
 		if (match_fd(fd))
 			continue;
 
-		if (conf == NULL || conf->close_all_fds) {
+		if (closeall) {
 			close(fd);
 			closedir(dir);
 			INFO("closed inherited fd %d", fd);
@@ -1187,9 +1199,6 @@ int lxc_start(const char *name, char *const argv[], struct lxc_conf *conf,
 		.argv = argv,
 	};
 
-	if (lxc_check_inherited(conf, -1))
-		return -1;
-
 	conf->need_utmp_watch = 1;
 	return __lxc_start(name, conf, &start_ops, &start_arg, lxcpath);
 }
diff --git a/src/lxc/start.h b/src/lxc/start.h
index 7c75b16..d39b3b4 100644
--- a/src/lxc/start.h
+++ b/src/lxc/start.h
@@ -25,6 +25,7 @@
 
 #include <signal.h>
 #include <sys/param.h>
+#include <stdbool.h>
 
 #include "config.h"
 #include "state.h"
@@ -81,7 +82,7 @@ extern void lxc_abort(const char *name, struct lxc_handler *handler);
 extern struct lxc_handler *lxc_init(const char *name, struct lxc_conf *, const char *);
 extern void lxc_fini(const char *name, struct lxc_handler *handler);
 
-extern int lxc_check_inherited(struct lxc_conf *conf, int fd_to_ignore);
+extern int lxc_check_inherited(struct lxc_conf *conf, bool closeall, int fd_to_ignore);
 int __lxc_start(const char *, struct lxc_conf *, struct lxc_operations *,
 		void *, const char *);
 
-- 
2.1.3