[lxc-devel] [lxc/master] conf: rework and fix leak in userns_exec_1()

[brauner on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 364 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200327/bc5bb2ed/attachment.bin>
-------------- next part --------------
From b8885e74384f73ae52e44b5a8b77932ce8cc3a92 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Fri, 27 Mar 2020 09:37:48 +0100
Subject: [PATCH] conf: rework and fix leak in userns_exec_1()

Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/conf.c | 62 +++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index 8cd06abf90..b084cb88ee 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -3846,19 +3846,18 @@ struct userns_fn_data {
 
 static int run_userns_fn(void *data)
 {
+	struct userns_fn_data *d = data;
 	int ret;
 	char c;
-	struct userns_fn_data *d = data;
 
-	/* Close write end of the pipe. */
-	close(d->p[1]);
+	close_prot_errno_disarm(d->p[1]);
 
-	/* Wait for parent to finish establishing a new mapping in the user
+	/*
+	 * Wait for parent to finish establishing a new mapping in the user
 	 * namespace we are executing in.
 	 */
 	ret = lxc_read_nointr(d->p[0], &c, 1);
-	/* Close read end of the pipe. */
-	close(d->p[0]);
+	close_prot_errno_disarm(d->p[0]);
 	if (ret != 1)
 		return -1;
 
@@ -4033,7 +4032,8 @@ static struct lxc_list *get_minimal_idmap(const struct lxc_conf *conf)
 	return move_ptr(idmap);
 }
 
-/* Run a function in a new user namespace.
+/*
+ * Run a function in a new user namespace.
  * The caller's euid/egid will be mapped if it is not already.
  * Afaict, userns_exec_1() is only used to operate based on privileges for the
  * user's own {g,u}id on the host and for the container root's unmapped {g,u}id.
@@ -4047,30 +4047,29 @@ static struct lxc_list *get_minimal_idmap(const struct lxc_conf *conf)
 int userns_exec_1(const struct lxc_conf *conf, int (*fn)(void *), void *data,
 		  const char *fn_name)
 {
-	pid_t pid;
-	int p[2];
-	struct userns_fn_data d;
-	struct lxc_list *idmap;
+	__do_free struct lxc_list *idmap = NULL;
 	int ret = -1, status = -1;
 	char c = '1';
+	pid_t pid;
+	int pipe_fds[2];
+	struct userns_fn_data d;
 
 	if (!conf)
 		return -EINVAL;
 
 	idmap = get_minimal_idmap(conf);
 	if (!idmap)
-		return -1;
+		return ret_errno(ENOENT);
 
-	ret = pipe2(p, O_CLOEXEC);
-	if (ret < 0) {
-		SYSERROR("Failed to create pipe");
-		return -1;
-	}
-	d.fn = fn;
-	d.fn_name = fn_name;
-	d.arg = data;
-	d.p[0] = p[0];
-	d.p[1] = p[1];
+	ret = pipe2(pipe_fds, O_CLOEXEC);
+	if (ret < 0)
+		return -errno;
+
+	d.fn		= fn;
+	d.fn_name	= fn_name;
+	d.arg		= data;
+	d.p[0]		= pipe_fds[0];
+	d.p[1]		= pipe_fds[1];
 
 	/* Clone child in new user namespace. */
 	pid = lxc_raw_clone_cb(run_userns_fn, &d, CLONE_NEWUSER, NULL);
@@ -4079,21 +4078,17 @@ int userns_exec_1(const struct lxc_conf *conf, int (*fn)(void *), void *data,
 		goto on_error;
 	}
 
-	close(p[0]);
-	p[0] = -1;
+	close_prot_errno_disarm(pipe_fds[0]);
 
 	if (lxc_log_get_level() == LXC_LOG_LEVEL_TRACE ||
 	    conf->loglevel == LXC_LOG_LEVEL_TRACE) {
 		struct id_map *map;
 		struct lxc_list *it;
 
-		lxc_list_for_each (it, idmap) {
+		lxc_list_for_each(it, idmap) {
 			map = it->elem;
-			TRACE("Establishing %cid mapping for \"%d\" in new "
-			      "user namespace: nsuid %lu - hostid %lu - range "
-			      "%lu",
-			      (map->idtype == ID_TYPE_UID) ? 'u' : 'g', pid,
-			      map->nsid, map->hostid, map->range);
+			TRACE("Establishing %cid mapping for \"%d\" in new user namespace: nsuid %lu - hostid %lu - range %lu",
+			      (map->idtype == ID_TYPE_UID) ? 'u' : 'g', pid, map->nsid, map->hostid, map->range);
 		}
 	}
 
@@ -4105,15 +4100,14 @@ int userns_exec_1(const struct lxc_conf *conf, int (*fn)(void *), void *data,
 	}
 
 	/* Tell child to proceed. */
-	if (lxc_write_nointr(p[1], &c, 1) != 1) {
+	if (lxc_write_nointr(pipe_fds[1], &c, 1) != 1) {
 		SYSERROR("Failed telling child process \"%d\" to proceed", pid);
 		goto on_error;
 	}
 
 on_error:
-	if (p[0] != -1)
-		close(p[0]);
-	close(p[1]);
+	close_prot_errno_disarm(pipe_fds[0]);
+	close_prot_errno_disarm(pipe_fds[1]);
 
 	/* Wait for child to finish. */
 	if (pid > 0)