From ad045176238549c3267fff3e8c0014dd5e9ffbdf Mon Sep 17 00:00:00 2001 From: Roy Marples Date: Sat, 2 Feb 2008 00:17:35 +0000 Subject: Block signals to avoid fork /signal races. --- TODO | 2 +- src/includes/rc-misc.h | 3 +- src/librc/librc.c | 33 ++++++++++- src/rc/rc-misc.c | 7 +-- src/rc/rc-plugin.c | 158 ++++++++++++++++++++++++++++--------------------- src/rc/rc.c | 16 ++--- src/rc/runscript.c | 2 - 7 files changed, 136 insertions(+), 85 deletions(-) diff --git a/TODO b/TODO index a3cdeff..d04785b 100644 --- a/TODO +++ b/TODO @@ -1,3 +1,3 @@ -- stop using signal() and convert everything to sigaction() +- ensure all forks block, restore and unblock signals. needs review - add support somehow for optional translations diff --git a/src/includes/rc-misc.h b/src/includes/rc-misc.h index 946754e..1ffe48d 100644 --- a/src/includes/rc-misc.h +++ b/src/includes/rc-misc.h @@ -34,7 +34,6 @@ #include #include -#include #include #include @@ -134,7 +133,7 @@ bool rc_conf_yesno (const char *var); char **env_filter (void); char **env_config (void); bool service_plugable (const char *service); -void signal_setup (int sig, void (*handler)(int)); +int signal_setup (int sig, void (*handler)(int)); /* basename_c never modifies the argument. As such, if there is a trailing * slash then an empty string is returned. */ diff --git a/src/librc/librc.c b/src/librc/librc.c index dbbdbb5..1cd3eec 100644 --- a/src/librc/librc.c +++ b/src/librc/librc.c @@ -32,6 +32,7 @@ const char librc_copyright[] = "Copyright (c) 2007-2008 Roy Marples"; #include "librc.h" +#include #define SOFTLEVEL RC_SVCDIR "/softlevel" @@ -575,6 +576,10 @@ static pid_t _exec_service (const char *service, const char *arg) char *file; char *fifo; pid_t pid = -1; + sigset_t empty; + sigset_t full; + sigset_t old; + struct sigaction sa; file = rc_service_resolve (service); if (! exists (file)) { @@ -593,13 +598,37 @@ static pid_t _exec_service (const char *service, const char *arg) return (-1); } - if ((pid = vfork ()) == 0) { + /* We need to block signals until we have forked */ + memset (&sa, 0, sizeof (sa)); + sa.sa_handler = SIG_DFL; + sigemptyset (&sa.sa_mask); + sigemptyset (&empty); + sigfillset (&full); + sigprocmask (SIG_SETMASK, &full, &old); + + if ((pid = fork ()) == 0) { + /* Restore default handlers */ + sigaction (SIGCHLD, &sa, NULL); + sigaction (SIGHUP, &sa, NULL); + sigaction (SIGINT, &sa, NULL); + sigaction (SIGQUIT, &sa, NULL); + sigaction (SIGTERM, &sa, NULL); + sigaction (SIGUSR1, &sa, NULL); + sigaction (SIGWINCH, &sa, NULL); + + /* Unmask signals */ + sigprocmask (SIG_SETMASK, &empty, NULL); + + /* Safe to run now */ execl (file, file, arg, (char *) NULL); - fprintf (stderr, "unable to exec `%s': %s\n", file, strerror (errno)); + fprintf (stderr, "unable to exec `%s': %s\n", + file, strerror (errno)); unlink (fifo); _exit (EXIT_FAILURE); } + sigprocmask (SIG_SETMASK, &old, NULL); + free (fifo); free (file); diff --git a/src/rc/rc-misc.c b/src/rc/rc-misc.c index 50101b4..59a5261 100644 --- a/src/rc/rc-misc.c +++ b/src/rc/rc-misc.c @@ -432,13 +432,12 @@ bool service_plugable (const char *service) return (allow); } -void signal_setup (int sig, void (*handler)(int)) +int signal_setup (int sig, void (*handler)(int)) { struct sigaction sa; memset (&sa, 0, sizeof (sa)); - sa.sa_handler = handler; sigemptyset (&sa.sa_mask); - if (sigaction (sig, &sa, NULL) == -1) - eerrorx ("sigaction: %s", strerror (errno)); + sa.sa_handler = handler; + return (sigaction (sig, &sa, NULL)); } diff --git a/src/rc/rc-plugin.c b/src/rc/rc-plugin.c index 29bd43c..e4b4e0e 100644 --- a/src/rc/rc-plugin.c +++ b/src/rc/rc-plugin.c @@ -143,84 +143,110 @@ int rc_waitpid (pid_t pid) void rc_plugin_run (rc_hook_t hook, const char *value) { plugin_t *plugin = plugins; + struct sigaction sa; + sigset_t empty; + sigset_t full; + sigset_t old; /* Don't run plugins if we're in one */ if (rc_in_plugin) return; + /* We need to block signals until we have forked */ + memset (&sa, 0, sizeof (sa)); + sa.sa_handler = SIG_DFL; + sigemptyset (&sa.sa_mask); + sigemptyset (&empty); + sigfillset (&full); + while (plugin) { - if (plugin->hook) { - int i; - int flags; - int pfd[2]; - pid_t pid; - - /* We create a pipe so that plugins can affect our environment - * vars, which in turn influence our scripts. */ - if (pipe (pfd) == -1) { - eerror ("pipe: %s", strerror (errno)); - return; - } + int i; + int flags; + int pfd[2]; + pid_t pid; + char *buffer; + char *token; + char *p; + ssize_t nr; + + if (! plugin->hook) { + plugin = plugin->next; + continue; + } - /* Stop any scripts from inheriting us. - * This is actually quite important as without this, the splash - * plugin will probably hang when running in silent mode. */ - for (i = 0; i < 2; i++) - if ((flags = fcntl (pfd[i], F_GETFD, 0)) < 0 || - fcntl (pfd[i], F_SETFD, flags | FD_CLOEXEC) < 0) - eerror ("fcntl: %s", strerror (errno)); - - /* We run the plugin in a new process so we never crash - * or otherwise affected by it */ - if ((pid = fork ()) == -1) { - eerror ("fork: %s", strerror (errno)); - return; - } + /* We create a pipe so that plugins can affect our environment + * vars, which in turn influence our scripts. */ + if (pipe (pfd) == -1) { + eerror ("pipe: %s", strerror (errno)); + return; + } - if (pid == 0) { - int retval; - - rc_in_plugin = true; - close (pfd[0]); - rc_environ_fd = fdopen (pfd[1], "w"); - retval = plugin->hook (hook, value); - fclose (rc_environ_fd); - rc_environ_fd = NULL; - - /* Just in case the plugin sets this to false */ - rc_in_plugin = true; - exit (retval); - } else { - char *buffer; - char *token; - char *p; - ssize_t nr; - - close (pfd[1]); - buffer = xmalloc (sizeof (char) * BUFSIZ); - memset (buffer, 0, BUFSIZ); - - while ((nr = read (pfd[0], buffer, BUFSIZ)) > 0) { - p = buffer; - while (*p && p - buffer < nr) { - token = strsep (&p, "="); - if (token) { - unsetenv (token); - if (*p) { - setenv (token, p, 1); - p += strlen (p) + 1; - } else - p++; - } - } - } + /* Stop any scripts from inheriting us. + * This is actually quite important as without this, the splash + * plugin will probably hang when running in silent mode. */ + for (i = 0; i < 2; i++) + if ((flags = fcntl (pfd[i], F_GETFD, 0)) < 0 || + fcntl (pfd[i], F_SETFD, flags | FD_CLOEXEC) < 0) + eerror ("fcntl: %s", strerror (errno)); + + sigprocmask (SIG_SETMASK, &full, &old); + + /* We run the plugin in a new process so we never crash + * or otherwise affected by it */ + if ((pid = fork ()) == -1) { + eerror ("fork: %s", strerror (errno)); + break; + } - free (buffer); - close (pfd[0]); + if (pid == 0) { + int retval; + + /* Restore default handlers */ + sigaction (SIGCHLD, &sa, NULL); + sigaction (SIGHUP, &sa, NULL); + sigaction (SIGINT, &sa, NULL); + sigaction (SIGQUIT, &sa, NULL); + sigaction (SIGTERM, &sa, NULL); + sigaction (SIGUSR1, &sa, NULL); + sigaction (SIGWINCH, &sa, NULL); + sigprocmask (SIG_SETMASK, &old, NULL); + + rc_in_plugin = true; + close (pfd[0]); + rc_environ_fd = fdopen (pfd[1], "w"); + retval = plugin->hook (hook, value); + fclose (rc_environ_fd); + rc_environ_fd = NULL; + + /* Just in case the plugin sets this to false */ + rc_in_plugin = true; + exit (retval); + } - rc_waitpid (pid); + sigprocmask (SIG_SETMASK, &old, NULL); + close (pfd[1]); + buffer = xmalloc (sizeof (char) * BUFSIZ); + memset (buffer, 0, BUFSIZ); + + while ((nr = read (pfd[0], buffer, BUFSIZ)) > 0) { + p = buffer; + while (*p && p - buffer < nr) { + token = strsep (&p, "="); + if (token) { + unsetenv (token); + if (*p) { + setenv (token, p, 1); + p += strlen (p) + 1; + } else + p++; + } } } + + free (buffer); + close (pfd[0]); + + rc_waitpid (pid); plugin = plugin->next; } } diff --git a/src/rc/rc.c b/src/rc/rc.c index f093b87..f87ddc9 100644 --- a/src/rc/rc.c +++ b/src/rc/rc.c @@ -663,6 +663,7 @@ int main (int argc, char **argv) int opt; bool parallel; int regen = 0; + pid_t pid; applet = basename_c (argv[0]); atexit (cleanup); @@ -994,7 +995,7 @@ int main (int argc, char **argv) /* We always stop the service when in these runlevels */ if (going_down) { - pid_t pid = rc_service_stop (service); + pid = rc_service_stop (service); if (pid > 0 && ! parallel) rc_waitpid (pid); continue; @@ -1060,11 +1061,10 @@ int main (int argc, char **argv) /* After all that we can finally stop the blighter! */ if (! found) { - pid_t pid; + pid = rc_service_stop (service); - if ((pid = rc_service_stop (service)) > 0) { + if (pid > 0) { add_pid (pid); - if (! parallel) { rc_waitpid (pid); remove_pid (pid); @@ -1136,9 +1136,7 @@ int main (int argc, char **argv) #endif STRLIST_FOREACH (start_services, service, i) { - if (rc_service_state (service) & RC_SERVICE_STOPPED) { - pid_t pid; - + if (rc_service_state (service) & RC_SERVICE_STOPPED) { if (! interactive) interactive = want_interactive (); @@ -1160,8 +1158,10 @@ interactive_option: } } + pid = rc_service_start (service); + /* Remember the pid if we're running in parallel */ - if ((pid = rc_service_start (service)) > 0) { + if (pid > 0) { add_pid (pid); if (! parallel) { diff --git a/src/rc/runscript.c b/src/rc/runscript.c index dfabb86..83f2086 100644 --- a/src/rc/runscript.c +++ b/src/rc/runscript.c @@ -432,8 +432,6 @@ static bool svc_exec (const char *arg1, const char *arg2) eerrorx ("%s: vfork: %s", service, strerror (errno)); if (service_pid == 0) { if (slave_tty >= 0) { - /* Hmmm, this shouldn't work in a vfork, but it does which is - * good for us */ close (master_tty); dup2 (slave_tty, 1); -- cgit v1.2.3