Merge pull request #2535 from brauner/2018-08-16/cmd_fixes

log: add logging tools for commands; lxc-usernsexec: cleanup and bugfixes

src/lxc/Makefile.am

@@ -325,7 +325,12 @@ lxc_user_nic_SOURCES = cmd/lxc_user_nic.c \
 		       namespace.c namespace.h \
 		       network.c network.h \
 		       parse.c parse.h
-lxc_usernsexec_SOURCES = cmd/lxc_usernsexec.c
+lxc_usernsexec_SOURCES = cmd/lxc_usernsexec.c \
+			 conf.c conf.h \
+			 list.h \
+			 log.c log.h \
+			 namespace.c namespace.h \
+			 utils.c utils.h
 endif
 
 

src/lxc/cmd/lxc_usernsexec.c

@@ -42,6 +42,8 @@
 #include <grp.h>
 
 #include "conf.h"
+#include "list.h"
+#include "log.h"
 #include "namespace.h"
 #include "utils.h"
 
@@ -77,21 +79,22 @@ static void usage(const char *name)
 
 static void opentty(const char *tty, int which)
 {
-	int fd, flags;
+	int fd, flags, ret;
 
 	if (tty[0] == '\0')
 		return;
 
 	fd = open(tty, O_RDWR | O_NONBLOCK);
 	if (fd < 0) {
-		printf("WARN: could not reopen tty: %s\n", strerror(errno));
+		CMD_SYSERROR("Failed to open tty");
 		return;
 	}
 
 	flags = fcntl(fd, F_GETFL);
 	flags &= ~O_NONBLOCK;
-	if (fcntl(fd, F_SETFL, flags) < 0) {
-		printf("WARN: could not set fd flags: %s\n", strerror(errno));
+	ret = fcntl(fd, F_SETFL, flags);
+	if (ret < 0) {
+		CMD_SYSINFO("Failed to remove O_NONBLOCK from file descriptor %d", fd);
 		close(fd);
 		return;
 	}
@@ -106,51 +109,61 @@ static void opentty(const char *tty, int which)
 
 static int do_child(void *vargv)
 {
+	int ret;
 	char **argv = (char **)vargv;
 
 	/* Assume we want to become root */
-	if (setgid(0) < 0) {
-		perror("setgid");
+	ret = setgid(0);
+	if (ret < 0) {
+		CMD_SYSERROR("Failed to set gid to");
 		return -1;
 	}
-	if (setuid(0) < 0) {
-		perror("setuid");
+
+	ret = setuid(0);
+	if (ret < 0) {
+		CMD_SYSERROR("Failed to set uid to 0");
 		return -1;
 	}
-	if (setgroups(0, NULL) < 0) {
-		perror("setgroups");
+
+	ret = setgroups(0, NULL);
+	if (ret < 0) {
+		CMD_SYSERROR("Failed to clear supplementary groups");
 		return -1;
 	}
-	if (unshare(CLONE_NEWNS) < 0) {
-		perror("unshare CLONE_NEWNS");
+
+	ret = unshare(CLONE_NEWNS);
+	if (ret < 0) {
+		CMD_SYSERROR("Failed to unshare mount namespace");
 		return -1;
 	}
+
 	if (detect_shared_rootfs()) {
-		if (mount(NULL, "/", NULL, MS_SLAVE|MS_REC, NULL)) {
-			printf("Failed to make / rslave\n");
+		ret = mount(NULL, "/", NULL, MS_SLAVE | MS_REC, NULL);
+		if (ret < 0) {
+			CMD_SYSINFO("Failed to make \"/\" rslave");
 			return -1;
 		}
 	}
+
 	execvp(argv[0], argv);
-	perror("execvpe");
+	CMD_SYSERROR("Failed to execute \"%s\"", argv[0]);
 	return -1;
 }
 
 static struct lxc_list active_map;
 
 /*
- * given a string like "b:0:100000:10", map both uids and gids
- * 0-10 to 100000 to 100010
+ * Given a string like "b:0:100000:10", map both uids and gids 0-10 to 100000
+ * to 100010
  */
 static int parse_map(char *map)
 {
-	struct id_map *newmap;
-	struct lxc_list *tmp = NULL;
-	int ret;
-	int i;
-	char types[2] = {'u', 'g'};
-	char which;
+	int i, ret;
 	long host_id, ns_id, range;
+	char which;
+	struct id_map *newmap;
+	char types[2] = {'u', 'g'};
+	struct lxc_list *tmp = NULL;
 
 	if (!map)
 		return -1;
@@ -193,45 +206,49 @@ static int parse_map(char *map)
 }
 
 /*
- * This is called if the user did not pass any uid ranges in
- * through -m flags.  It's called once to get the default uid
- * map, and once for the default gid map.
- * Go through /etc/subuids and /etc/subgids to find this user's
- * allowed map.  We only use the first one for each of uid and
- * gid, because otherwise we're not sure which entries the user
- * wanted.
+ * This is called if the user did not pass any uid ranges in through -m flags.
+ * It's called once to get the default uid map, and once for the default gid
+ * map.
+ * Go through /etc/subuids and /etc/subgids to find this user's allowed map. We
+ * only use the first one for each of uid and gid, because otherwise we're not
+ * sure which entries the user wanted.
  */
 static int read_default_map(char *fnam, int which, char *username)
 {
-	FILE *fin;
-	char *line = NULL;
-	size_t sz = 0;
-	struct id_map *newmap;
-	struct lxc_list *tmp = NULL;
 	char *p1, *p2;
+	FILE *fin;
+	struct id_map *newmap;
+	size_t sz = 0;
+	char *line = NULL;
+	struct lxc_list *tmp = NULL;
 
 	fin = fopen(fnam, "r");
 	if (!fin)
 		return -1;
+
 	while (getline(&line, &sz, fin) != -1) {
 		if (sz <= strlen(username) ||
 		    strncmp(line, username, strlen(username)) != 0 ||
 		    line[strlen(username)] != ':')
 			continue;
+
 		p1 = strchr(line, ':');
 		if (!p1)
 			continue;
-		p2 = strchr(p1+1, ':');
+
+		p2 = strchr(p1 + 1, ':');
 		if (!p2)
 			continue;
+
 		newmap = malloc(sizeof(*newmap));
-		if (!newmap)  {
+		if (!newmap) {
 			fclose(fin);
 			free(line);
 			return -1;
 		}
-		newmap->hostid = atol(p1+1);
-		newmap->range = atol(p2+1);
+
+		newmap->hostid = atol(p1 + 1);
+		newmap->range = atol(p2 + 1);
 		newmap->nsid = 0;
 		newmap->idtype = which;
 
@@ -250,16 +267,17 @@ static int read_default_map(char *fnam, int which, char *username)
 
 	free(line);
 	fclose(fin);
+
 	return 0;
 }
 
 static int find_default_map(void)
 {
-	struct passwd pwent;
-	struct passwd *pwentp = NULL;
-	char *buf;
 	size_t bufsize;
-	int ret;
+	char *buf;
+	struct passwd pwent;
+	int ret = -1;
+	struct passwd *pwentp = NULL;
 
 	bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
 	if (bufsize == -1)
@@ -272,60 +290,58 @@ static int find_default_map(void)
 	ret = getpwuid_r(getuid(), &pwent, buf, bufsize, &pwentp);
 	if (!pwentp) {
 		if (ret == 0)
-			printf("WARN: could not find matched password record\n");
+			CMD_SYSERROR("Failed to find matched password record");
 
-		printf("Failed to get password record - %u\n", getuid());
-		free(buf);
-		return -1;
+		CMD_SYSERROR("Failed to get password record for uid %d", getuid());
+		ret = -1;
+		goto out;
 	}
 
-	if (read_default_map(subuidfile, ID_TYPE_UID, pwent.pw_name) < 0) {
-		free(buf);
-		return -1;
-	}
+	ret = read_default_map(subuidfile, ID_TYPE_UID, pwent.pw_name);
+	if (ret < 0)
+		goto out;
 
-	if (read_default_map(subgidfile, ID_TYPE_GID, pwent.pw_name) < 0) {
-		free(buf);
-		return -1;
-	}
+	ret = read_default_map(subgidfile, ID_TYPE_GID, pwent.pw_name);
+	if (ret < 0)
+		goto out;
 
+	ret = 0;
+
+out:
 	free(buf);
-	return 0;
+
+	return ret;
 }
 
 int main(int argc, char *argv[])
 {
-	int c;
-	unsigned long flags = CLONE_NEWUSER | CLONE_NEWNS;
-	char ttyname0[256], ttyname1[256], ttyname2[256];
-	int status;
-	int ret;
-	int pid;
-	char *default_args[] = {"/bin/sh", NULL};
+	int c, pid, ret, status;
 	char buf[1];
-	int pipe_fds1[2],  /* child tells parent it has unshared */
-	    pipe_fds2[2];  /* parent tells child it is mapped and may proceed */
+	int pipe_fds1[2], /* child tells parent it has unshared */
+	    pipe_fds2[2]; /* parent tells child it is mapped and may proceed */
+	unsigned long flags = CLONE_NEWUSER | CLONE_NEWNS;
+	char ttyname0[256] = {0}, ttyname1[256] = {0}, ttyname2[256] = {0};
+	char *default_args[] = {"/bin/sh", NULL};
 
 	lxc_log_fd = STDERR_FILENO;
 
-	memset(ttyname0, '\0', sizeof(ttyname0));
-	memset(ttyname1, '\0', sizeof(ttyname1));
-	memset(ttyname2, '\0', sizeof(ttyname2));
-	if (isatty(0)) {
+	if (isatty(STDIN_FILENO)) {
 		ret = readlink("/proc/self/fd/0", ttyname0, sizeof(ttyname0));
 		if (ret < 0) {
-			perror("unable to open stdin.");
-			exit(EXIT_FAILURE);
+			CMD_SYSERROR("Failed to open stdin");
+			_exit(EXIT_FAILURE);
 		}
+
 		ret = readlink("/proc/self/fd/1", ttyname1, sizeof(ttyname1));
 		if (ret < 0) {
-			printf("Warning: unable to open stdout, continuing.\n");
-			memset(ttyname1, '\0', sizeof(ttyname1));
+			CMD_SYSINFO("Failed to open stdout. Continuing");
+			ttyname1[0] = '\0';
 		}
+
 		ret = readlink("/proc/self/fd/2", ttyname2, sizeof(ttyname2));
 		if (ret < 0) {
-			printf("Warning: unable to open stderr, continuing.\n");
-			memset(ttyname2, '\0', sizeof(ttyname2));
+			CMD_SYSINFO("Failed to open stderr. Continuing");
+			ttyname2[0] = '\0';
 		}
 	}
 
@@ -334,24 +350,26 @@ int main(int argc, char *argv[])
 	while ((c = getopt(argc, argv, "m:h")) != EOF) {
 		switch (c) {
 		case 'm':
-			if (parse_map(optarg)) {
+			ret = parse_map(optarg);
+			if (ret < 0) {
 				usage(argv[0]);
-				exit(EXIT_FAILURE);
+				_exit(EXIT_FAILURE);
 			}
 			break;
 		case 'h':
 			usage(argv[0]);
-			exit(EXIT_SUCCESS);
+			_exit(EXIT_SUCCESS);
 		default:
 			usage(argv[0]);
-			exit(EXIT_FAILURE);
+			_exit(EXIT_FAILURE);
 		}
 	};
 
 	if (lxc_list_empty(&active_map)) {
-		if (find_default_map()) {
-			fprintf(stderr, "You have no allocated subuids or subgids\n");
-			exit(EXIT_FAILURE);
+		ret = find_default_map();
+		if (ret < 0) {
+			fprintf(stderr, "Failed to find subuid or subgid allocation\n");
+			_exit(EXIT_FAILURE);
 		}
 	}
 
@@ -360,68 +378,103 @@ int main(int argc, char *argv[])
 	if (argc < 1)
 		argv = default_args;
 
-	if (pipe2(pipe_fds1, O_CLOEXEC) < 0 || pipe2(pipe_fds2, O_CLOEXEC) < 0) {
-		perror("pipe");
-		exit(EXIT_FAILURE);
+	ret = pipe2(pipe_fds1, O_CLOEXEC);
+	if (ret < 0) {
+		CMD_SYSERROR("Failed to open new pipe");
+		_exit(EXIT_FAILURE);
 	}
+
+	ret = pipe2(pipe_fds2, O_CLOEXEC);
+	if (ret < 0) {
+		CMD_SYSERROR("Failed to open new pipe");
+		close(pipe_fds1[0]);
+		close(pipe_fds1[1]);
+		_exit(EXIT_FAILURE);
+	}
+
 	pid = fork();
-	if (pid == 0) { /* Child. */
+	if (pid < 0) {
+		close(pipe_fds1[0]);
+		close(pipe_fds1[1]);
+		close(pipe_fds2[0]);
+		close(pipe_fds2[1]);
+		_exit(EXIT_FAILURE);
+	}
+
+	if (pid == 0) {
 		close(pipe_fds1[0]);
 		close(pipe_fds2[1]);
-		opentty(ttyname0, 0);
-		opentty(ttyname1, 1);
-		opentty(ttyname2, 2);
+
+		opentty(ttyname0, STDIN_FILENO);
+		opentty(ttyname1, STDOUT_FILENO);
+		opentty(ttyname2, STDERR_FILENO);
 
 		ret = unshare(flags);
 		if (ret < 0) {
-			fprintf(stderr,
-				"Failed to unshare mount and user namespace\n");
-			return 1;
+			CMD_SYSERROR("Failed to unshare mount and user namespace");
+			close(pipe_fds1[1]);
+			close(pipe_fds2[0]);
+			_exit(EXIT_FAILURE);
 		}
+
 		buf[0] = '1';
-		if (lxc_write_nointr(pipe_fds1[1], buf, 1) < 1) {
-			perror("write pipe");
-			exit(EXIT_FAILURE);
+		ret = lxc_write_nointr(pipe_fds1[1], buf, 1);
+		if (ret != 1) {
+			CMD_SYSERROR("Failed to write to pipe file descriptor %d",
+				     pipe_fds1[1]);
+			close(pipe_fds1[1]);
+			close(pipe_fds2[0]);
+			_exit(EXIT_FAILURE);
 		}
-		if (lxc_read_nointr(pipe_fds2[0], buf, 1) < 1) {
-			perror("read pipe");
-			exit(EXIT_FAILURE);
-		}
-		if (buf[0] != '1') {
-			fprintf(stderr, "parent had an error, child exiting\n");
-			exit(EXIT_FAILURE);
+
+		ret = lxc_read_nointr(pipe_fds2[0], buf, 1);
+		if (ret != 1) {
+			CMD_SYSERROR("Failed to read from pipe file descriptor %d",
+				     pipe_fds2[0]);
+			close(pipe_fds1[1]);
+			close(pipe_fds2[0]);
+			_exit(EXIT_FAILURE);
 		}
 
 		close(pipe_fds1[1]);
 		close(pipe_fds2[0]);
-		return do_child((void *)argv);
+
+		if (buf[0] != '1') {
+			fprintf(stderr, "Received unexpected value from parent process\n");
+			_exit(EXIT_FAILURE);
+		}
+
+		ret = do_child((void *)argv);
+		if (ret < 0)
+			_exit(EXIT_FAILURE);
+
+		_exit(EXIT_SUCCESS);
 	}
 
 	close(pipe_fds1[1]);
 	close(pipe_fds2[0]);
+
 	ret = lxc_read_nointr(pipe_fds1[0], buf, 1);
-	if (ret < 0) {
-		perror("read pipe");
-		exit(EXIT_FAILURE);
-	} else if (ret == 0) {
-		fprintf(stderr, "Failed to read from pipe\n");
-		exit(EXIT_FAILURE);
-	}
+	if (ret <= 0)
+		CMD_SYSERROR("Failed to read from pipe file descriptor %d", pipe_fds1[0]);
 
 	buf[0] = '1';
 
-	if (lxc_map_ids(&active_map, pid))
-		fprintf(stderr, "error mapping child\n");
+	ret = lxc_map_ids(&active_map, pid);
+	if (ret < 0)
+		fprintf(stderr, "Failed to write id mapping for child process\n");
 
-	if (lxc_write_nointr(pipe_fds2[1], buf, 1) < 0) {
-		perror("write to pipe");
-		exit(EXIT_FAILURE);
+	ret = lxc_write_nointr(pipe_fds2[1], buf, 1);
+	if (ret < 0) {
+		CMD_SYSERROR("Failed to write to pipe file descriptor %d", pipe_fds2[1]);
+		_exit(EXIT_FAILURE);
 	}
+
 	ret = waitpid(pid, &status, __WALL);
 	if (ret < 0) {
-		printf("waitpid() returns %d, errno %d\n", ret, errno);
-		exit(EXIT_FAILURE);
+		CMD_SYSERROR("Failed to wait on child process");
+		_exit(EXIT_FAILURE);
 	}
 
-	exit(WEXITSTATUS(status));
+	_exit(WEXITSTATUS(status));
 }

src/lxc/log.h

@@ -429,6 +429,18 @@ ATTR_UNUSED static inline void LXC_##LEVEL(struct lxc_log_locinfo* locinfo,	\
 		ERROR("%s - " format, ptr, ##__VA_ARGS__); \
 	} while (0)
 
+#define CMD_SYSERROR(format, ...)                                    \
+	do {                                                         \
+		lxc_log_strerror_r;                                  \
+		fprintf(stderr, "%s - " format, ptr, ##__VA_ARGS__); \
+	} while (0)
+
+#define CMD_SYSINFO(format, ...)                            \
+	do {                                                \
+		lxc_log_strerror_r;                         \
+		printf("%s - " format, ptr, ##__VA_ARGS__); \
+	} while (0)
+
 extern int lxc_log_fd;
 
 extern int lxc_log_syslog(int facility);