logwrapper: prevent logwrap from hanging when child dies
Sometimes the read on the PTY can wait indefinitely if the child
dies. By using a poll statement that monitors both the output
of the child and its state we prevent this from happening.
Change-Id: I51d5556c66f039bca673145ca72db262977e1689
diff --git a/logwrapper/include/logwrap/logwrap.h b/logwrapper/include/logwrap/logwrap.h
index a58f238..722dda2 100644
--- a/logwrapper/include/logwrap/logwrap.h
+++ b/logwrapper/include/logwrap/logwrap.h
@@ -20,7 +20,29 @@
__BEGIN_DECLS
-int logwrap(int argc, char* argv[], int *chld_sts);
+/*
+ * Run a command while logging its stdout and stderr
+ *
+ * WARNING: while this function is running it will clear all SIGCHLD handlers
+ * if you rely on SIGCHLD in the caller there is a chance zombies will be
+ * created if you're not calling waitpid after calling this. This function will
+ * log a warning when it clears SIGCHLD for processes other than the child it
+ * created.
+ *
+ * Arguments:
+ * argc: the number of elements in argv
+ * argv: an array of strings containing the command to be executed and its
+ * arguments as separate strings. argv does not need to be
+ * NULL-terminated
+ * status: the equivalent child status as populated by wait(status). This
+ * value is only valid when logwrap successfully completes
+ *
+ * Return value:
+ * 0 when logwrap successfully run the child process and captured its status
+ * -1 when an internal error occurred
+ *
+ */
+int logwrap(int argc, char* argv[], int *status);
__END_DECLS
diff --git a/logwrapper/logwrap.c b/logwrapper/logwrap.c
index 302a739..c2b36be 100644
--- a/logwrapper/logwrap.c
+++ b/logwrapper/logwrap.c
@@ -16,6 +16,9 @@
#include <string.h>
#include <sys/types.h>
+#include <sys/signalfd.h>
+#include <signal.h>
+#include <poll.h>
#include <sys/wait.h>
#include <stdio.h>
#include <stdlib.h>
@@ -28,15 +31,28 @@
#include "private/android_filesystem_config.h"
#include "cutils/log.h"
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+
static int fatal(const char *msg) {
fprintf(stderr, "%s", msg);
ALOG(LOG_ERROR, "logwrapper", "%s", msg);
return -1;
}
-void parent(const char *tag, int parent_read, int *chld_sts) {
+static int parent(const char *tag, int parent_read, int signal_fd, pid_t pid,
+ int *chld_sts) {
int status;
char buffer[4096];
+ struct pollfd poll_fds[] = {
+ [0] = {
+ .fd = parent_read,
+ .events = POLLIN,
+ },
+ [1] = {
+ .fd = signal_fd,
+ .events = POLLIN,
+ },
+ };
int a = 0; // start index of unprocessed data
int b = 0; // end index of unprocessed data
@@ -45,60 +61,82 @@
char *btag = basename(tag);
if (!btag) btag = (char*) tag;
- while ((sz = read(parent_read, &buffer[b], sizeof(buffer) - 1 - b)) > 0) {
+ while (1) {
+ if (poll(poll_fds, ARRAY_SIZE(poll_fds), -1) <= 0) {
+ return fatal("poll failed\n");
+ }
- sz += b;
- // Log one line at a time
- for (b = 0; b < sz; b++) {
- if (buffer[b] == '\r') {
- buffer[b] = '\0';
- } else if (buffer[b] == '\n') {
+ if (poll_fds[0].revents & POLLIN) {
+ sz = read(parent_read, &buffer[b], sizeof(buffer) - 1 - b);
+
+ sz += b;
+ // Log one line at a time
+ for (b = 0; b < sz; b++) {
+ if (buffer[b] == '\r') {
+ buffer[b] = '\0';
+ } else if (buffer[b] == '\n') {
+ buffer[b] = '\0';
+ ALOG(LOG_INFO, btag, "%s", &buffer[a]);
+ a = b + 1;
+ }
+ }
+
+ if (a == 0 && b == sizeof(buffer) - 1) {
+ // buffer is full, flush
buffer[b] = '\0';
ALOG(LOG_INFO, btag, "%s", &buffer[a]);
- a = b + 1;
+ b = 0;
+ } else if (a != b) {
+ // Keep left-overs
+ b -= a;
+ memmove(buffer, &buffer[a], b);
+ a = 0;
+ } else {
+ a = 0;
+ b = 0;
}
}
- if (a == 0 && b == sizeof(buffer) - 1) {
- // buffer is full, flush
- buffer[b] = '\0';
- ALOG(LOG_INFO, btag, "%s", &buffer[a]);
- b = 0;
- } else if (a != b) {
- // Keep left-overs
- b -= a;
- memmove(buffer, &buffer[a], b);
- a = 0;
- } else {
- a = 0;
- b = 0;
- }
+ if (poll_fds[1].revents & POLLIN) {
+ struct signalfd_siginfo sfd_info;
+ pid_t wpid;
+ // Clear all pending signals before reading the child's status
+ while (read(signal_fd, &sfd_info, sizeof(sfd_info)) > 0) {
+ if ((pid_t)sfd_info.ssi_pid != pid)
+ ALOG(LOG_WARN, "logwrapper", "cleared SIGCHLD for pid %u\n",
+ sfd_info.ssi_pid);
+ }
+ wpid = waitpid(pid, &status, WNOHANG);
+ if (wpid > 0)
+ break;
+ }
}
+
// Flush remaining data
if (a != b) {
buffer[b] = '\0';
ALOG(LOG_INFO, btag, "%s", &buffer[a]);
}
- if (wait(&status) != -1) { // Wait for child
- if (WIFEXITED(status) && WEXITSTATUS(status))
+ if (WIFEXITED(status)) {
+ if (WEXITSTATUS(status))
ALOG(LOG_INFO, "logwrapper", "%s terminated by exit(%d)", tag,
WEXITSTATUS(status));
- else if (WIFSIGNALED(status))
- ALOG(LOG_INFO, "logwrapper", "%s terminated by signal %d", tag,
- WTERMSIG(status));
- else if (WIFSTOPPED(status))
- ALOG(LOG_INFO, "logwrapper", "%s stopped by signal %d", tag,
- WSTOPSIG(status));
- if (chld_sts != NULL)
- *chld_sts = status;
- } else
- ALOG(LOG_INFO, "logwrapper", "%s wait() failed: %s (%d)", tag,
- strerror(errno), errno);
+ } else if (WIFSIGNALED(status)) {
+ ALOG(LOG_INFO, "logwrapper", "%s terminated by signal %d", tag,
+ WTERMSIG(status));
+ } else if (WIFSTOPPED(status)) {
+ ALOG(LOG_INFO, "logwrapper", "%s stopped by signal %d", tag,
+ WSTOPSIG(status));
+ }
+ if (chld_sts != NULL)
+ *chld_sts = status;
+
+ return 0;
}
-void child(int argc, char* argv[]) {
+static void child(int argc, char* argv[]) {
// create null terminated argv_child array
char* argv_child[argc + 1];
memcpy(argv_child, argv, argc * sizeof(char *));
@@ -111,12 +149,13 @@
}
}
-int logwrap(int argc, char* argv[], int *chld_sts) {
+int logwrap(int argc, char* argv[], int *status) {
pid_t pid;
int parent_ptty;
int child_ptty;
char *child_devname = NULL;
+ sigset_t chldset;
/* Use ptty instead of socketpair so that STDOUT is not buffered */
parent_ptty = open("/dev/ptmx", O_RDWR);
@@ -129,13 +168,19 @@
return fatal("Problem with /dev/ptmx\n");
}
+ sigemptyset(&chldset);
+ sigaddset(&chldset, SIGCHLD);
+ sigprocmask(SIG_BLOCK, &chldset, NULL);
+
pid = fork();
if (pid < 0) {
close(parent_ptty);
+ sigprocmask(SIG_UNBLOCK, &chldset, NULL);
return fatal("Failed to fork\n");
} else if (pid == 0) {
- child_ptty = open(child_devname, O_RDWR);
close(parent_ptty);
+ sigprocmask(SIG_UNBLOCK, &chldset, NULL);
+ child_ptty = open(child_devname, O_RDWR);
if (child_ptty < 0) {
return fatal("Problem with child ptty\n");
}
@@ -146,16 +191,35 @@
close(child_ptty);
child(argc - 1, &argv[1]);
+ return fatal("This should never happen\n");
} else {
+ int rc;
+ int fd;
+
+ fd = signalfd(-1, &chldset, SFD_NONBLOCK);
+ if (fd == -1) {
+ char msg[40];
+
+ snprintf(msg, sizeof(msg), "signalfd failed: %d\n", errno);
+
+ close(parent_ptty);
+ sigprocmask(SIG_UNBLOCK, &chldset, NULL);
+ return fatal(msg);
+ }
+
// switch user and group to "log"
- // this may fail if we are not root,
- // but in that case switching user/group is unnecessary
+ // this may fail if we are not root,
+ // but in that case switching user/group is unnecessary
setgid(AID_LOG);
setuid(AID_LOG);
- parent(argv[1], parent_ptty, chld_sts);
- }
+ rc = parent(argv[1], parent_ptty, fd, pid, status);
+ close(parent_ptty);
+ close(fd);
- return 0;
+ sigprocmask(SIG_UNBLOCK, &chldset, NULL);
+
+ return rc;
+ }
}