diff options
author | Rom Lemarchand <romlem@google.com> | 2013-01-09 21:31:25 -0800 |
---|---|---|
committer | Rom Lemarchand <romlem@google.com> | 2013-01-14 11:11:58 -0800 |
commit | b58a82295529e775fbb900ecfb2d9104b2dafdc1 (patch) | |
tree | b1eda8cc7f4ba541ed6bc539609a245ae0f592e9 | |
parent | 1b325d196857d29fe5f022f2c574d3625e0adc08 (diff) | |
download | system_core-b58a82295529e775fbb900ecfb2d9104b2dafdc1.tar.gz system_core-b58a82295529e775fbb900ecfb2d9104b2dafdc1.tar.bz2 system_core-b58a82295529e775fbb900ecfb2d9104b2dafdc1.zip |
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
-rw-r--r-- | logwrapper/include/logwrap/logwrap.h | 24 | ||||
-rw-r--r-- | logwrapper/logwrap.c | 152 |
2 files changed, 131 insertions, 45 deletions
diff --git a/logwrapper/include/logwrap/logwrap.h b/logwrapper/include/logwrap/logwrap.h index a58f23820..722dda2ac 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 302a739dd..c2b36be29 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 @@ void parent(const char *tag, int parent_read, int *chld_sts) { 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 @@ void child(int argc, char* argv[]) { } } -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 @@ int logwrap(int argc, char* argv[], int *chld_sts) { 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 @@ int logwrap(int argc, char* argv[], int *chld_sts) { 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; + } } |