From: Michael Tremer <michael.tremer@ipfire.org>
Date: Sun, 17 Jan 2021 18:03:42 +0000 (+0000)
Subject: libpakfire: execute: Statically allocate the log buffer
X-Git-Tag: 0.9.28~1285^2~848
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=608bdb89dfca38bc47dc2c8de024160a308380b0;p=pakfire.git

libpakfire: execute: Statically allocate the log buffer

This prevents the buffer from growing unboundedly, but limits us to only
process log messages of up to the buffer size.

I have chosen 64k which should be more than enought that we never run
into this situation.

Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
---

diff --git a/src/libpakfire/execute.c b/src/libpakfire/execute.c
index 02fd2f0a7..1c48b764d 100644
--- a/src/libpakfire/execute.c
+++ b/src/libpakfire/execute.c
@@ -36,6 +36,7 @@
 #include <pakfire/private.h>
 #include <pakfire/types.h>
 
+#define BUFFER_SIZE      1024 * 64
 #define EPOLL_MAX_EVENTS 2
 
 static char* envp_empty[1] = { NULL };
@@ -51,46 +52,12 @@ struct pakfire_execute {
 };
 
 struct pakfire_execute_buffer {
-	char* data;
-	size_t size;
+	char data[BUFFER_SIZE];
 	size_t used;
 };
 
-static int pakfire_execute_buffer_realloc(Pakfire pakfire, struct pakfire_execute_buffer* buffer, size_t size) {
-	// We cannot decrease the buffer size
-	if (size <= buffer->size)
-		return -EINVAL;
-
-	// Allocate the new buffer
-	buffer->data = realloc(buffer->data, size);
-	if (!buffer->data)
-		return -errno;
-
-	buffer->size = size;
-
-	DEBUG(pakfire, "Buffer %p is now %zu byte(s) long\n", buffer, buffer->size);
-
-	return 0;
-}
-
-static int pakfire_execute_buffer_init(Pakfire pakfire, struct pakfire_execute_buffer* buffer) {
-	// Initialize all values with nothing
-	buffer->data = NULL;
-	buffer->size = buffer->used = 0;
-
-	// Allocate one page
-	return pakfire_execute_buffer_realloc(pakfire, buffer, PAGE_SIZE);
-}
-
-static void pakfire_execute_buffer_free(struct pakfire_execute_buffer* buffer) {
-	if (buffer->data)
-		free(buffer->data);
-
-	buffer->size = buffer->used = 0;
-}
-
 static int pakfire_execute_buffer_is_full(const struct pakfire_execute_buffer* buffer) {
-	return (buffer->size == buffer->used);
+	return (sizeof(buffer->data) == buffer->used);
 }
 
 /*
@@ -100,10 +67,12 @@ static int pakfire_execute_buffer_is_full(const struct pakfire_execute_buffer* b
 */
 static int pakfire_execute_logger_proxy(Pakfire pakfire,
 		int(*log_func)(Pakfire pakfire, const char* data), int fd, struct pakfire_execute_buffer* buffer) {
+	char line[BUFFER_SIZE + 1];
+
 	// Fill up buffer from fd
-	while (buffer->used < buffer->size) {
+	while (buffer->used < sizeof(buffer->data)) {
 		ssize_t bytes_read = read(fd, buffer->data + buffer->used,
-				buffer->size - buffer->used);
+				sizeof(buffer->data) - buffer->used);
 
 		// Handle errors
 		if (bytes_read < 0) {
@@ -128,27 +97,20 @@ static int pakfire_execute_logger_proxy(Pakfire pakfire,
 
 		// No newline found
 		if (!eol) {
-			// If the buffer is full, we double its size and try to read more
+			// If the buffer is full, we send the content to the logger and try again
+			// This should not happen in practise
 			if (pakfire_execute_buffer_is_full(buffer)) {
-				int r = pakfire_execute_buffer_realloc(pakfire, buffer, buffer->size * 2);
-				if (r)
-					return -1;
-
-				return pakfire_execute_logger_proxy(pakfire, log_func, fd, buffer);
-			}
+				ERROR(pakfire, "Logging buffer is full. Sending all content\n");
+				eol = buffer->data + sizeof(buffer->data) - 1;
 
 			// Otherwise we might have only read parts of the output
-			return 0;
+			} else
+				break;
 		}
 
 		// Find the length of the string
 		size_t length = eol - buffer->data + 1;
 
-		DEBUG(pakfire, "Found a line of %zu byte(s) length\n", length);
-
-		// Allocate a new buffer that is large enough to hold the line
-		char* line = alloca(length + 1);
-
 		// Copy the line into the buffer
 		memcpy(line, buffer->data, length);
 
@@ -184,18 +146,7 @@ static int pakfire_execute_logger(Pakfire pakfire, struct pakfire_execute_logger
 		struct pakfire_execute_buffer stdout;
 		struct pakfire_execute_buffer stderr;
 	} buffers;
-
-	r = pakfire_execute_buffer_init(pakfire, &buffers.stdout);
-	if (r) {
-		ERROR(pakfire, "Could not initialize buffer for stdout: %s\n", strerror(errno));
-		goto OUT;
-	}
-
-	r = pakfire_execute_buffer_init(pakfire, &buffers.stderr);
-	if (r) {
-		ERROR(pakfire, "Could not initialize buffer for stderr: %s\n", strerror(errno));
-		goto OUT;
-	}
+	buffers.stdout.used = buffers.stderr.used = 0;
 
 	// Setup epoll
 	epollfd = epoll_create1(0);
@@ -272,10 +223,6 @@ OUT:
 	if (epollfd > 0)
 		close(epollfd);
 
-	// Free buffers
-	pakfire_execute_buffer_free(&buffers.stdout);
-	pakfire_execute_buffer_free(&buffers.stderr);
-
 	return r;
 }
 
diff --git a/tests/python/execute.py b/tests/python/execute.py
index f8719411c..25f60f112 100755
--- a/tests/python/execute.py
+++ b/tests/python/execute.py
@@ -61,8 +61,8 @@ class Test(unittest.TestCase):
 	def test_execute_output(self):
 		self.pakfire.execute(["/bin/bash", "--help"])
 
-		# Run a command with a lot of output which will increase the buffer sizes
-		self.pakfire.execute(["/usr/bin/openssl", "rand", "-hex", "4096"])
+		# Run a command with a lot of output which exceeds the buffer size
+		self.pakfire.execute(["/usr/bin/openssl", "rand", "-hex", "65536"])
 
 		# Run a command that generates lots of lines
 		self.pakfire.execute(["/usr/bin/openssl", "rand", "-base64", "4096"])