external-helper: improve run_external_process() robustness

run_external_process() contained pipe file descriptors leaks (e.g. one
pipe end was never closed). Also the stdout might have been captured
incomplete, since only a single read() was performed on the pipe.
Furthermore should a child process try to write a larger amount of data
onto the pipe then it will become stuck, because the parent process
isn't consuming the data. Thus the timeout would trigger in these cases
although the child process does nothing wrong.

This commit changes the implementation to follow a select() based
approach that continually reads from the pipe, but discards data that
doesn't fit in the provided buffer.

daemon/external-helper.c

@@ -36,11 +36,75 @@ POSSIBILITY OF SUCH DAMAGE.
 
 #include <linux/limits.h>
 #include <stdio.h>
+#include <sys/time.h>
+#include <sys/types.h>
 #include <sys/wait.h>
-#include <time.h>
 #include <unistd.h>
 
-static const int default_timeout = 5;
+static const int DEFAULT_TIMEOUT = 5;
+
+static int read_child_stdout(int pipe_fd, char buffer[EXTERNAL_BUFFER_MAX], int tsec)
+{
+	fd_set fds;
+	struct timeval timeout;
+	int num_readable = 0;
+	ssize_t buffer_bytes_read = 0;
+	ssize_t just_read = 0;
+	bool buffer_full = false;
+	char discard_buffer[EXTERNAL_BUFFER_MAX];
+
+	/* Set up the timout */
+	timeout.tv_sec = tsec;
+	timeout.tv_usec = 0;
+
+	FD_ZERO(&fds);
+
+	/* Wait for the child to finish up with a timout */
+	while (true) {
+		FD_SET(pipe_fd, &fds);
+		num_readable = select(pipe_fd + 1, &fds, NULL, NULL, &timeout);
+
+		if (num_readable < 0) {
+			if (errno == EINTR) {
+				continue;
+			} else {
+				LOG_ERROR("sigtimedwait failed: %s\n", strerror(errno));
+				return -1;
+			}
+		} else if (num_readable == 0) {
+			return -2;
+		}
+
+		if (!buffer_full) {
+			just_read = read(pipe_fd,
+			                 buffer + buffer_bytes_read,
+			                 EXTERNAL_BUFFER_MAX - (size_t)buffer_bytes_read - 1);
+		} else {
+			just_read = read(pipe_fd, discard_buffer, EXTERNAL_BUFFER_MAX - 1);
+		}
+
+		if (just_read < 0) {
+			return -1;
+		} else if (just_read == 0) {
+			// EOF encountered
+			break;
+		}
+
+		if (!buffer_full) {
+			buffer_bytes_read += just_read;
+
+			if (buffer_bytes_read == EXTERNAL_BUFFER_MAX - 1) {
+				// our buffer is exhausted, discard the rest
+				// of the output
+				buffer_full = true;
+			}
+		}
+	}
+
+	buffer[buffer_bytes_read] = 0;
+
+	return 0;
+}
 
 /**
  * Call an external process
@@ -50,6 +114,7 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF
 	pid_t p;
 	int status = 0;
 	int pipes[2];
+	int ret = 0;
 	char internal[EXTERNAL_BUFFER_MAX] = { 0 };
 
 	if (pipe(pipes) == -1) {
@@ -59,20 +124,12 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF
 
 	/* Set the default timeout */
 	if (tsec == -1) {
-		tsec = default_timeout;
-	}
-
-	/* set up our signaling for the child and the timout */
-	sigset_t mask;
-	sigset_t omask;
-	sigemptyset(&mask);
-	sigaddset(&mask, SIGCHLD);
-	if (sigprocmask(SIG_BLOCK, &mask, &omask) < 0) {
-		LOG_ERROR("sigprocmask failed: %s\n", strerror(errno));
-		return -1;
+		tsec = DEFAULT_TIMEOUT;
 	}
 
 	if ((p = fork()) < 0) {
+		close(pipes[0]);
+		close(pipes[1]);
 		LOG_ERROR("Failed to fork(): %s\n", strerror(errno));
 		return false;
 	} else if (p == 0) {
@@ -91,39 +148,31 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF
 			LOG_ERROR("Failed to execute external process: %s %s\n", exec_args[0], strerror(errno));
 			exit(EXIT_FAILURE);
 		}
-		_exit(EXIT_SUCCESS);
-	}
-
-	/* Set up the timout */
-	struct timespec timeout;
-	timeout.tv_sec = tsec;
-	timeout.tv_nsec = 0;
-
-	/* Wait for the child to finish up with a timout */
-	while (true) {
-		if (sigtimedwait(&mask, NULL, &timeout) < 0) {
-			if (errno == EINTR) {
-				continue;
-			} else if (errno == EAGAIN) {
-				LOG_ERROR("Child process timed out for %s, killing and returning\n", exec_args[0]);
-				kill(p, SIGKILL);
-			} else {
-				LOG_ERROR("sigtimedwait failed: %s\n", strerror(errno));
-				return -1;
-			}
-		}
-		break;
+		// should never be reached
+		abort();
 	}
 
+	// close the write end of the pipe so we get signaled EOF once the
+	// child exits
 	close(pipes[1]);
-	ssize_t output_size = read(pipes[0], internal, EXTERNAL_BUFFER_MAX - 1);
-	if (output_size < 0) {
-		LOG_ERROR("Failed to read from process %s: %s\n", exec_args[0], strerror(errno));
-		return -1;
+	ret = read_child_stdout(pipes[0], internal, tsec);
+	close(pipes[0]);
+
+	if (ret != 0) {
+		if (ret == -2) {
+			LOG_ERROR("Child process timed out for %s, killing and returning\n", exec_args[0]);
+			kill(p, SIGKILL);
+		} else {
+			LOG_ERROR("Failed to read from process %s: %s\n", exec_args[0], strerror(errno));
+		}
+		if (buffer) {
+			// make sure the buffer is a terminated empty string on error
+			buffer[0] = 0;
+		}
+	} else if (buffer) {
+		memcpy(buffer, internal, EXTERNAL_BUFFER_MAX);
 	}
 
-	internal[output_size] = 0;
-
 	if (waitpid(p, &status, 0) < 0) {
 		LOG_ERROR("Failed to waitpid(%d): %s\n", (int)p, strerror(errno));
 		return -1;
@@ -134,12 +183,9 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF
 		LOG_ERROR("Child process '%s' exited abnormally\n", exec_args[0]);
 	} else if (WEXITSTATUS(status) != 0) {
 		LOG_ERROR("External process failed with exit code %d\n", WEXITSTATUS(status));
-		LOG_ERROR("Output was: %s\n", buffer ? buffer : internal);
+		LOG_ERROR("Output was: %s\n", internal);
 		return -1;
 	}
 
-	if (buffer)
-		memcpy(buffer, internal, EXTERNAL_BUFFER_MAX);
-
 	return 0;
 }

daemon/gpuclockctl.c

@@ -67,7 +67,7 @@ static void print_usage_and_exit(void)
 	exit(EXIT_FAILURE);
 }
 
-static const char* get_nv_attr(const char *attr)
+static const char *get_nv_attr(const char *attr)
 {
 	static char out[EXTERNAL_BUFFER_MAX];
 	const char *exec_args[] = { "/usr/bin/nvidia-settings", "-q", attr, "-t", NULL };
@@ -315,8 +315,7 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info)
 		ret = -1;
 	}
 
-	if( ret == 0 )
-	{
+	if (ret == 0) {
 		/* Copy in the value from the file */
 		strncpy(info->amd_performance_level, buff, CONFIG_VALUE_MAX - 1);
 		info->amd_performance_level[CONFIG_VALUE_MAX - 1] = '\0';