commit 691a39f31465c3118a31d3c8be580cbbaf603578 Author: elly elly@leptoquark.net Date: Fri Jun 21 12:01:18 2013 -0400

Fix subprocess watching.

Currently, the subprocess watching code polls with a delay between attempts. Instead, introduce wait_with_timeout() and use it, simplifying this code.

Signed-off-by: Elly Fong-Jones elly@leptoquark.net --- CHANGELOG | 1 + etc/tlsdated.conf | 3 +- man/tlsdated.conf.5 | 6 ++-- src/tlsdate.h | 6 ++-- src/tlsdated-unittest.c | 14 +++------ src/tlsdated.c | 77 ++++++++++++++++++----------------------------- src/util.c | 31 +++++++++++++++++++ src/util.h | 4 +++ 8 files changed, 75 insertions(+), 67 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG index fbe7a83..c408a15 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -15,6 +15,7 @@ Add Mac OS X 10.8.3 support Extensive setup/install documentation is now present in INSTALL for most OSes Add DragonFly BSD 3.3 support + Refactored subprocess watching. 0.0.6 Mon 18 Feb, 2013 Ensure that tlsdate compiles with g++ by explicit casting rather than implicit casting by whatever compiler is compiling tlsdate. diff --git a/etc/tlsdated.conf b/etc/tlsdated.conf index 9f9b43e..c6dbbf0 100644 --- a/etc/tlsdated.conf +++ b/etc/tlsdated.conf @@ -12,8 +12,7 @@ should-netlink yes should-save-disk yes should-sync-hwclock yes steady-state-interval 86400 -subprocess-tries 10 -subprocess-wait-between-tries 3 +subprocess-timeout 30 verbose yes wait-between-tries 10

diff --git a/man/tlsdated.conf.5 b/man/tlsdated.conf.5 index 13fb153..79ed27a 100644 --- a/man/tlsdated.conf.5 +++ b/man/tlsdated.conf.5 @@ -42,10 +42,8 @@ at exit. If enabled, set the hwclock to the fetched time. .IP "steady-state-interval [int]" Check at least once this many seconds when in steady state. -.IP "subprocess-tries [int]" -How many times to try waiting for the subprocess. -.IP "subprocess-wait-between-tries [int]" -How many seconds to wait between each attempt to wait for the subprocess. +.IP "subprocess-timeout [int]" +How many seconds to wait for the subprocess to exit. .IP "verbose [bool]" If enabled, tlsdated will be annoyingly verbose in syslog and on stdout. .IP "wait-between-tries [int]" diff --git a/src/tlsdate.h b/src/tlsdate.h index 5b24980..978396d 100644 --- a/src/tlsdate.h +++ b/src/tlsdate.h @@ -33,8 +33,7 @@ /* tlsdated magic numbers */ #define MAX_TRIES 10 #define WAIT_BETWEEN_TRIES 10 -#define SUBPROCESS_TRIES 10 -#define SUBPROCESS_WAIT_BETWEEN_TRIES 3 +#define SUBPROCESS_TIMEOUT 30 #define STEADY_STATE_INTERVAL 86400 #define DEFAULT_SYNC_HWCLOCK 1 #define DEFAULT_LOAD_FROM_DISK 1 @@ -67,8 +66,7 @@ struct opts { int max_tries; int min_steady_state_interval; int wait_between_tries; - int subprocess_tries; - int subprocess_wait_between_tries; + int subprocess_timeout; int steady_state_interval; const char *base_path; char **base_argv; diff --git a/src/tlsdated-unittest.c b/src/tlsdated-unittest.c index cb30313..5baca13 100644 --- a/src/tlsdated-unittest.c +++ b/src/tlsdated-unittest.c @@ -118,8 +118,7 @@ TEST(tlsdate_tests) { memset(&opts, 0, sizeof(opts)); opts.sources = &source; opts.base_argv = args; - opts.subprocess_tries = 2; - opts.subprocess_wait_between_tries = 1; + opts.subprocess_timeout = 1; extern char **environ; EXPECT_EQ(1, tlsdate(&opts, environ)); args[0] = "/bin/false"; @@ -129,7 +128,7 @@ TEST(tlsdate_tests) { args[0] = "src/test/sleep-wrap"; args[1] = "3"; EXPECT_EQ(-1, tlsdate(&opts, environ)); - opts.subprocess_wait_between_tries = 5; + opts.subprocess_timeout = 5; EXPECT_EQ(0, tlsdate(&opts, environ)); }

@@ -167,8 +166,7 @@ TEST(rotate_hosts) { memset(&opts, 0, sizeof(opts)); opts.sources = &s1; opts.base_argv = args; - opts.subprocess_tries = 2; - opts.subprocess_wait_between_tries = 1; + opts.subprocess_timeout = 2; extern char **environ; EXPECT_EQ(1, tlsdate(&opts, environ)); EXPECT_EQ(2, tlsdate(&opts, environ)); @@ -188,8 +186,7 @@ TEST(proxy_override) { memset(&opts, 0, sizeof(opts)); opts.sources = &s1; opts.base_argv = args; - opts.subprocess_tries = 2; - opts.subprocess_wait_between_tries = 1; + opts.subprocess_timeout = 2; extern char **environ; EXPECT_EQ(1, tlsdate(&opts, environ)); s1.proxy = "socks5://bad.proxy"; @@ -210,8 +207,7 @@ TEST(tlsdate_args) { memset(&opts, 0, sizeof(opts)); opts.sources = &s1; opts.base_argv = args; - opts.subprocess_tries = 2; - opts.subprocess_wait_between_tries = 1; + opts.subprocess_timeout = 2; opts.leap = 1; verbose = 1; EXPECT_EQ(9, tlsdate(&opts, environ)); diff --git a/src/tlsdated.c b/src/tlsdated.c index ef9fe68..7816223 100644 --- a/src/tlsdated.c +++ b/src/tlsdated.c @@ -106,43 +106,35 @@ int tlsdate (struct opts *opts, char *envp[]) { pid_t pid; + pid_t exited; + int status; + build_argv(opts); - - if ((pid = fork ()) > 0) - { - /* - * We launched tlsdate; wait for up to kMaxTries intervals of - * kWaitBetweenTries for it to exit, then kill it if it still - * hasn't. - */ - int status = -1; - int i = 0; - for (i = 0; i < opts->subprocess_tries; ++i) + + pid = fork(); + if (pid < 0) { - info ("wait for child attempt %d", i); - if (waitpid (-1, &status, WNOHANG) > 0) - break; - sleep (opts->subprocess_wait_between_tries); + pinfo("fork() failed"); + return -1; } - if (i == opts->subprocess_tries) + else if (pid == 0) { - error ("child hung?"); - kill (pid, SIGKILL); - /* still have to wait() so we don't leak the child. */ - wait (&status); + execve(opts->argv[0], opts->argv, envp); + pinfo("execve() failed"); + exit(1); + } + + exited = wait_with_timeout(&status, opts->subprocess_timeout); + info("child %d exited with %d", exited, status); + + if (exited == -ETIMEDOUT) + { + kill(pid, SIGKILL); + wait(&status); return -1; } - info ("child exited with %d", status); - return WIFEXITED (status) ? WEXITSTATUS (status) : -1; - } - else if (pid < 0) - { - pinfo ("fork() failed"); - return -1; - } - execve (opts->argv[0], opts->argv, envp); - pinfo ("execve() failed"); - exit (1); + + return WIFEXITED(status) ? WEXITSTATUS(status) : -1; }

/* @@ -394,8 +386,7 @@ set_conf_defaults(struct opts *opts) opts->max_tries = MAX_TRIES; opts->min_steady_state_interval = STEADY_STATE_INTERVAL; opts->wait_between_tries = WAIT_BETWEEN_TRIES; - opts->subprocess_tries = SUBPROCESS_TRIES; - opts->subprocess_wait_between_tries = SUBPROCESS_WAIT_BETWEEN_TRIES; + opts->subprocess_timeout = SUBPROCESS_TIMEOUT; opts->steady_state_interval = STEADY_STATE_INTERVAL; opts->base_path = kCacheDir; opts->base_argv = kDefaultArgv; @@ -418,7 +409,7 @@ parse_argv(struct opts *opts, int argc, char *argv[]) { int opt;

- while ((opt = getopt (argc, argv, "hwrpt:d:T:D:c:a:lsvm:j:f:x:")) != -1) + while ((opt = getopt (argc, argv, "hwrpt:d:c:a:lsvm:j:f:x:")) != -1) { switch (opt) { @@ -437,12 +428,6 @@ parse_argv(struct opts *opts, int argc, char *argv[]) case 'd': opts->wait_between_tries = atoi (optarg); break; - case 'T': - opts->subprocess_tries = atoi (optarg); - break; - case 'D': - opts->subprocess_wait_between_tries = atoi (optarg); - break; case 'c': opts->base_path = optarg; break; @@ -572,10 +557,8 @@ load_conf(struct opts *opts) opts->min_steady_state_interval = atoi (e->value); } else if (!strcmp (e->key, "wait-between-tries") && e->value) { opts->wait_between_tries = atoi (e->value); - } else if (!strcmp (e->key, "subprocess-tries") && e->value) { - opts->subprocess_tries = atoi (e->value); - } else if (!strcmp (e->key, "subprocess-wait-between-tries") && e->value) { - opts->subprocess_wait_between_tries = atoi (e->value); + } else if (!strcmp (e->key, "subprocess-timeout") && e->value) { + opts->subprocess_timeout = atoi (e->value); } else if (!strcmp (e->key, "steady-state-interval") && e->value) { opts->steady_state_interval = atoi (e->value); } else if (!strcmp (e->key, "base-path") && e->value) { @@ -611,10 +594,8 @@ check_conf(struct opts *opts) fatal ("-t argument must be nonzero"); if (!opts->wait_between_tries) fatal ("-d argument must be nonzero"); - if (!opts->subprocess_tries) - fatal ("-T argument must be nonzero"); - if (!opts->subprocess_wait_between_tries) - fatal ("-D argument must be nonzero"); + if (!opts->subprocess_timeout) + fatal ("subprocess timeout must be nonzero"); if (!opts->steady_state_interval) fatal ("-a argument must be nonzero"); if (snprintf (timestamp_path, sizeof (timestamp_path), "%s/timestamp", diff --git a/src/util.c b/src/util.c index 68f3a98..30a0f44 100644 --- a/src/util.c +++ b/src/util.c @@ -9,9 +9,11 @@

#include <grp.h> #include <pwd.h> +#include <signal.h> #include <stdarg.h> #include <stdio.h> #include <sys/types.h> +#include <sys/wait.h> #include <syslog.h> #include <unistd.h>

@@ -101,3 +103,32 @@ drop_privs_to (const char *user, const char *group) #endif }

+pid_t +wait_with_timeout(int *status, int timeout_secs) +{ + int st = 0; + pid_t exited; + /* synthesize waiting with a timeout by using a helper process. We + * launch a child process that will exit in |timeout_secs|, guaranteeing + * that wait() will return by then. */ + pid_t helper = fork(); + if (helper < 0) + return helper; + if (helper == 0) + { + sleep(timeout_secs); + exit(0); + } + + /* use temporary status to avoid touching it if we do ETIMEDOUT */ + exited = wait(&st); + if (exited == helper) + /* helper exited before any other child did */ + return -ETIMEDOUT; + + /* a real child process exited - don't leak the helper */ + kill(helper, SIGKILL); + waitpid(helper, NULL, 0); + *status = st; + return exited; +} diff --git a/src/util.h b/src/util.h index bfd61dd..2632933 100644 --- a/src/util.h +++ b/src/util.h @@ -37,4 +37,8 @@ static inline int min(int x, int y) { return x < y ? x : y; }

void drop_privs_to (const char *user, const char *group);

+/* like wait(), but with a timeout. Returns ordinary fork() error codes, or + * ETIMEDOUT. */ +pid_t wait_with_timeout(int *status, int timeout_secs); + #endif /* !UTIL_H */