Merge "logwrapper: open child_ptty in child process and remove ignore_int_quit"
am: 7d91385cf5
Change-Id: I80953248846f9869127d04751307265110e99a8a
diff --git a/logwrapper/include/logwrap/logwrap.h b/logwrapper/include/logwrap/logwrap.h
index d3538b3..b1f6410 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 <stdbool.h>
#include <stdint.h>
@@ -26,12 +25,6 @@
/*
* 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 @@
* 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 @@
* 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 @@
#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 @@
}
__END_DECLS
-
-#endif /* __LIBS_LOGWRAP_H */
diff --git a/logwrapper/logwrap.c b/logwrapper/logwrap.c
index 8621993..0314f37 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 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 @@
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 @@
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 @@
}
}
- 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 @@
if (ret > 0) {
found_child = true;
}
+
+ if (forward_signals) {
+ if (found_child) {
+ restore_signal_handlers();
+ }
+ unblock_signals(&oldset);
+ }
}
}
@@ -472,21 +540,14 @@
}
}
-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 @@
}
/* 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 @@
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 @@
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 33454c6..e786609 100644
--- a/logwrapper/logwrapper.c
+++ b/logwrapper/logwrapper.c
@@ -79,8 +79,7 @@
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);