[tor-commits] [tlsdate/debian-master] Fix subprocess watching.

ioerror at torproject.org ioerror at torproject.org
Thu Oct 31 10:51:31 UTC 2013


commit 691a39f31465c3118a31d3c8be580cbbaf603578
Author: elly <elly at 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 at 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 */





More information about the tor-commits mailing list