diff --git a/logwrapper/include/logwrap/logwrap.h b/logwrapper/include/logwrap/logwrap.h index d3538b30c..b1f64102a 100644 --- a/logwrapper/include/logwrap/logwrap.h +++ b/logwrapper/include/logwrap/logwrap.h @@ -15,8 +15,7 @@ * limitations under the License. */ -#ifndef __LIBS_LOGWRAP_H -#define __LIBS_LOGWRAP_H +#pragma once #include #include @@ -26,12 +25,6 @@ __BEGIN_DECLS /* * 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 @@ -40,10 +33,10 @@ __BEGIN_DECLS * status: the equivalent child status as populated by wait(status). This * value is only valid when logwrap successfully completes. If NULL * the return value of the child will be the function's return value. - * ignore_int_quit: set to true if you want to completely ignore SIGINT and - * SIGQUIT while logwrap is running. This may force the end-user to - * send a signal twice to signal the caller (once for the child, and - * once for the caller) + * forward_signals: set to true if you want to forward SIGINT, SIGQUIT, and + * SIGHUP to the child process, while it is running. You likely do + * not need to use this; it is primarily for the logwrapper + * executable itself. * log_target: Specify where to log the output of the child, either LOG_NONE, * LOG_ALOG (for the Android system log), LOG_KLOG (for the kernel * log), or LOG_FILE (and you need to specify a pathname in the @@ -54,8 +47,6 @@ __BEGIN_DECLS * the specified log until the child has exited. * file_path: if log_target has the LOG_FILE bit set, then this parameter * must be set to the pathname of the file to log to. - * unused_opts: currently unused. - * unused_opts_len: currently unused. * * Return value: * 0 when logwrap successfully run the child process and captured its status @@ -71,10 +62,18 @@ __BEGIN_DECLS #define LOG_KLOG 2 #define LOG_FILE 4 -// TODO: Remove unused_opts / unused_opts_len in a followup change. -int android_fork_execvp_ext(int argc, char* argv[], int *status, bool ignore_int_quit, - int log_target, bool abbreviated, char *file_path, void* unused_opts, - int unused_opts_len); +int android_fork_execvp_ext2(int argc, char* argv[], int* status, bool forward_signals, + int log_target, bool abbreviated, char* file_path); + +// TODO: Actually deprecate this and the below. +static inline int android_fork_execvp_ext(int argc, char* argv[], int* status, bool ignore_int_quit, + int log_target, bool abbreviated, char* file_path, + void* unused_opts, int unused_opts_len) { + (void)ignore_int_quit; + (void)unused_opts; + (void)unused_opts_len; + return android_fork_execvp_ext2(argc, argv, status, false, log_target, abbreviated, file_path); +} /* Similar to above, except abbreviated logging is not available, and if logwrap * is true, logging is to the Android system log, and if false, there is no @@ -89,5 +88,3 @@ static inline int android_fork_execvp(int argc, char* argv[], int *status, } __END_DECLS - -#endif /* __LIBS_LOGWRAP_H */ diff --git a/logwrapper/logwrap.c b/logwrapper/logwrap.c index 86219936d..0314f374a 100644 --- a/logwrapper/logwrap.c +++ b/logwrapper/logwrap.c @@ -36,6 +36,11 @@ #define MIN(a,b) (((a)<(b))?(a):(b)) static pthread_mutex_t fd_mutex = PTHREAD_MUTEX_INITIALIZER; +// Protected by fd_mutex. These signals must be blocked while modifying as well. +static pid_t child_pid; +static struct sigaction old_int; +static struct sigaction old_quit; +static struct sigaction old_hup; #define ERROR(fmt, args...) \ do { \ @@ -289,8 +294,47 @@ static void print_abbr_buf(struct log_info *log_info) { } } -static int parent(const char *tag, int parent_read, pid_t pid, - int *chld_sts, int log_target, bool abbreviated, char *file_path) { +static void signal_handler(int signal_num); + +static void block_signals(sigset_t* oldset) { + sigset_t blockset; + + sigemptyset(&blockset); + sigaddset(&blockset, SIGINT); + sigaddset(&blockset, SIGQUIT); + sigaddset(&blockset, SIGHUP); + pthread_sigmask(SIG_BLOCK, &blockset, oldset); +} + +static void unblock_signals(sigset_t* oldset) { + pthread_sigmask(SIG_SETMASK, oldset, NULL); +} + +static void setup_signal_handlers(pid_t pid) { + struct sigaction handler = {.sa_handler = signal_handler}; + + child_pid = pid; + sigaction(SIGINT, &handler, &old_int); + sigaction(SIGQUIT, &handler, &old_quit); + sigaction(SIGHUP, &handler, &old_hup); +} + +static void restore_signal_handlers() { + sigaction(SIGINT, &old_int, NULL); + sigaction(SIGQUIT, &old_quit, NULL); + sigaction(SIGHUP, &old_hup, NULL); + child_pid = 0; +} + +static void signal_handler(int signal_num) { + if (child_pid == 0 || kill(child_pid, signal_num) != 0) { + restore_signal_handlers(); + raise(signal_num); + } +} + +static int parent(const char* tag, int parent_read, pid_t pid, int* chld_sts, int log_target, + bool abbreviated, char* file_path, bool forward_signals) { int status = 0; char buffer[4096]; struct pollfd poll_fds[] = { @@ -308,6 +352,11 @@ static int parent(const char *tag, int parent_read, pid_t pid, int b = 0; // end index of unprocessed data int sz; bool found_child = false; + // There is a very small chance that opening child_ptty in the child will fail, but in this case + // POLLHUP will not be generated below. Therefore, we use a 1 second timeout for poll() until + // we receive a message from child_ptty. If this times out, we call waitpid() with WNOHANG to + // check the status of the child process and exit appropriately if it has terminated. + bool received_messages = false; char tmpbuf[256]; log_info.btag = basename(tag); @@ -347,13 +396,15 @@ static int parent(const char *tag, int parent_read, pid_t pid, log_info.abbreviated = abbreviated; while (!found_child) { - if (TEMP_FAILURE_RETRY(poll(poll_fds, ARRAY_SIZE(poll_fds), -1)) < 0) { + int timeout = received_messages ? -1 : 1000; + if (TEMP_FAILURE_RETRY(poll(poll_fds, ARRAY_SIZE(poll_fds), timeout)) < 0) { ERROR("poll failed\n"); rc = -1; goto err_poll; } if (poll_fds[0].revents & POLLIN) { + received_messages = true; sz = TEMP_FAILURE_RETRY( read(parent_read, &buffer[b], sizeof(buffer) - 1 - b)); @@ -396,10 +447,20 @@ static int parent(const char *tag, int parent_read, pid_t pid, } } - if (poll_fds[0].revents & POLLHUP) { + if (!received_messages || (poll_fds[0].revents & POLLHUP)) { int ret; + sigset_t oldset; - ret = TEMP_FAILURE_RETRY(waitpid(pid, &status, 0)); + if (forward_signals) { + // Our signal handlers forward these signals to 'child_pid', but waitpid() may reap + // the child, so we must block these signals until we either 1) conclude that the + // child is still running or 2) determine the child has been reaped and we have + // reset the signals to their original disposition. + block_signals(&oldset); + } + + int flags = (poll_fds[0].revents & POLLHUP) ? 0 : WNOHANG; + ret = TEMP_FAILURE_RETRY(waitpid(pid, &status, flags)); if (ret < 0) { rc = errno; ALOG(LOG_ERROR, "logwrap", "waitpid failed with %s\n", strerror(errno)); @@ -408,6 +469,13 @@ static int parent(const char *tag, int parent_read, pid_t pid, if (ret > 0) { found_child = true; } + + if (forward_signals) { + if (found_child) { + restore_signal_handlers(); + } + unblock_signals(&oldset); + } } } @@ -472,21 +540,14 @@ static void child(int argc, char* argv[]) { } } -int android_fork_execvp_ext(int argc, char* argv[], int *status, bool ignore_int_quit, - int log_target, bool abbreviated, char *file_path, - void *unused_opts, int unused_opts_len) { +int android_fork_execvp_ext2(int argc, char* argv[], int* status, bool forward_signals, + int log_target, bool abbreviated, char* file_path) { pid_t pid; int parent_ptty; - int child_ptty; - struct sigaction intact; - struct sigaction quitact; sigset_t blockset; sigset_t oldset; int rc = 0; - LOG_ALWAYS_FATAL_IF(unused_opts != NULL); - LOG_ALWAYS_FATAL_IF(unused_opts_len != 0); - rc = pthread_mutex_lock(&fd_mutex); if (rc) { ERROR("failed to lock signal_fd mutex\n"); @@ -494,7 +555,7 @@ int android_fork_execvp_ext(int argc, char* argv[], int *status, bool ignore_int } /* Use ptty instead of socketpair so that STDOUT is not buffered */ - parent_ptty = TEMP_FAILURE_RETRY(open("/dev/ptmx", O_RDWR)); + parent_ptty = TEMP_FAILURE_RETRY(posix_openpt(O_RDWR | O_CLOEXEC)); if (parent_ptty < 0) { ERROR("Cannot create parent ptty\n"); rc = -1; @@ -509,27 +570,26 @@ int android_fork_execvp_ext(int argc, char* argv[], int *status, bool ignore_int goto err_ptty; } - child_ptty = TEMP_FAILURE_RETRY(open(child_devname, O_RDWR)); - if (child_ptty < 0) { - ERROR("Cannot open child_ptty\n"); - rc = -1; - goto err_child_ptty; + if (forward_signals) { + // Block these signals until we have the child pid and our signal handlers set up. + block_signals(&oldset); } - sigemptyset(&blockset); - sigaddset(&blockset, SIGINT); - sigaddset(&blockset, SIGQUIT); - pthread_sigmask(SIG_BLOCK, &blockset, &oldset); - pid = fork(); if (pid < 0) { - close(child_ptty); ERROR("Failed to fork\n"); rc = -1; goto err_fork; } else if (pid == 0) { pthread_mutex_unlock(&fd_mutex); - pthread_sigmask(SIG_SETMASK, &oldset, NULL); + unblock_signals(&oldset); + + setsid(); + + int child_ptty = TEMP_FAILURE_RETRY(open(child_devname, O_RDWR | O_CLOEXEC)); + if (child_ptty < 0) { + FATAL_CHILD("Cannot open child_ptty: %s\n", strerror(errno)); + } close(parent_ptty); dup2(child_ptty, 1); @@ -538,27 +598,23 @@ int android_fork_execvp_ext(int argc, char* argv[], int *status, bool ignore_int child(argc, argv); } else { - close(child_ptty); - if (ignore_int_quit) { - struct sigaction ignact; - - memset(&ignact, 0, sizeof(ignact)); - ignact.sa_handler = SIG_IGN; - sigaction(SIGINT, &ignact, &intact); - sigaction(SIGQUIT, &ignact, &quitact); + if (forward_signals) { + setup_signal_handlers(pid); + unblock_signals(&oldset); } - rc = parent(argv[0], parent_ptty, pid, status, log_target, - abbreviated, file_path); + rc = parent(argv[0], parent_ptty, pid, status, log_target, abbreviated, file_path, + forward_signals); + + if (forward_signals) { + restore_signal_handlers(); + } } - if (ignore_int_quit) { - sigaction(SIGINT, &intact, NULL); - sigaction(SIGQUIT, &quitact, NULL); - } err_fork: - pthread_sigmask(SIG_SETMASK, &oldset, NULL); -err_child_ptty: + if (forward_signals) { + unblock_signals(&oldset); + } err_ptty: close(parent_ptty); err_open: diff --git a/logwrapper/logwrapper.c b/logwrapper/logwrapper.c index 33454c6a6..e78660908 100644 --- a/logwrapper/logwrapper.c +++ b/logwrapper/logwrapper.c @@ -79,8 +79,7 @@ int main(int argc, char* argv[]) { usage(); } - rc = android_fork_execvp_ext(argc, &argv[0], &status, true, - log_target, abbreviated, NULL, NULL, 0); + rc = android_fork_execvp_ext2(argc, &argv[0], &status, true, log_target, abbreviated, NULL); if (!rc) { if (WIFEXITED(status)) rc = WEXITSTATUS(status);