[lxc-devel] [lxc/master] conf: improve mountinfo and config parsing

[brauner on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 364 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20201116/0dfcc236/attachment.bin>
-------------- next part --------------
From 1e3b7c8177eee4dde31d2112ed77dab4b4873b67 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Mon, 16 Nov 2020 12:18:14 +0100
Subject: [PATCH 1/2] parse: rework config parsing routine

Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/parse.c | 54 +++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/src/lxc/parse.c b/src/lxc/parse.c
index 291bf3efc1..fe459b527a 100644
--- a/src/lxc/parse.c
+++ b/src/lxc/parse.c
@@ -5,6 +5,7 @@
 #endif
 #include <dirent.h>
 #include <errno.h>
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -50,11 +51,12 @@ int lxc_strmunmap(void *addr, size_t length)
 
 int lxc_file_for_each_line_mmap(const char *file, lxc_file_cb callback, void *data)
 {
-	int saved_errno;
-	ssize_t ret = -1, bytes_sent;
-	char *line;
-	int fd = -1, memfd = -1;
+	__do_close int fd = -EBADF, memfd = -EBADF;
+	ssize_t ret = -1;
 	char *buf = NULL;
+	struct stat st = {};
+	ssize_t bytes;
+	char *line;
 
 	memfd = memfd_create(".lxc_config_file", MFD_CLOEXEC);
 	if (memfd < 0) {
@@ -65,8 +67,7 @@ int lxc_file_for_each_line_mmap(const char *file, lxc_file_cb callback, void *da
 			goto on_error;
 		}
 
-		TRACE("Failed to create in-memory file. Falling back to "
-		      "temporary file");
+		TRACE("Failed to create in-memory file. Falling back to temporary file");
 		memfd = lxc_make_tmpfile(template, true);
 		if (memfd < 0) {
 			SYSERROR("Failed to create temporary file \"%s\"", template);
@@ -80,10 +81,26 @@ int lxc_file_for_each_line_mmap(const char *file, lxc_file_cb callback, void *da
 		goto on_error;
 	}
 
-	/* sendfile() handles up to 2GB. No config file should be that big. */
-	bytes_sent = lxc_sendfile_nointr(memfd, fd, NULL, LXC_SENDFILE_MAX);
-	if (bytes_sent < 0) {
-		SYSERROR("Failed to sendfile \"%s\"", file);
+	ret = fstat(fd, &st);
+	if (ret) {
+		SYSERROR("Failed to stat file \"%s\"", file);
+		goto on_error;
+	}
+
+	if (st.st_size > INT_MAX) {
+		SYSERROR("Excessively large config file \"%s\"", file);
+		goto on_error;
+	}
+
+
+	bytes = fd_to_fd(fd, memfd);
+	if (bytes < 0) {
+		SYSERROR("Failed to copy config file \"%s\"", file);
+		goto on_error;
+	}
+
+	if (bytes != st.st_size) {
+		SYSERROR("Size of the config file \"%s\" seems to have changed while reading it", file);
 		goto on_error;
 	}
 
@@ -92,7 +109,7 @@ int lxc_file_for_each_line_mmap(const char *file, lxc_file_cb callback, void *da
 		SYSERROR("Failed to append zero byte");
 		goto on_error;
 	}
-	bytes_sent++;
+	bytes++;
 
 	ret = lseek(memfd, 0, SEEK_SET);
 	if (ret < 0) {
@@ -101,8 +118,7 @@ int lxc_file_for_each_line_mmap(const char *file, lxc_file_cb callback, void *da
 	}
 
 	ret = -1;
-	buf = mmap(NULL, bytes_sent, PROT_READ | PROT_WRITE,
-		   MAP_SHARED | MAP_POPULATE, memfd, 0);
+	buf = mmap(NULL, bytes, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_POPULATE, memfd, 0);
 	if (buf == MAP_FAILED) {
 		buf = NULL;
 		SYSERROR("Failed to mmap");
@@ -117,24 +133,18 @@ int lxc_file_for_each_line_mmap(const char *file, lxc_file_cb callback, void *da
 			 * error.
 			 */
 			if (ret < 0)
-				ERROR("Failed to parse config file \"%s\" at "
-				      "line \"%s\"", file, line);
+				ERROR("Failed to parse config file \"%s\" at line \"%s\"",
+				      file, line);
 			break;
 		}
 	}
 
 on_error:
-	saved_errno = errno;
-	if (fd >= 0)
-		close(fd);
-	if (memfd >= 0)
-		close(memfd);
-	if (buf && munmap(buf, bytes_sent)) {
+	if (buf && munmap(buf, bytes)) {
 		SYSERROR("Failed to unmap");
 		if (ret == 0)
 			ret = -1;
 	}
-	errno = saved_errno;
 
 	return ret;
 }

From a7da00867df0f4367e3b34f2c3ba9e07198ca90a Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Mon, 16 Nov 2020 12:30:18 +0100
Subject: [PATCH 2/2] conf: switch to fd_to_fd() when copying mountinfo

Closes: #3580.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=209971
Suggested-by: Joan Bruguera <joanbrugueram at gmail.com>
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/conf.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index d36d9063b6..84d16d7749 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -2980,9 +2980,9 @@ void turn_into_dependent_mounts(void)
 	__do_free char *line = NULL;
 	__do_fclose FILE *f = NULL;
 	__do_close int memfd = -EBADF, mntinfo_fd = -EBADF;
-	int ret;
-	ssize_t copied;
 	size_t len = 0;
+	ssize_t copied;
+	int ret;
 
 	mntinfo_fd = open("/proc/self/mountinfo", O_RDONLY | O_CLOEXEC);
 	if (mntinfo_fd < 0) {
@@ -3006,12 +3006,8 @@ void turn_into_dependent_mounts(void)
 		}
 	}
 
-again:
-	copied = lxc_sendfile_nointr(memfd, mntinfo_fd, NULL, LXC_SENDFILE_MAX);
+	copied = fd_to_fd(mntinfo_fd, memfd);
 	if (copied < 0) {
-		if (errno == EINTR)
-			goto again;
-
 		SYSERROR("Failed to copy \"/proc/self/mountinfo\"");
 		return;
 	}