Merge pull request #1539 from brauner/2017-05-06/fix_abstract_unix_sockets

bugfixes: {segfaults, hashes, abstract unix sockets}

src/lxc/af_unix.c

@@ -55,8 +55,9 @@ int lxc_abstract_unix_open(const char *path, int type, int flags)
 
 	addr.sun_family = AF_UNIX;
 
-	len = strlen(&path[1]) + 1;
-	if (len >= sizeof(addr.sun_path) - 1) {
+	len = strlen(&path[1]);
+	/* do not enforce \0-termination */
+	if (len >= sizeof(addr.sun_path)) {
 		close(fd);
 		errno = ENAMETOOLONG;
 		return -1;
@@ -64,7 +65,7 @@ int lxc_abstract_unix_open(const char *path, int type, int flags)
 	/* addr.sun_path[0] has already been set to 0 by memset() */
 	strncpy(&addr.sun_path[1], &path[1], strlen(&path[1]));
 
-	if (bind(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len)) {
+	if (bind(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len + 1)) {
 		int tmp = errno;
 		close(fd);
 		errno = tmp;
@@ -109,8 +110,9 @@ int lxc_abstract_unix_connect(const char *path)
 
 	addr.sun_family = AF_UNIX;
 
-	len = strlen(&path[1]) + 1;
-	if (len >= sizeof(addr.sun_path) - 1) {
+	len = strlen(&path[1]);
+	/* do not enforce \0-termination */
+	if (len >= sizeof(addr.sun_path)) {
 		close(fd);
 		errno = ENAMETOOLONG;
 		return -1;
@@ -118,7 +120,7 @@ int lxc_abstract_unix_connect(const char *path)
 	/* addr.sun_path[0] has already been set to 0 by memset() */
 	strncpy(&addr.sun_path[1], &path[1], strlen(&path[1]));
 
-	if (connect(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len)) {
+	if (connect(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len + 1)) {
 		int tmp = errno;
 		/* special case to connect to older containers */
 		if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) == 0)
@@ -136,8 +138,8 @@ int lxc_abstract_unix_send_fd(int fd, int sendfd, void *data, size_t size)
 	struct msghdr msg = { 0 };
 	struct iovec iov;
 	struct cmsghdr *cmsg;
-	char cmsgbuf[CMSG_SPACE(sizeof(int))];
-	char buf[1];
+	char cmsgbuf[CMSG_SPACE(sizeof(int))] = {0};
+	char buf[1] = {0};
 	int *val;
 
 	msg.msg_control = cmsgbuf;
@@ -166,9 +168,9 @@ int lxc_abstract_unix_recv_fd(int fd, int *recvfd, void *data, size_t size)
 	struct msghdr msg = { 0 };
 	struct iovec iov;
 	struct cmsghdr *cmsg;
-	char cmsgbuf[CMSG_SPACE(sizeof(int))];
-	char buf[1];
 	int ret, *val;
+	char cmsgbuf[CMSG_SPACE(sizeof(int))] = {0};
+	char buf[1] = {0};
 
 	msg.msg_name = NULL;
 	msg.msg_namelen = 0;
@@ -210,8 +212,8 @@ int lxc_abstract_unix_send_credential(int fd, void *data, size_t size)
 		.uid = getuid(),
 		.gid = getgid(),
 	};
-	char cmsgbuf[CMSG_SPACE(sizeof(cred))];
-	char buf[1];
+	char cmsgbuf[CMSG_SPACE(sizeof(cred))] = {0};
+	char buf[1] = {0};
 
 	msg.msg_control = cmsgbuf;
 	msg.msg_controllen = sizeof(cmsgbuf);
@@ -239,9 +241,9 @@ int lxc_abstract_unix_rcv_credential(int fd, void *data, size_t size)
 	struct iovec iov;
 	struct cmsghdr *cmsg;
 	struct ucred cred;
-	char cmsgbuf[CMSG_SPACE(sizeof(cred))];
-	char buf[1];
 	int ret;
+	char cmsgbuf[CMSG_SPACE(sizeof(cred))] = {0};
+	char buf[1] = {0};
 
 	msg.msg_name = NULL;
 	msg.msg_namelen = 0;

src/lxc/af_unix.h

@@ -24,8 +24,10 @@
 #ifndef __LXC_AF_UNIX_H
 #define __LXC_AF_UNIX_H
 
+/* does not enforce \0-termination */
 extern int lxc_abstract_unix_open(const char *path, int type, int flags);
 extern int lxc_abstract_unix_close(int fd);
+/* does not enforce \0-termination */
 extern int lxc_abstract_unix_connect(const char *path);
 extern int lxc_abstract_unix_send_fd(int fd, int sendfd, void *data, size_t size);
 extern int lxc_abstract_unix_recv_fd(int fd, int *recvfd, void *data, size_t size);

src/lxc/commands.c

@@ -74,14 +74,19 @@
 
 lxc_log_define(lxc_commands, lxc);
 
-static int fill_sock_name(char *path, int len, const char *name,
+static int fill_sock_name(char *path, int len, const char *lxcname,
 			  const char *lxcpath, const char *hashed_sock_name)
 {
+	const char *name;
 	char *tmppath;
 	size_t tmplen;
 	uint64_t hash;
 	int ret;
 
+	name = lxcname;
+	if (!name)
+		name = "";
+
 	if (hashed_sock_name != NULL) {
 		ret = snprintf(path, len, "lxc/%s/command", hashed_sock_name);
 		if (ret < 0 || ret >= len) {
@@ -193,8 +198,11 @@ static int lxc_cmd_rsp_recv(int sock, struct lxc_cmd_rr *cmd)
 		rsp->data = rspdata;
 	}
 
-	if (rsp->datalen == 0)
+	if (rsp->datalen == 0) {
+		DEBUG("command %s response data length is 0",
+		      lxc_cmd_str(cmd->req.cmd));
 		return ret;
+	}
 	if (rsp->datalen > LXC_CMD_DATA_MAX) {
 		ERROR("Command %s response data %d too long.",
 		      lxc_cmd_str(cmd->req.cmd), rsp->datalen);
@@ -274,7 +282,7 @@ static int lxc_cmd(const char *name, struct lxc_cmd_rr *cmd, int *stopped,
 	int sock, ret = -1;
 	char path[sizeof(((struct sockaddr_un *)0)->sun_path)] = { 0 };
 	char *offset = &path[1];
-	int len;
+	size_t len;
 	int stay_connected = cmd->req.cmd == LXC_CMD_CONSOLE;
 
 	*stopped = 0;
@@ -982,7 +990,7 @@ int lxc_cmd_init(const char *name, struct lxc_handler *handler,
 	 * Although null termination isn't required by the API, we do it anyway
 	 * because we print the sockname out sometimes.
 	 */
-	len = sizeof(path)-2;
+	len = sizeof(path) - 2;
 	if (fill_sock_name(offset, len, name, lxcpath, NULL))
 		return -1;
 

src/lxc/lxccontainer.c

@@ -4370,7 +4370,10 @@ int list_active_containers(const char *lxcpath, char ***nret,
 		*p2 = '\0';
 
 		if (is_hashed) {
-			if (strncmp(lxcpath, lxc_cmd_get_lxcpath(p), lxcpath_len) != 0)
+			char *recvpath = lxc_cmd_get_lxcpath(p);
+			if (!recvpath)
+				continue;
+			if (strncmp(lxcpath, recvpath, lxcpath_len) != 0)
 				continue;
 			p = lxc_cmd_get_name(p);
 		}

src/lxc/monitor.c

@@ -153,36 +153,52 @@ int lxc_monitor_close(int fd)
 	return close(fd);
 }
 
+/* Enforces \0-termination for the abstract unix socket. This is not required
+ * but allows us to print it out.
+ *
+ * Older version of liblxc only allowed for 105 bytes to be used for the
+ * abstract unix domain socket name because the code for our abstract unix
+ * socket handling performed invalid checks. Since we \0-terminate we could now
+ * have a maximum of 106 chars. But to not break backwards compatibility we keep
+ * the limit at 105.
+ */
 int lxc_monitor_sock_name(const char *lxcpath, struct sockaddr_un *addr) {
 	size_t len;
 	int ret;
-	char *sockname;
 	char *path;
 	uint64_t hash;
 
 	/* addr.sun_path is only 108 bytes, so we hash the full name and
 	 * then append as much of the name as we can fit.
 	 */
-	sockname = &addr->sun_path[1];
 	memset(addr, 0, sizeof(*addr));
 	addr->sun_family = AF_UNIX;
 
+	/* strlen("lxc/") + strlen("/monitor-sock") + 1 = 18 */
 	len = strlen(lxcpath) + 18;
 	path = alloca(len);
 	ret = snprintf(path, len, "lxc/%s/monitor-sock", lxcpath);
 	if (ret < 0 || (size_t)ret >= len) {
-		ERROR("Failed to create path for monitor.");
+		ERROR("failed to create name for monitor socket");
 		return -1;
 	}
 
+	/* Note: snprintf() will \0-terminate addr->sun_path on the 106th byte
+	 * and so the abstract socket name has 105 "meaningful" characters. This
+	 * is absolutely intentional. For further info read the comment for this
+	 * function above!
+	 */
 	len = sizeof(addr->sun_path) - 1;
 	hash = fnv_64a_buf(path, ret, FNV1A_64_INIT);
-	ret = snprintf(sockname, len, "lxc/%016" PRIx64 "/%s", hash, lxcpath);
-	if (ret < 0)
+	ret = snprintf(addr->sun_path, len, "@lxc/%016" PRIx64 "/%s", hash, lxcpath);
+	if (ret < 0) {
+		ERROR("failed to create hashed name for monitor socket");
 		return -1;
+	}
 
-	sockname[sizeof(addr->sun_path)-3] = '\0';
-	INFO("Using monitor socket name \"%s\".", sockname);
+	/* replace @ with \0 */
+	addr->sun_path[0] = '\0';
+	INFO("using monitor socket name \"%s\" (length of socket name %zu must be <= %zu)", &addr->sun_path[1], strlen(&addr->sun_path[1]), sizeof(addr->sun_path) - 3);
 
 	return 0;
 }
@@ -193,7 +209,8 @@ int lxc_monitor_open(const char *lxcpath)
 	int fd;
 	size_t retry;
 	size_t len;
-	int ret = 0, backoff_ms[] = {10, 50, 100};
+	int ret = -1;
+	int backoff_ms[] = {10, 50, 100};
 
 	if (lxc_monitor_sock_name(lxcpath, &addr) < 0)
 		return -1;
@@ -201,28 +218,32 @@ int lxc_monitor_open(const char *lxcpath)
 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
 	if (fd < 0) {
 		ERROR("Failed to create socket: %s.", strerror(errno));
-		return -1;
+		return -errno;
 	}
 
-	len = strlen(&addr.sun_path[1]) + 1;
+	len = strlen(&addr.sun_path[1]);
+	DEBUG("opening monitor socket %s with len %zu", &addr.sun_path[1], len);
 	if (len >= sizeof(addr.sun_path) - 1) {
-		ret = -1;
 		errno = ENAMETOOLONG;
+		ret = -errno;
+		ERROR("name of monitor socket too long (%zu bytes): %s", len, strerror(errno));
 		goto on_error;
 	}
 
 	for (retry = 0; retry < sizeof(backoff_ms) / sizeof(backoff_ms[0]); retry++) {
-		ret = connect(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len);
-		if (ret == 0 || errno != ECONNREFUSED)
+		fd = lxc_abstract_unix_connect(addr.sun_path);
+		if (fd < 0 || errno != ECONNREFUSED)
 			break;
-		ERROR("Failed to connect to monitor socket. Retrying in %d ms.", backoff_ms[retry]);
+		ERROR("Failed to connect to monitor socket. Retrying in %d ms: %s", backoff_ms[retry], strerror(errno));
 		usleep(backoff_ms[retry] * 1000);
 	}
 
-	if (ret < 0) {
+	if (fd < 0) {
+		ret = -errno;
 		ERROR("Failed to connect to monitor socket: %s.", strerror(errno));
 		goto on_error;
 	}
+	ret = 0;
 
 	return fd;