Merge pull request #3313 from brauner/2020-03-20/fixes

src/lxc/cmd/lxc_user_nic.c

@@ -53,6 +53,14 @@
 
 #define usernic_error(format, ...) usernic_debug_stream(stderr, format, __VA_ARGS__)
 
+#define cmd_error_errno(__ret__, __errno__, format, ...)      \
+	({                                                    \
+		typeof(__ret__) __internal_ret__ = (__ret__); \
+		errno = (__errno__);                          \
+		CMD_SYSERROR(format, ##__VA_ARGS__);          \
+		__internal_ret__;                             \
+	})
+
 __noreturn static void usage(bool fail)
 {
 	fprintf(stderr, "Description:\n");
@@ -493,19 +501,19 @@ static int instantiate_veth(char *veth1, char *veth2, pid_t pid, unsigned int mt
 
 	ret = lxc_veth_create(veth1, veth2, pid, mtu);
 	if (ret < 0) {
-		errno = -ret;
 		CMD_SYSERROR("Failed to create %s-%s\n", veth1, veth2);
-		return -1;
+		return ret_errno(-ret);
 	}
 
-	/* Changing the high byte of the mac address to 0xfe, the bridge
+	/*
+	 * Changing the high byte of the mac address to 0xfe, the bridge
 	 * interface will always keep the host's mac address and not take the
 	 * mac address of a container.
 	 */
 	ret = setup_private_host_hw_addr(veth1);
 	if (ret < 0) {
-		errno = -ret;
 		CMD_SYSERROR("Failed to change mac address of host interface %s\n", veth1);
+		return ret_errno(-ret);
 	}
 
 	return netdev_set_flag(veth1, IFF_UP);
@@ -769,13 +777,25 @@ static char *get_nic_if_avail(int fd, struct alloted_s *names, int pid,
 		return NULL;
 	}
 
-	if (lxc_pwrite_nointr(fd, newline, slen, length) != slen)
+	if (lxc_pwrite_nointr(fd, newline, slen, length) != slen) {
 		CMD_SYSERROR("Failed to append new entry \"%s\" to database file", newline);
 
+		if (lxc_netdev_delete_by_name(nicname) != 0)
+			usernic_error("Error unlinking %s\n", nicname);
+
+		return NULL;
+	}
+
 	ret = ftruncate(fd, length + slen);
-	if (ret < 0)
+	if (ret < 0) {
 		CMD_SYSERROR("Failed to truncate file\n");
 
+		if (lxc_netdev_delete_by_name(nicname) != 0)
+			usernic_error("Error unlinking %s\n", nicname);
+
+		return NULL;
+	}
+
 	return strdup(nicname);
 }
 
@@ -814,113 +834,84 @@ again:
 static char *lxc_secure_rename_in_ns(int pid, char *oldname, char *newname,
 				     int *container_veth_ifidx)
 {
-	int ofd, ret;
+	__do_close int fd = -EBADF, ofd = -EBADF;
+	int fret = -1;
+	int ifindex, ret;
 	pid_t pid_self;
 	uid_t ruid, suid, euid;
 	char ifname[IFNAMSIZ];
-	char *string_ret = NULL, *name = NULL;
-	int fd = -1, ifindex = -1;
 
 	pid_self = lxc_raw_getpid();
 
 	ofd = lxc_preserve_ns(pid_self, "net");
-	if (ofd < 0) {
-		usernic_error("Failed opening network namespace path for %d", pid_self);
-		return NULL;
-	}
+	if (ofd < 0)
+		return cmd_error_errno(NULL, errno, "Failed opening network namespace path for %d", pid_self);
 
 	fd = lxc_preserve_ns(pid, "net");
-	if (fd < 0) {
-		usernic_error("Failed opening network namespace path for %d", pid);
-		goto do_partial_cleanup;
-	}
+	if (fd < 0)
+		return cmd_error_errno(NULL, errno, "Failed opening network namespace path for %d", pid);
 
 	ret = getresuid(&ruid, &euid, &suid);
-	if (ret < 0) {
-		CMD_SYSERROR("Failed to retrieve real, effective, and saved user IDs\n");
-		goto do_partial_cleanup;
-	}
+	if (ret < 0)
+		return cmd_error_errno(NULL, errno, "Failed to retrieve real, effective, and saved user IDs\n");
 
 	ret = setns(fd, CLONE_NEWNET);
-	close(fd);
-	fd = -1;
-	if (ret < 0) {
-		CMD_SYSERROR("Failed to setns() to the network namespace of the container with PID %d\n",
-			     pid);
-		goto do_partial_cleanup;
-	}
+	if (ret < 0)
+		return cmd_error_errno(NULL, errno, "Failed to setns() to the network namespace of the container with PID %d\n", pid);
 
 	ret = setresuid(ruid, ruid, 0);
 	if (ret < 0) {
-		CMD_SYSERROR("Failed to drop privilege by setting effective user id and real user id to %d, and saved user ID to 0\n",
-			     ruid);
-		/* It's ok to jump to do_full_cleanup here since setresuid()
-		 * will succeed when trying to set real, effective, and saved to
-		 * values they currently have.
+		CMD_SYSERROR("Failed to drop privilege by setting effective user id and real user id to %d, and saved user ID to 0\n", ruid);
+		/*
+		 * It's ok to jump to do_full_cleanup here since setresuid()
+		 * will succeed when trying to set real, effective, and saved
+		 * to values they currently have.
 		 */
-		goto do_full_cleanup;
+		goto out_setns;
 	}
 
 	/* Check if old interface exists. */
 	ifindex = if_nametoindex(oldname);
 	if (!ifindex) {
 		CMD_SYSERROR("Failed to get netdev index\n");
-		goto do_full_cleanup;
+		goto out_setresuid;
 	}
 
-	/* When the IFLA_IFNAME attribute is passed something like "<prefix>%d"
+	/*
+	 * When the IFLA_IFNAME attribute is passed something like "<prefix>%d"
 	 * netlink will replace the format specifier with an appropriate index.
 	 * So we pass "eth%d".
 	 */
-	if (newname)
-		name = newname;
-	else
-		name = "eth%d";
-
-	ret = lxc_netdev_rename_by_name(oldname, name);
-	name = NULL;
+	ret = lxc_netdev_rename_by_name(oldname, newname ? newname : "eth%d");
 	if (ret < 0) {
-		usernic_error("Error %d renaming netdev %s to %s in container\n",
-		              ret, oldname, newname ? newname : "eth%d");
-		goto do_full_cleanup;
+		CMD_SYSERROR("Error %d renaming netdev %s to %s in container\n", ret, oldname, newname ? newname : "eth%d");
+		goto out_setresuid;
 	}
 
 	/* Retrieve new name for interface. */
 	if (!if_indextoname(ifindex, ifname)) {
 		CMD_SYSERROR("Failed to get new netdev name\n");
-		goto do_full_cleanup;
+		goto out_setresuid;
 	}
 
-	/* Allocation failure for strdup() is checked below. */
-	name = strdup(ifname);
-	string_ret = name;
-	*container_veth_ifidx = ifindex;
+	fret = 0;
 
-do_full_cleanup:
+out_setresuid:
 	ret = setresuid(ruid, euid, suid);
-	if (ret < 0) {
-		CMD_SYSERROR("Failed to restore privilege by setting effective user id to %d, real user id to %d, and saved user ID to %d\n",
-			     ruid, euid, suid);
-
-		string_ret = NULL;
-	}
+	if (ret < 0)
+		return cmd_error_errno(NULL, errno, "Failed to restore privilege by setting effective user id to %d, real user id to %d, and saved user ID to %d\n",
+				       ruid, euid, suid);
 
+out_setns:
 	ret = setns(ofd, CLONE_NEWNET);
-	if (ret < 0) {
-		CMD_SYSERROR("Failed to setns() to original network namespace of PID %d\n", ofd);
-		string_ret = NULL;
-	}
-
-do_partial_cleanup:
-	if (fd >= 0)
-		close(fd);
-
-	if (!string_ret && name)
-		free(name);
+	if (ret < 0)
+		return cmd_error_errno(NULL, errno, "Failed to setns() to original network namespace of PID %d\n", ofd);
 
-	close(ofd);
+	if (fret < 0)
+		return NULL;
 
-	return string_ret;
+	*container_veth_ifidx = ifindex;
+	return strdup(ifname);
 }
 
 /* If the caller (real uid, not effective uid) may read the /proc/[pid]/ns/net,

src/lxc/log.h

@@ -456,13 +456,15 @@ __lxc_unused static inline void LXC_##LEVEL(struct lxc_log_locinfo* locinfo,	\
 #endif
 
 #if HAVE_M_FORMAT
-#define CMD_SYSERROR(format, ...)                                    \
-		fprintf(stderr, "%m - " format, ##__VA_ARGS__)
+#define CMD_SYSERROR(format, ...)                                            \
+	fprintf(stderr, "%m - %s: %d: %s: " format "\n", __FILE__, __LINE__, \
+		__func__, ##__VA_ARGS__);
 #else
-#define CMD_SYSERROR(format, ...)                                    \
-	do {                                                         \
-		lxc_log_strerror_r;                                  \
-		fprintf(stderr, "%s - " format, ptr, ##__VA_ARGS__); \
+#define CMD_SYSERROR(format, ...)                                          \
+	do {                                                               \
+		lxc_log_strerror_r;                                        \
+		fprintf(stderr, "%s - %s: %d: %s: " format "\n", __FILE__, \
+			__LINE__, __func__, ##__VA_ARGS__);
 	} while (0)
 #endif
 

src/lxc/nl.c

@@ -19,16 +19,19 @@
 
 lxc_log_define(nl, lxc);
 
-extern size_t nlmsg_len(const struct nlmsg *nlmsg)
+size_t nlmsg_len(const struct nlmsg *nlmsg)
 {
 	return nlmsg->nlmsghdr->nlmsg_len - NLMSG_HDRLEN;
 }
 
-extern void *nlmsg_data(struct nlmsg *nlmsg)
+void *nlmsg_data(struct nlmsg *nlmsg)
 {
-	char *data = ((char *)nlmsg) + NLMSG_HDRLEN;
+	char *data;
+
+	data = ((char *)nlmsg) + NLMSG_HDRLEN;
 	if (!nlmsg_len(nlmsg))
-		return NULL;
+		return ret_set_errno(NULL, EINVAL);
+
 	return data;
 }
 
@@ -40,7 +43,7 @@ static int nla_put(struct nlmsg *nlmsg, int attr,
 	size_t tlen = NLMSG_ALIGN(nlmsg->nlmsghdr->nlmsg_len) + RTA_ALIGN(rtalen);
 
 	if (tlen > nlmsg->cap)
-		return -ENOMEM;
+		return ret_errno(ENOMEM);
 
 	rta = NLMSG_TAIL(nlmsg->nlmsghdr);
 	rta->rta_type = attr;
@@ -48,41 +51,42 @@ static int nla_put(struct nlmsg *nlmsg, int attr,
 	if (data && len)
 		memcpy(RTA_DATA(rta), data, len);
 	nlmsg->nlmsghdr->nlmsg_len = tlen;
+
 	return 0;
 }
 
-extern int nla_put_buffer(struct nlmsg *nlmsg, int attr,
-			  const void *data, size_t size)
+int nla_put_buffer(struct nlmsg *nlmsg, int attr, const void *data, size_t size)
 {
 	return nla_put(nlmsg, attr, data, size);
 }
 
-extern int nla_put_string(struct nlmsg *nlmsg, int attr, const char *string)
+int nla_put_string(struct nlmsg *nlmsg, int attr, const char *string)
 {
 	return nla_put(nlmsg, attr, string, strlen(string) + 1);
 }
 
-extern int nla_put_u32(struct nlmsg *nlmsg, int attr, int value)
+int nla_put_u32(struct nlmsg *nlmsg, int attr, int value)
 {
 	return nla_put(nlmsg, attr, &value, sizeof(value));
 }
 
-extern int nla_put_u16(struct nlmsg *nlmsg, int attr, unsigned short value)
+int nla_put_u16(struct nlmsg *nlmsg, int attr, unsigned short value)
 {
 	return nla_put(nlmsg, attr, &value, 2);
 }
 
-extern int nla_put_attr(struct nlmsg *nlmsg, int attr)
+int nla_put_attr(struct nlmsg *nlmsg, int attr)
 {
 	return nla_put(nlmsg, attr, NULL, 0);
 }
 
 struct rtattr *nla_begin_nested(struct nlmsg *nlmsg, int attr)
 {
-	struct rtattr *rtattr = NLMSG_TAIL(nlmsg->nlmsghdr);
+	struct rtattr *rtattr;
 
+	rtattr = NLMSG_TAIL(nlmsg->nlmsghdr);
 	if (nla_put_attr(nlmsg, attr))
-		return NULL;
+		return ret_set_errno(NULL, ENOMEM);
 
 	return rtattr;
 }
@@ -92,37 +96,34 @@ void nla_end_nested(struct nlmsg *nlmsg, struct rtattr *attr)
 	attr->rta_len = (void *)NLMSG_TAIL(nlmsg->nlmsghdr) - (void *)attr;
 }
 
-extern struct nlmsg *nlmsg_alloc(size_t size)
+struct nlmsg *nlmsg_alloc(size_t size)
 {
-	struct nlmsg *nlmsg;
+	__do_free struct nlmsg *nlmsg = NULL;
 	size_t len = NLMSG_HDRLEN + NLMSG_ALIGN(size);
 
-	nlmsg = (struct nlmsg *)malloc(sizeof(struct nlmsg));
+	nlmsg = malloc(sizeof(struct nlmsg));
 	if (!nlmsg)
-		return NULL;
+		return ret_set_errno(NULL, ENOMEM);
 
-	nlmsg->nlmsghdr = (struct nlmsghdr *)malloc(len);
+	nlmsg->nlmsghdr = malloc(len);
 	if (!nlmsg->nlmsghdr)
-		goto errout;
+		return ret_set_errno(NULL, ENOMEM);
 
 	memset(nlmsg->nlmsghdr, 0, len);
 	nlmsg->cap = len;
 	nlmsg->nlmsghdr->nlmsg_len = NLMSG_HDRLEN;
 
-	return nlmsg;
-errout:
-	free(nlmsg);
-	return NULL;
+	return move_ptr(nlmsg);
 }
 
-extern void *nlmsg_reserve(struct nlmsg *nlmsg, size_t len)
+void *nlmsg_reserve(struct nlmsg *nlmsg, size_t len)
 {
 	void *buf;
 	size_t nlmsg_len = nlmsg->nlmsghdr->nlmsg_len;
 	size_t tlen = NLMSG_ALIGN(len);
 
 	if (nlmsg_len + tlen > nlmsg->cap)
-		return NULL;
+		return ret_set_errno(NULL, ENOMEM);
 
 	buf = ((char *)(nlmsg->nlmsghdr)) + nlmsg_len;
 	nlmsg->nlmsghdr->nlmsg_len += tlen;
@@ -133,29 +134,28 @@ extern void *nlmsg_reserve(struct nlmsg *nlmsg, size_t len)
 	return buf;
 }
 
-extern struct nlmsg *nlmsg_alloc_reserve(size_t size)
+struct nlmsg *nlmsg_alloc_reserve(size_t size)
 {
 	struct nlmsg *nlmsg;
 
 	nlmsg = nlmsg_alloc(size);
 	if (!nlmsg)
-		return NULL;
+		return ret_set_errno(NULL, ENOMEM);
 
 	/* Just set message length to cap directly. */
 	nlmsg->nlmsghdr->nlmsg_len = nlmsg->cap;
 	return nlmsg;
 }
 
-extern void nlmsg_free(struct nlmsg *nlmsg)
+void nlmsg_free(struct nlmsg *nlmsg)
 {
-	if (!nlmsg)
-		return;
-
-	free(nlmsg->nlmsghdr);
-	free(nlmsg);
+	if (nlmsg) {
+		free(nlmsg->nlmsghdr);
+		free(nlmsg);
+	}
 }
 
-extern int __netlink_recv(struct nl_handler *handler, struct nlmsghdr *nlmsghdr)
+int __netlink_recv(struct nl_handler *handler, struct nlmsghdr *nlmsghdr)
 {
 	int ret;
 	struct sockaddr_nl nladdr;
@@ -182,26 +182,24 @@ again:
 		if (errno == EINTR)
 			goto again;
 
-		return -1;
+		return ret_errno(errno);
 	}
 
 	if (!ret)
 		return 0;
 
-	if (msg.msg_flags & MSG_TRUNC && (ret == nlmsghdr->nlmsg_len)) {
-		errno = EMSGSIZE;
-		ret = -1;
-	}
+	if (msg.msg_flags & MSG_TRUNC && (ret == nlmsghdr->nlmsg_len))
+		return ret_errno(EMSGSIZE);
 
 	return ret;
 }
 
-extern int netlink_rcv(struct nl_handler *handler, struct nlmsg *answer)
+int netlink_rcv(struct nl_handler *handler, struct nlmsg *answer)
 {
 	return __netlink_recv(handler, answer->nlmsghdr);
 }
 
-extern int __netlink_send(struct nl_handler *handler, struct nlmsghdr *nlmsghdr)
+int __netlink_send(struct nl_handler *handler, struct nlmsghdr *nlmsghdr)
 {
 	int ret;
 	struct sockaddr_nl nladdr;
@@ -223,7 +221,7 @@ extern int __netlink_send(struct nl_handler *handler, struct nlmsghdr *nlmsghdr)
 
 	ret = sendmsg(handler->fd, &msg, MSG_NOSIGNAL);
 	if (ret < 0)
-		return -1;
+		return ret_errno(errno);
 
 	return ret;
 }
@@ -241,21 +239,19 @@ extern int __netlink_transaction(struct nl_handler *handler,
 
 	ret = __netlink_send(handler, request);
 	if (ret < 0)
-		return -1;
+		return ret;
 
 	ret = __netlink_recv(handler, answer);
 	if (ret < 0)
-		return -1;
+		return ret;
 
-	ret = 0;
 	if (answer->nlmsg_type == NLMSG_ERROR) {
 		struct nlmsgerr *err = (struct nlmsgerr *)NLMSG_DATA(answer);
-		errno = -err->error;
 		if (err->error < 0)
-			ret = -1;
+			return ret_errno(-err->error);
 	}
 
-	return ret;
+	return 0;
 }
 
 extern int netlink_transaction(struct nl_handler *handler,
@@ -267,56 +263,44 @@ extern int netlink_transaction(struct nl_handler *handler,
 
 extern int netlink_open(struct nl_handler *handler, int protocol)
 {
+	__do_close int fd = -EBADF;
 	socklen_t socklen;
 	int sndbuf = 32768;
 	int rcvbuf = 32768;
-	int err;
 
 	memset(handler, 0, sizeof(*handler));
+	handler->fd = -EBADF;
 
-	handler->fd = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, protocol);
-	if (handler->fd < 0)
-		return -errno;
+	fd = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, protocol);
+	if (fd < 0)
+		return ret_errno(errno);
 
-	if (setsockopt(handler->fd, SOL_SOCKET, SO_SNDBUF,
-		       &sndbuf, sizeof(sndbuf)) < 0)
-		goto err_with_errno;
+	if (setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf)) < 0)
+		return ret_errno(errno);
 
-	if (setsockopt(handler->fd, SOL_SOCKET, SO_RCVBUF,
-		       &rcvbuf,sizeof(rcvbuf)) < 0)
-		goto err_with_errno;
+	if (setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &rcvbuf,sizeof(rcvbuf)) < 0)
+		return ret_errno(errno);
 
 	memset(&handler->local, 0, sizeof(handler->local));
 	handler->local.nl_family = AF_NETLINK;
 	handler->local.nl_groups = 0;
 
-	if (bind(handler->fd, (struct sockaddr*)&handler->local,
-		 sizeof(handler->local)) < 0)
-		goto err_with_errno;
+	if (bind(fd, (struct sockaddr*)&handler->local, sizeof(handler->local)) < 0)
+		return ret_errno(errno);
 
 	socklen = sizeof(handler->local);
-	if (getsockname(handler->fd, (struct sockaddr*)&handler->local,
-			&socklen) < 0)
-		goto err_with_errno;
+	if (getsockname(fd, (struct sockaddr*)&handler->local, &socklen) < 0)
+		return ret_errno(errno);
 
-	if (socklen != sizeof(handler->local)) {
-		err = -EINVAL;
-		goto errclose;
-	}
+	if (socklen != sizeof(handler->local))
+		return ret_errno(EINVAL);
 
-	if (handler->local.nl_family != AF_NETLINK) {
-		err = -EINVAL;
-		goto errclose;
-	}
+	if (handler->local.nl_family != AF_NETLINK)
+		return ret_errno(EINVAL);
 
 	handler->seq = time(NULL);
-
+	handler->fd = move_fd(fd);
 	return 0;
-err_with_errno:
-	err = -errno;
-errclose:
-	close(handler->fd);
-	return err;
 }
 
 extern void netlink_close(struct nl_handler *handler)
@@ -330,9 +314,8 @@ int addattr(struct nlmsghdr *n, size_t maxlen, int type, const void *data,
 	int len = RTA_LENGTH(alen);
 	struct rtattr *rta;
 
-	errno = EMSGSIZE;
 	if (NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len) > maxlen)
-		return -1;
+		return ret_errno(EMSGSIZE);
 
 	rta = NLMSG_TAIL(n);
 	rta->rta_type = type;