Always clean up PID files on exit

After a SIGHUP or SIGTERM when not running a command, k5start and
krenew now clean up their PID files, if any, before exiting.

diff --git a/NEWS b/NEWS
index 37eb101019898e81d65ea59bbad004a70c017f18..1defdefb015c7335299efde464aa8635fb376bb1 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,9 @@ kstart 4.1 (unreleased)
     renew the ticket.  This is useful when continuing to run the command
     without a valid ticket would be pointless.
 
+    After a SIGHUP or SIGTERM when not running a command, k5start and
+    krenew now clean up their PID files, if any, before exiting.
+
 kstart 4.0 (2011-12-29)
 
     Remove k4start from the distribution.  I no longer have a Kerberos v4

diff --git a/TODO b/TODO
index 495a0501520c9e265ffa2ebc8b0affe134b3169b..502573650a389d16233c0f4f04c90bb91e2e925a 100644 (file)
--- a/TODO
+++ b/TODO
@@ -6,9 +6,6 @@ General:
    the normal period; instead, shorten the wakeup period and keep retrying
    until the error resolves itself.
 
- * Clean up the PID files and any temporary ticket cache on receipt of
-   SIGHUP or SIGTERM rather than just exiting.
-
 k5start:
 
  * Attempt to renew the ticket before prompting the user for a new

diff --git a/framework.c b/framework.c
index 49e37c0fd5f5d1f19b43d92f38b4a38369a8d414..2dce420f47c29b865aa7bcb09f204e61b8cb80e0 100644 (file)
--- a/framework.c
+++ b/framework.c
@@ -58,6 +58,13 @@
  */
 static volatile sig_atomic_t alarm_signaled = 0;
 
+/*
+ * Set when the program receives SIGHUP or SIGTERM to do cleanup and exit.
+ * These signal handlers are only used when we're not running a command, since
+ * running a command provides its own signal handlers.
+ */
+static volatile sig_atomic_t exit_signaled = 0;
+
 
 /*
  * Convert from a string to a number, checking errors, and return -1 on any
@@ -88,6 +95,17 @@ alarm_handler(int s UNUSED)
 }
 
 
+/*
+ * Signal handler for SIGHUP and SIGTERM.  Just sets the global sentinel
+ * variable.
+ */
+static void
+exit_handler(int s UNUSED)
+{
+    exit_signaled = 1;
+}
+
+
 /*
  * Get the principal name for the krbtgt ticket for the local realm.  The
  * caller is responsible for freeing the principal.  Takes an existing
@@ -214,6 +232,24 @@ write_pidfile(const char *path, pid_t pid)
 }
 
 
+/*
+ * Add a signal handler, exiting if there was a failure.
+ */
+static void
+add_handler(krb5_context ctx, struct config *config, void (*handler)(int),
+            int sig, const char *name)
+{
+    struct sigaction sa;
+
+    memset(&sa, 0, sizeof(sa));
+    sa.sa_handler = handler;
+    if (sigaction(sig, &sa, NULL) < 0) {
+        syswarn("cannot set %s handler", name);
+        exit_cleanup(ctx, config, 1);
+    }
+}
+
+
 /*
  * The primary entry point of the framework.  Both k5start and krenew call
  * this function after setting up the options and configuration to do the real
@@ -307,14 +343,12 @@ run_framework(krb5_context ctx, struct config *config)
     /* Loop if we're running as a daemon. */
     if (config->keep_ticket > 0) {
         struct timeval timeout;
-        struct sigaction sa;
 
         code = 0;
-        memset(&sa, 0, sizeof(sa));
-        sa.sa_handler = alarm_handler;
-        if (sigaction(SIGALRM, &sa, NULL) < 0) {
-            syswarn("cannot set SIGALRM handler");
-            exit_cleanup(ctx, config, 1);
+        add_handler(ctx, config, alarm_handler, SIGALRM, "SIGALRM");
+        if (config->command == NULL) {
+            add_handler(ctx, config, exit_handler, SIGHUP, "SIGHUP");
+            add_handler(ctx, config, exit_handler, SIGTERM, "SIGTERM");
         }
         while (1) {
             if (config->command != NULL) {
@@ -331,6 +365,8 @@ run_framework(krb5_context ctx, struct config *config)
             timeout.tv_sec = config->keep_ticket * 60;
             timeout.tv_usec = 0;
             select(0, NULL, NULL, NULL, &timeout);
+            if (exit_signaled)
+                exit_cleanup(ctx, config, 0);
             code = ticket_expired(ctx, config);
             if (alarm_signaled || code != 0) {
                 code = config->auth(ctx, config, code);

diff --git a/tests/k5start/daemon-t b/tests/k5start/daemon-t
index 187cf4c473c87cbe0679479e252e3b217f231e0c..321311d08241aeff130783f2eba0b14c82733c71 100755 (executable)
--- a/tests/k5start/daemon-t
+++ b/tests/k5start/daemon-t
@@ -3,7 +3,7 @@
 # Tests for k5start daemon functionality.
 #
 # Written by Russ Allbery <rra@stanford.edu>
-# Copyright 2008, 2009, 2011
+# Copyright 2008, 2009, 2011, 2012
 #     The Board of Trustees of the Leland Stanford Junior University
 #
 # See LICENSE for licensing terms.
@@ -30,7 +30,7 @@ require "$ENV{SOURCE}/libtest.pl";
 
 # Decide whether we have the configuration to run the tests.
 if (-f "$DATA/test.keytab" and -f "$DATA/test.principal") {
-    plan tests => 72;
+    plan tests => 76;
 } else {
     plan skip_all => "no keytab configuration";
     exit 0;
@@ -83,7 +83,7 @@ like ($default, qr/^\Q$principal\E(\@\S+)?\z/,
 like ($service, qr%^krbtgt/%, ' and the right service');
 kill (15, $pid) or warn "Can't kill $pid: $!\n";
 is (waitpid ($pid, 0), $pid, ' and k5start dies after SIGTERM');
-unlink "$TMP/pid";
+ok (!-f "$TMP/pid", ' and the PID file was removed');
 
 # Try again with the -b flag.
 unlink "$TMP/krb5cc_test";
@@ -105,7 +105,8 @@ like ($service, qr%^krbtgt/%, ' and the right service');
 $pid = contents ("$TMP/pid");
 ok (kill (0, $pid), ' and the PID file is correct');
 kill (15, $pid) or warn "Can't kill $pid: $!\n";
-unlink "$TMP/pid";
+select (undef, undef, undef, 0.2);
+ok (!-f "$TMP/pid", ' and the PID file was removed');
 
 # Check that k5start keeps running if the ticket cache directory is not
 # writeable.
@@ -138,7 +139,8 @@ if (open (ERRORS, '<', "$TMP/k5start-errors")) {
 }
 unlink "$TMP/k5start-errors";
 kill (15, $pid) or warn "Can't kill $pid: $!\n";
-unlink "$TMP/pid";
+is (waitpid ($pid, 0), $pid, ' and k5start dies after SIGTERM');
+ok (!-f "$TMP/pid", ' and the PID file was removed');
 
 # If we do that again with -x, k5start should exit.
 ($out, $err, $status)

diff --git a/tests/krenew/daemon-t b/tests/krenew/daemon-t
index 0866bb16b9c38c8edee69967e76c375a22e003ea..d937d4850b3d5ca7ae37c795a60e9e8fdf1458ec 100755 (executable)
--- a/tests/krenew/daemon-t
+++ b/tests/krenew/daemon-t
@@ -41,7 +41,7 @@ if (not -f "$DATA/test.keytab" or not -f "$DATA/test.principal") {
         plan skip_all => 'cannot get renewable tickets';
         exit 0;
     }
-    plan tests => 90;
+    plan tests => 94;
 }
 
 # Start a krenew daemon and be sure it gets tickets and stays running.
@@ -96,7 +96,7 @@ ok (kill (0, $pid), ' and the PID file is correct');
 kill (15, $pid) or warn "Can't kill $pid: $!\n";
 select (undef, undef, undef, 0.5);
 ok (!kill (0, $pid), ' and it dies after SIGTERM');
-unlink "$TMP/pid";
+ok (!-f "$TMP/pid", ' and the PID file was removed');
 
 # Now try with -i.  In this case, krenew should keep running even if the
 # ticket cache disappears and be able to start refreshing it again when it
@@ -132,7 +132,7 @@ ok ($time < (stat "$TMP/krb5cc_test")[9], ' and is updated after SIGALRM');
 kill (15, $pid) or warn "Can't kill $pid: $!\n";
 select (undef, undef, undef, 0.5);
 ok (!kill (0, $pid), ' and it dies after SIGTERM');
-unlink "$TMP/pid";
+ok (!-f "$TMP/pid", ' and the PID file was removed');
 
 # Check that krenew keeps running if the ticket cache directory is not
 # writeable.
@@ -165,7 +165,8 @@ if (open (ERRORS, '<', "$TMP/krenew-errors")) {
 }
 unlink "$TMP/krenew-errors";
 kill (15, $pid) or warn "Can't kill $pid: $!\n";
-unlink "$TMP/pid";
+is (waitpid ($pid, 0), $pid, ' and it dies on SIGTERM');
+ok (!-f "$TMP/pid", ' and the PID file was removed');
 
 # If we do that again with -x, krenew should exit.
 ($out, $err, $status) = command ($KRENEW, '-xbK', 30, '-p', "$TMP/pid");