From 5e37d368b0e659d593535c254682772dce25beff Mon Sep 17 00:00:00 2001 From: Pavel Charvat Date: Thu, 9 Oct 2014 06:30:08 +0000 Subject: [PATCH] Daemon: Fixed invalid permissions and some races when closing daemons. --- ucw/daemon-ctrl.c | 119 +++++++++++++++++++++++++++------------------- ucw/daemon.c | 28 +++++++++-- 2 files changed, 94 insertions(+), 53 deletions(-) diff --git a/ucw/daemon-ctrl.c b/ucw/daemon-ctrl.c index 21132e32..c64be6c3 100644 --- a/ucw/daemon-ctrl.c +++ b/ucw/daemon-ctrl.c @@ -2,6 +2,7 @@ * UCW Library -- Daemon Control * * (c) 2012 Martin Mares + * (c) 2014 Pavel Charvat * * This software may be freely distributed and used according to the terms * of the GNU Lesser General Public License. @@ -18,6 +19,7 @@ #include #include #include +#include #include #include @@ -31,34 +33,44 @@ daemon_control_err(struct daemon_control_params *dc, char *msg, ...) return DAEMON_STATUS_ERROR; } -static int -daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *stalep) +static enum daemon_control_status +daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *pidp) { - *stalep = 0; + // Possible results: + // -- DAEMON_STATUS_ERROR, pid == 0 + // -- DAEMON_STATUS_NOT_RUNNING, pid == 0 + // -- DAEMON_STATUS_OK, pid > 0 + // -- DAEMON_STATUS_OK, pid == 0 (stopping) + + enum daemon_control_status st = DAEMON_STATUS_NOT_RUNNING; + *pidp = 0; - int pid_fd = open(dc->pid_file, O_RDONLY); + int pid_fd = open(dc->pid_file, O_RDWR); if (pid_fd < 0) { if (errno == ENOENT) - return 0; - daemon_control_err(dc, "Cannot open PID file `%s': %m", dc->pid_file); - return -1; + return st; + return daemon_control_err(dc, "Cannot open PID file `%s': %m", dc->pid_file); } - if (flock(pid_fd, LOCK_EX | (will_wait ? 0 : LOCK_NB)) >= 0) + while (flock(pid_fd, LOCK_EX | (will_wait ? 0 : LOCK_NB)) < 0) { - // The lock file is stale - close(pid_fd); - *stalep = 1; - return 0; - } - - if (errno != EINTR && errno != EWOULDBLOCK) - { - daemon_control_err(dc, "Cannot lock PID file `%s': %m", dc->pid_file); - goto fail; + if (errno == EWOULDBLOCK) + { + st = DAEMON_STATUS_OK; + break; + } + else if (errno != EINTR) + { + daemon_control_err(dc, "Cannot lock PID file `%s': %m", dc->pid_file); + goto fail; + } } + // Beware that daemon modifies PID file by write() + ftruncate() => even with + // atomic read() and write() we could get more bytes then expected. + // It should not be problem here, but it would be probably better to use + // the already existing fcntl lock or align all writes to a common size. char buf[16]; int n = read(pid_fd, buf, sizeof(buf)); if (n < 0) @@ -72,6 +84,19 @@ daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *stalep) goto fail; } buf[n] = 0; + + if (!strncmp(buf, "(stopped)", 9)) + { + close(pid_fd); + return st; + } + + if (st != DAEMON_STATUS_OK) + { + close(pid_fd); + return DAEMON_STATUS_STALE; + } + int pid = atoi(buf); if (!pid) { @@ -79,19 +104,17 @@ daemon_read_pid(struct daemon_control_params *dc, int will_wait, int *stalep) goto fail; } close(pid_fd); - return pid; + *pidp = pid; + return DAEMON_STATUS_OK; fail: close(pid_fd); - return -1; + return DAEMON_STATUS_ERROR; } enum daemon_control_status daemon_control(struct daemon_control_params *dc) { - enum daemon_control_status st = DAEMON_STATUS_ERROR; - int sig, stale, stale2; - int guard_fd = open(dc->guard_file, O_RDWR | O_CREAT, 0666); if (guard_fd < 0) return daemon_control_err(dc, "Cannot open guard file `%s': %m", dc->guard_file); @@ -99,29 +122,24 @@ daemon_control(struct daemon_control_params *dc) return daemon_control_err(dc, "Cannot lock guard file `%s': %m", dc->guard_file); // Read the PID file - int pid = daemon_read_pid(dc, 0, &stale); - if (pid < 0) + int pid, sig; + enum daemon_control_status st = daemon_read_pid(dc, 0, &pid); + if (st == DAEMON_STATUS_ERROR) goto done; switch (dc->action) { case DAEMON_CONTROL_CHECK: - if (pid) - st = DAEMON_STATUS_OK; - else if (stale) - st = DAEMON_STATUS_STALE; - else - st = DAEMON_STATUS_NOT_RUNNING; break; case DAEMON_CONTROL_START: - if (pid) + if (st == DAEMON_STATUS_OK) st = DAEMON_STATUS_ALREADY_DONE; else { pid_t pp = fork(); if (pp < 0) { - daemon_control_err(dc, "Cannot fork: %m"); + st = daemon_control_err(dc, "Cannot fork: %m"); goto done; } if (!pp) @@ -135,39 +153,40 @@ daemon_control(struct daemon_control_params *dc) int ec = waitpid(pp, &stat, 0); if (ec < 0) { - daemon_control_err(dc, "Cannot wait: %m"); + st = daemon_control_err(dc, "Cannot wait: %m"); goto done; } if (WIFEXITED(stat) && WEXITSTATUS(stat) == DAEMON_STATUS_ERROR) { - daemon_control_err(dc, "Cannot execute the daemon"); + st = daemon_control_err(dc, "Cannot execute the daemon"); goto done; } char ecmsg[EXIT_STATUS_MSG_SIZE]; if (format_exit_status(ecmsg, stat)) { - daemon_control_err(dc, "Daemon %s %s", dc->argv[0], ecmsg); + st = daemon_control_err(dc, "Daemon %s %s", dc->argv[0], ecmsg); goto done; } - pid = daemon_read_pid(dc, 0, &stale2); - if (!pid) - daemon_control_err(dc, "Daemon %s failed to write the PID file `%s'", dc->argv[0], dc->pid_file); - else - st = stale ? DAEMON_STATUS_STALE : DAEMON_STATUS_OK; + st = DAEMON_STATUS_OK; } break; case DAEMON_CONTROL_STOP: - if (!pid) - return stale ? DAEMON_STATUS_STALE : DAEMON_STATUS_ALREADY_DONE; - sig = dc->signal ? : SIGTERM; - if (kill(pid, sig) < 0) + if (st == DAEMON_STATUS_NOT_RUNNING) { - daemon_control_err(dc, "Cannot send signal %d: %m", sig); + st = DAEMON_STATUS_ALREADY_DONE; goto done; } - pid = daemon_read_pid(dc, 1, &stale2); - ASSERT(pid <= 0); - if (!pid) + if (pid) + { + sig = dc->signal ? : SIGTERM; + if (kill(pid, sig) < 0) + { + st = daemon_control_err(dc, "Cannot send signal %d: %m", sig); + goto done; + } + } + st = daemon_read_pid(dc, 1, &pid); + if (st != DAEMON_STATUS_ERROR) st = DAEMON_STATUS_OK; break; case DAEMON_CONTROL_SIGNAL: @@ -175,7 +194,7 @@ daemon_control(struct daemon_control_params *dc) return DAEMON_STATUS_NOT_RUNNING; sig = dc->signal ? : SIGHUP; if (kill(pid, sig) < 0) - daemon_control_err(dc, "Cannot send signal %d: %m", sig); + st = daemon_control_err(dc, "Cannot send signal %d: %m", sig); else st = DAEMON_STATUS_OK; break; diff --git a/ucw/daemon.c b/ucw/daemon.c index 9b136d8a..5ca9543a 100644 --- a/ucw/daemon.c +++ b/ucw/daemon.c @@ -2,6 +2,7 @@ * UCW Library -- Daemonization * * (c) 2012--2014 Martin Mares + * (c) 2014 Pavel Charvat * * This software may be freely distributed and used according to the terms * of the GNU Lesser General Public License. @@ -18,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -79,7 +81,8 @@ daemon_resolve_ugid(struct daemon_params *dp) } } -void daemon_switch_ugid(struct daemon_params *dp) +void +daemon_switch_ugid(struct daemon_params *dp) { if (dp->want_setgid && setresgid(dp->run_as_gid, dp->run_as_gid, dp->run_as_gid) < 0) die("Cannot set GID to %d: %m", (int) dp->run_as_gid); @@ -89,6 +92,17 @@ void daemon_switch_ugid(struct daemon_params *dp) die("Cannot set UID to %d: %m", (int) dp->run_as_uid); } +static void +daemon_fcntl_lock(struct daemon_params *dp) +{ + struct flock fl = { .l_type = F_WRLCK, .l_whence = SEEK_SET }; + while (fcntl(dp->pid_fd, F_SETLKW, &fl) < 0) + { + if (errno != EINTR) + die("Unable to get fcntl lock on '%s': %m", dp->pid_file); + } +} + void daemon_init(struct daemon_params *dp) { @@ -120,6 +134,11 @@ daemon_init(struct daemon_params *dp) die("Cannot lock `%s': %m", dp->pid_file); } + // Also temporarily lock it with a fcntl lock until the master process + // finishes writing of pid -- used to avoid possible collision with + // writing of "(stopped)" + daemon_fcntl_lock(dp); + // Make a note that the daemon is starting if (write(dp->pid_fd, "(starting)\n", 11) != 11 || ftruncate(dp->pid_fd, 11) < 0) @@ -189,8 +208,11 @@ daemon_exit(struct daemon_params *dp) if (dp->pid_file) { - if (unlink(dp->pid_file) < 0) - msg(L_ERROR, "Cannot unlink PID file `%s': %m", dp->pid_file); + daemon_fcntl_lock(dp); + if (lseek(dp->pid_fd, 0, SEEK_SET) < 0 || + write(dp->pid_fd, "(stopped)", 9) != 9 || + ftruncate(dp->pid_fd, 9)) + die("Error writing `%s': %m", dp->pid_file); close(dp->pid_fd); } } -- 2.39.2