Merge pull request #3281 from brauner/2020-03-10/fixes

src/lxc/cgroups/cgfsng.c

@@ -54,19 +54,6 @@
 
 lxc_log_define(cgfsng, cgroup);
 
-static void free_string_list(char **clist)
-{
-	int i;
-
-	if (!clist)
-		return;
-
-	for (i = 0; clist[i]; i++)
-		free(clist[i]);
-
-	free(clist);
-}
-
 /* Given a pointer to a null-terminated array of pointers, realloc to add one
  * entry, and point the new entry to NULL. Do not fail. Return the index to the
  * second-to-last entry - that is, the one which is now available for use
@@ -2940,12 +2927,11 @@ static void cg_unified_delegate(char ***delegate)
  */
 static int cg_hybrid_init(struct cgroup_ops *ops, bool relative, bool unprivileged)
 {
-	__do_free char *basecginfo = NULL;
-	__do_free char *line = NULL;
+	__do_free char *basecginfo = NULL, *line = NULL;
+	__do_free_string_list char **klist = NULL, **nlist = NULL;
 	__do_fclose FILE *f = NULL;
 	int ret;
 	size_t len = 0;
-	char **klist = NULL, **nlist = NULL;
 
 	/* Root spawned containers escape the current cgroup, so use init's
 	 * cgroups as our base in that case.
@@ -2968,11 +2954,11 @@ static int cg_hybrid_init(struct cgroup_ops *ops, bool relative, bool unprivileg
 	lxc_cgfsng_print_basecg_debuginfo(basecginfo, klist, nlist);
 
 	while (getline(&line, &len, f) != -1) {
+		__do_free char *base_cgroup = NULL, *mountpoint = NULL;
+		__do_free_string_list char **controller_list = NULL;
 		int type;
 		bool writeable;
 		struct hierarchy *new;
-		char *base_cgroup = NULL, *mountpoint = NULL;
-		char **controller_list = NULL;
 
 		type = get_cgroup_version(line);
 		if (type == 0)
@@ -3000,18 +2986,18 @@ static int cg_hybrid_init(struct cgroup_ops *ops, bool relative, bool unprivileg
 
 		if (type == CGROUP_SUPER_MAGIC)
 			if (controller_list_is_dup(ops->hierarchies, controller_list))
-				log_trace_errno(goto next, EEXIST, "Skipping duplicating controller");
+				log_trace_errno(continue, EEXIST, "Skipping duplicating controller");
 
 		mountpoint = cg_hybrid_get_mountpoint(line);
 		if (!mountpoint)
-			log_error_errno(goto next, EINVAL, "Failed parsing mountpoint from \"%s\"", line);
+			log_error_errno(continue, EINVAL, "Failed parsing mountpoint from \"%s\"", line);
 
 		if (type == CGROUP_SUPER_MAGIC)
 			base_cgroup = cg_hybrid_get_current_cgroup(basecginfo, controller_list[0], CGROUP_SUPER_MAGIC);
 		else
 			base_cgroup = cg_hybrid_get_current_cgroup(basecginfo, NULL, CGROUP2_SUPER_MAGIC);
 		if (!base_cgroup)
-			log_error_errno(goto next, EINVAL, "Failed to find current cgroup");
+			log_error_errno(continue, EINVAL, "Failed to find current cgroup");
 
 		trim(base_cgroup);
 		prune_init_scope(base_cgroup);
@@ -3020,7 +3006,7 @@ static int cg_hybrid_init(struct cgroup_ops *ops, bool relative, bool unprivileg
 		else
 			writeable = test_writeable_v1(mountpoint, base_cgroup);
 		if (!writeable)
-			log_trace_errno(goto next, EROFS, "The %s group is not writeable", base_cgroup);
+			log_trace_errno(continue, EROFS, "The %s group is not writeable", base_cgroup);
 
 		if (type == CGROUP2_SUPER_MAGIC) {
 			char *cgv2_ctrl_path;
@@ -3040,26 +3026,16 @@ static int cg_hybrid_init(struct cgroup_ops *ops, bool relative, bool unprivileg
 
 		/* Exclude all controllers that cgroup use does not want. */
 		if (!cgroup_use_wants_controllers(ops, controller_list))
-			log_trace_errno(goto next, EINVAL, "Skipping controller");
+			log_trace_errno(continue, EINVAL, "Skipping controller");
 
-		new = add_hierarchy(&ops->hierarchies, controller_list, mountpoint, base_cgroup, type);
+		new = add_hierarchy(&ops->hierarchies, move_ptr(controller_list), move_ptr(mountpoint), move_ptr(base_cgroup), type);
 		if (type == CGROUP2_SUPER_MAGIC && !ops->unified) {
 			if (unprivileged)
 				cg_unified_delegate(&new->cgroup2_chown);
 			ops->unified = new;
 		}
-
-		continue;
-
-	next:
-		free_string_list(controller_list);
-		free(mountpoint);
-		free(base_cgroup);
 	}
 
-	free_string_list(klist);
-	free_string_list(nlist);
-
 	TRACE("Writable cgroup hierarchies:");
 	lxc_cgfsng_print_hierarchies(ops);
 

src/lxc/commands_utils.c

@@ -39,26 +39,18 @@ int lxc_cmd_sock_rcv_state(int state_client_fd, int timeout)
 		out.tv_sec = timeout;
 		ret = setsockopt(state_client_fd, SOL_SOCKET, SO_RCVTIMEO,
 				(const void *)&out, sizeof(out));
-		if (ret < 0) {
-			SYSERROR("Failed to set %ds timeout on container "
-				 "state socket",
-				 timeout);
-			return -1;
-		}
+		if (ret < 0)
+			return log_error_errno(-1, errno, "Failed to set %ds timeout on container state socket", timeout);
 	}
 
 	memset(&msg, 0, sizeof(msg));
 
 	ret = lxc_recv_nointr(state_client_fd, &msg, sizeof(msg), 0);
-	if (ret < 0) {
-		SYSERROR("Failed to receive message");
-		return -1;
-	}
+	if (ret < 0)
+		return log_error_errno(-1, errno, "Failed to receive message");
 
-	TRACE("Received state %s from state client %d",
+	return log_trace(msg.value, "Received state %s from state client %d",
 			 lxc_state2str(msg.value), state_client_fd);
-
-	return msg.value;
 }
 
 /* Register a new state client and retrieve state from command socket. */
@@ -110,26 +102,20 @@ int lxc_make_abstract_socket_name(char *path, size_t pathlen,
 
 	if (hashed_sock_name != NULL) {
 		ret = snprintf(offset, len, "lxc/%s/%s", hashed_sock_name, suffix);
-		if (ret < 0 || ret >= len) {
-			ERROR("Failed to create abstract socket name");
-			return -1;
-		}
+		if (ret < 0 || (size_t)ret >= len)
+			return log_error_errno(-1, errno, "Failed to create abstract socket name");
 		return 0;
 	}
 
 	if (!lxcpath) {
 		lxcpath = lxc_global_config_value("lxc.lxcpath");
-		if (!lxcpath) {
-			ERROR("Failed to allocate memory");
-			return -1;
-		}
+		if (!lxcpath)
+			return log_error(-1, "Failed to allocate memory");
 	}
 
 	ret = snprintf(offset, len, "%s/%s/%s", lxcpath, name, suffix);
-	if (ret < 0) {
-		ERROR("Failed to create abstract socket name");
-		return -1;
-	}
+	if (ret < 0 || (size_t)ret >= len)
+		return log_error_errno(-1, errno, "Failed to create abstract socket name");
 	if (ret < len)
 		return 0;
 
@@ -137,17 +123,13 @@ int lxc_make_abstract_socket_name(char *path, size_t pathlen,
 	tmplen = strlen(name) + strlen(lxcpath) + 2;
 	tmppath = must_realloc(NULL, tmplen);
 	ret = snprintf(tmppath, tmplen, "%s/%s", lxcpath, name);
-	if (ret < 0 || (size_t)ret >= tmplen) {
-		ERROR("Failed to create abstract socket name");
-		return -1;
-	}
+	if (ret < 0 || (size_t)ret >= tmplen)
+		return log_error_errno(-1, errno, "Failed to create abstract socket name");
 
 	hash = fnv_64a_buf(tmppath, ret, FNV1A_64_INIT);
 	ret = snprintf(offset, len, "lxc/%016" PRIx64 "/%s", hash, suffix);
-	if (ret < 0 || ret >= len) {
-		ERROR("Failed to create abstract socket name");
-		return -1;
-	}
+	if (ret < 0 || ret >= len)
+		return log_error_errno(-1, errno, "Failed to create abstract socket name");
 
 	return 0;
 }
@@ -198,8 +180,7 @@ int lxc_add_state_client(int state_client_fd, struct lxc_handler *handler,
 		return state;
 	}
 
-	TRACE("Added state client %d to state client list", state_client_fd);
 	move_ptr(newclient);
 	move_ptr(tmplist);
-	return MAX_STATE;
+	return log_trace(MAX_STATE, "Added state client %d to state client list", state_client_fd);
 }

src/lxc/memory_utils.h

@@ -5,6 +5,7 @@
 
 #include <dirent.h>
 #include <errno.h>
+#include <mntent.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/types.h>
@@ -12,11 +13,35 @@
 
 #include "macro.h"
 
+#define define_cleanup_attribute(type, func)     \
+	static inline void func##_ptr(type *ptr) \
+	{                                        \
+		if (*ptr)                        \
+			func(*ptr);              \
+	}
+
+#define free_disarm(ptr)       \
+	({                     \
+		free(ptr);     \
+		move_ptr(ptr); \
+	})
+
 static inline void __auto_free__(void *p)
 {
 	free(*(void **)p);
 }
 
+static inline void free_string_list(char **list)
+{
+	if (list) {
+		for (int i = 0; list[i]; i++)
+			free(list[i]);
+		free_disarm(list);
+	}
+}
+define_cleanup_attribute(char **, free_string_list);
+#define __do_free_string_list __attribute__((__cleanup__(free_string_list_ptr)))
+
 static inline void __auto_fclose__(FILE **f)
 {
 	if (*f)
@@ -37,12 +62,6 @@ static inline void __auto_closedir__(DIR **d)
 		fd = -EBADF;        \
 	}
 
-#define free_disarm(ptr)       \
-	({                     \
-		free(ptr);     \
-		move_ptr(ptr); \
-	})
-
 static inline void __auto_close__(int *fd)
 {
 	close_prot_errno_disarm(*fd);

src/lxc/pam/pam_cgfs.c

@@ -77,7 +77,6 @@ static inline void clear_bit(unsigned bit, uint32_t *bitarr)
 	bitarr[bit / NBITS] &= ~(1 << (bit % NBITS));
 }
 static char *copy_to_eol(char *s);
-static void free_string_list(char **list);
 static char *get_mountpoint(char *line);
 static bool get_uid_gid(const char *user, uid_t *uid, gid_t *gid);
 static int handle_login(const char *user, uid_t uid, gid_t gid);
@@ -454,16 +453,6 @@ static size_t string_list_length(char **list)
 	return len;
 }
 
-/* Free null-terminated array of strings. */
-static void free_string_list(char **list)
-{
-	char **it;
-
-	for (it = list; it && *it; it++)
-		free(*it);
-	free(list);
-}
-
 /* Write single integer to file. */
 static bool write_int(char *path, int v)
 {

src/lxc/utils.h

@@ -73,6 +73,13 @@ extern struct lxc_popen_FILE *lxc_popen(const char *command);
  */
 extern int lxc_pclose(struct lxc_popen_FILE *fp);
 
+static inline void __auto_lxc_pclose__(struct lxc_popen_FILE **f)
+{
+	if (*f)
+		lxc_pclose(*f);
+}
+#define __do_lxc_pclose __attribute__((__cleanup__(__auto_lxc_pclose__)))
+
 /*
  * wait on a child we forked
  */

src/tests/share_ns.c

@@ -35,8 +35,8 @@ struct thread_args {
 	int thread_id;
 	bool success;
 	pid_t init_pid;
-	char *inherited_ipc_ns;
-	char *inherited_net_ns;
+	char inherited_ipc_ns[4096];
+	char inherited_net_ns[4096];
 };
 
 void *ns_sharing_wrapper(void *data)
@@ -45,8 +45,8 @@ void *ns_sharing_wrapper(void *data)
 	ssize_t ret;
 	char name[100];
 	char owning_ns_init_pid[100];
-	char proc_ns_path[4096];
-	char ns_buf[4096];
+	char proc_ns_path[256];
+	char ns_buf[256];
 	struct lxc_container *c;
 	struct thread_args *args = data;
 
@@ -162,15 +162,11 @@ void *ns_sharing_wrapper(void *data)
 	args->success = true;
 
 out:
-	if (c->is_running(c) && !c->stop(c)) {
+	if (c->is_running(c) && !c->stop(c))
 		lxc_error("Failed to stop container \"%s\"\n", name);
-		goto out;
-	}
 
-	if (!c->destroy(c)) {
+	if (!c->destroy(c))
 		lxc_error("Failed to destroy container \"%s\"\n", name);
-		goto out;
-	}
 
 	pthread_exit(NULL);
 	return NULL;
@@ -178,16 +174,19 @@ out:
 
 int main(int argc, char *argv[])
 {
+	struct thread_args *args = NULL;
+	size_t nthreads = 10;
 	int i, init_pid, j;
 	char proc_ns_path[4096];
 	char ipc_ns_buf[4096];
 	char net_ns_buf[4096];
 	pthread_attr_t attr;
 	pthread_t threads[10];
-	struct thread_args args[10];
 	struct lxc_container *c;
 	int ret = EXIT_FAILURE;
 
+	pthread_attr_init(&attr);
+
 	c = lxc_container_new("owning-ns", NULL);
 	if (!c) {
 		lxc_error("%s", "Failed to create container \"owning-ns\"");
@@ -263,24 +262,28 @@ int main(int argc, char *argv[])
 
 	sleep(5);
 
-	pthread_attr_init(&attr);
+	args = malloc(sizeof(struct thread_args) * nthreads);
+	if (!args) {
+		lxc_error("%s\n", "Failed to allocate memory");
+		goto on_error_stop;
+	}
 
 	for (j = 0; j < 10; j++) {
 		lxc_debug("Starting namespace sharing test iteration %d\n", j);
 
-		for (i = 0; i < 10; i++) {
+		for (i = 0; i < nthreads; i++) {
 			args[i].thread_id = i;
 			args[i].success = false;
 			args[i].init_pid = init_pid;
-			args[i].inherited_ipc_ns = ipc_ns_buf;
-			args[i].inherited_net_ns = net_ns_buf;
+			memcpy(args[i].inherited_ipc_ns, ipc_ns_buf, sizeof(args[i].inherited_ipc_ns));
+			memcpy(args[i].inherited_net_ns, net_ns_buf, sizeof(args[i].inherited_net_ns));
 
-			ret = pthread_create(&threads[i], &attr, ns_sharing_wrapper, (void *) &args[i]);
+			ret = pthread_create(&threads[i], &attr, ns_sharing_wrapper, (void *)&args[i]);
 			if (ret != 0)
 				goto on_error_stop;
 		}
 
-		for (i = 0; i < 10; i++) {
+		for (i = 0; i < nthreads; i++) {
 			ret = pthread_join(threads[i], NULL);
 			if (ret != 0)
 				goto on_error_stop;
@@ -295,6 +298,9 @@ int main(int argc, char *argv[])
 	ret = EXIT_SUCCESS;
 
 on_error_stop:
+	free(args);
+	pthread_attr_destroy(&attr);
+
 	if (c->is_running(c) && !c->stop(c))
 		lxc_error("%s\n", "Failed to stop container \"owning-ns\"");
 
@@ -305,5 +311,6 @@ on_error_put:
 	lxc_container_put(c);
 	if (ret == EXIT_SUCCESS)
 		lxc_debug("%s\n", "All state namespace sharing tests passed");
+
 	exit(ret);
 }