[lxc-devel] [lxc/master] commands: fix state socket implementation

[brauner on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 1268 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20171119/2913a45f/attachment.bin>
-------------- next part --------------
From 344af23f19aca3edc67c3808289377817f65eca9 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Sun, 19 Nov 2017 18:59:11 +0100
Subject: [PATCH] commands: fix state socket implementation

The old state socket implementation used the command socket itself to send
the container state to state clients. This is problematic when e.g. the
container doesn't respond to a shutdown request and hangs and the container is
subsequently killed with a stop command. In this case the monitor would write
the STOPPED message into the command socket which would then end up as
additional data for the stop function leading to nonsense such as this:

root at conventiont|~
> lxc-start -n a1
root at conventiont|~
> lxc-stop -n a1
^C
root at conventiont|~
> lxc-stop -n a1 -k
lxc-stop: a1: commands.c: lxc_cmd_rsp_recv: 165 File too large - Response data for command "stop" is too long: 12641 bytes > 8192

This is also insecure. So switch to an implementation that uses an additional
socketpair() per client. This wastes more file descriptors but retains backwards
compatibility and is cleaner.

Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/commands.c       | 23 +++++++++++++++++++----
 src/lxc/commands_utils.c |  8 +++-----
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/lxc/commands.c b/src/lxc/commands.c
index f0f3c076a..5c4dd621a 100644
--- a/src/lxc/commands.c
+++ b/src/lxc/commands.c
@@ -150,6 +150,8 @@ static int lxc_cmd_rsp_recv(int sock, struct lxc_cmd_rr *cmd)
 		rspdata->masterfd = rspfd;
 		rspdata->ttynum = PTR_TO_INT(rsp->data);
 		rsp->data = rspdata;
+	} else if (cmd->req.cmd == LXC_CMD_ADD_STATE_CLIENT) {
+		rsp->ret = rspfd;
 	}
 
 	if (rsp->datalen == 0) {
@@ -965,6 +967,8 @@ int lxc_cmd_add_state_client(const char *name, const char *lxcpath,
 static int lxc_cmd_add_state_client_callback(int fd, struct lxc_cmd_req *req,
 					     struct lxc_handler *handler)
 {
+	int ret;
+	int sockfds[2];
 	struct lxc_cmd_rsp rsp = {0};
 
 	if (req->datalen < 0)
@@ -976,13 +980,24 @@ static int lxc_cmd_add_state_client_callback(int fd, struct lxc_cmd_req *req,
 	if (!req->data)
 		return -1;
 
-	rsp.ret = lxc_add_state_client(fd, handler, (lxc_state_t *)req->data);
+	ret = socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, sockfds);
+	if (ret < 0)
+		return -1;
+	close(sockfds[1]);
+
+	rsp.ret = lxc_add_state_client(sockfds[0], handler, (lxc_state_t *)req->data);
 	if (rsp.ret < 0)
-		ERROR("Failed to add state client %d to state client list", fd);
+		ERROR("Failed to add state client %d to state client list", sockfds[0]);
 	else
-		TRACE("Added state client %d to state client list", fd);
+		TRACE("Added state client %d to state client list", sockfds[0]);
 
-	return lxc_cmd_rsp_send(fd, &rsp);
+	ret = lxc_abstract_unix_send_fds(fd, &sockfds[0], 1, &rsp, sizeof(rsp));
+	if (ret < 0) {
+		ERROR("Failed to send tty to client");
+		return -1;
+	}
+
+	return 0;
 }
 
 int lxc_cmd_console_log(const char *name, const char *lxcpath,
diff --git a/src/lxc/commands_utils.c b/src/lxc/commands_utils.c
index 23a0b9504..49b3c5e6b 100644
--- a/src/lxc/commands_utils.c
+++ b/src/lxc/commands_utils.c
@@ -68,16 +68,14 @@ int lxc_cmd_sock_rcv_state(int state_client_fd, int timeout)
 			goto again;
 		}
 
-		ERROR("failed to receive message: %s", strerror(errno));
+		ERROR("Failed to receive message: %s", strerror(errno));
 		return -1;
 	}
 
-	if (ret == 0) {
-		ERROR("length of message was 0");
+	if (ret < 0)
 		return -1;
-	}
 
-	TRACE("received state %s from state client %d",
+	TRACE("Received state %s from state client %d",
 	      lxc_state2str(msg.value), state_client_fd);
 
 	return msg.value;