[libvirt] [PATCH v2] virtlockd: make re-exec more robust
Michal Privoznik
mprivozn at redhat.com
Wed Jan 22 09:51:03 UTC 2014
On 11.12.2013 09:07, Michael Chapman wrote:
> - Use $XDG_RUNTIME_DIR for re-exec state file when running unprivileged.
>
> - argv[0] may not contain a full path to the binary, however it should
> contain something that can be looked up in the PATH. Use execvp() to
> do path lookup on re-exec.
>
> - As per list discussion [1], ignore --daemon on re-exec.
>
> [1] https://www.redhat.com/archives/libvir-list/2013-December/msg00514.html
>
> Signed-off-by: Michael Chapman <mike at very.puzzling.org>
> ---
> src/locking/lock_daemon.c | 128 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 94 insertions(+), 34 deletions(-)
>
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index a6be43c..b405e3a 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -925,7 +925,41 @@ error:
> }
>
>
> -#define VIR_LOCK_DAEMON_RESTART_EXEC_FILE LOCALSTATEDIR "/run/virtlockd-restart-exec.json"
> +static int
> +virLockDaemonExecRestartStatePath(bool privileged,
> + char **state_file)
> +{
> + if (privileged) {
> + if (VIR_STRDUP(*state_file, LOCALSTATEDIR "/run/virtlockd-restart-exec.json") < 0)
> + goto error;
> + } else {
> + char *rundir = NULL;
> + mode_t old_umask;
> +
> + if (!(rundir = virGetUserRuntimeDirectory()))
> + goto error;
> +
> + old_umask = umask(077);
> + if (virFileMakePath(rundir) < 0) {
> + umask(old_umask);
> + goto error;
If we fail, when the control continues at the 'error' label ..
> + }
> + umask(old_umask);
> +
> + if (virAsprintf(state_file, "%s/virtlockd-restart-exec.json", rundir) < 0) {
> + VIR_FREE(rundir);
> + goto error;
> + }
> +
> + VIR_FREE(rundir);
> + }
> +
> + return 0;
> +
> +error:
.. here, where the @rundir is leaked.
> + return -1;
> +}
> +
>
> static char *
> virLockDaemonGetExecRestartMagic(void)
> @@ -938,7 +972,10 @@ virLockDaemonGetExecRestartMagic(void)
>
>
> static int
> -virLockDaemonPostExecRestart(bool privileged)
> +virLockDaemonPostExecRestart(const char *state_file,
> + const char *pid_file,
> + int *pid_file_fd,
> + bool privileged)
> {
> const char *gotmagic;
> char *wantmagic = NULL;
> @@ -948,14 +985,14 @@ virLockDaemonPostExecRestart(bool privileged)
>
> VIR_DEBUG("Running post-restart exec");
>
> - if (!virFileExists(VIR_LOCK_DAEMON_RESTART_EXEC_FILE)) {
> - VIR_DEBUG("No restart file %s present",
> - VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
> + if (!virFileExists(state_file)) {
> + VIR_DEBUG("No restart state file %s present",
> + state_file);
> ret = 0;
> goto cleanup;
> }
>
> - if (virFileReadAll(VIR_LOCK_DAEMON_RESTART_EXEC_FILE,
> + if (virFileReadAll(state_file,
> 1024 * 1024 * 10, /* 10 MB */
> &state) < 0)
> goto cleanup;
> @@ -982,13 +1019,18 @@ virLockDaemonPostExecRestart(bool privileged)
> goto cleanup;
> }
>
> + /* Re-claim PID file now as we will not be daemonizing */
> + if (pid_file &&
> + (*pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0)
> + goto cleanup;
> +
> if (!(lockDaemon = virLockDaemonNewPostExecRestart(object, privileged)))
> goto cleanup;
>
> ret = 1;
>
> cleanup:
> - unlink(VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
> + unlink(state_file);
> VIR_FREE(wantmagic);
> VIR_FREE(state);
> virJSONValueFree(object);
> @@ -997,7 +1039,8 @@ cleanup:
>
>
> static int
> -virLockDaemonPreExecRestart(virNetServerPtr srv,
> +virLockDaemonPreExecRestart(const char *state_file,
> + virNetServerPtr srv,
> char **argv)
> {
> virJSONValuePtr child;
> @@ -1065,15 +1108,15 @@ virLockDaemonPreExecRestart(virNetServerPtr srv,
>
> VIR_DEBUG("Saving state %s", state);
>
> - if (virFileWriteStr(VIR_LOCK_DAEMON_RESTART_EXEC_FILE,
> + if (virFileWriteStr(state_file,
> state, 0700) < 0) {
> virReportSystemError(errno,
> _("Unable to save state file %s"),
> - VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
> + state_file);
> goto cleanup;
> }
>
> - if (execv(argv[0], argv) < 0) {
> + if (execvp(argv[0], argv) < 0) {
> virReportSystemError(errno, "%s",
> _("Unable to restart self"));
> goto cleanup;
> @@ -1153,6 +1196,7 @@ int main(int argc, char **argv) {
> char *pid_file = NULL;
> int pid_file_fd = -1;
> char *sock_file = NULL;
> + char *state_file = NULL;
> bool implicit_conf = false;
> mode_t old_umask;
> bool privileged = false;
> @@ -1276,21 +1320,13 @@ int main(int argc, char **argv) {
> VIR_DEBUG("Decided on socket paths '%s'",
> sock_file);
>
> - if (godaemon) {
> - char ebuf[1024];
> -
> - if (chdir("/") < 0) {
> - VIR_ERROR(_("cannot change to root directory: %s"),
> - virStrerror(errno, ebuf, sizeof(ebuf)));
> - goto cleanup;
> - }
> -
> - if ((statuswrite = virLockDaemonForkIntoBackground(argv[0])) < 0) {
> - VIR_ERROR(_("Failed to fork as daemon: %s"),
> - virStrerror(errno, ebuf, sizeof(ebuf)));
> - goto cleanup;
> - }
> + if (virLockDaemonExecRestartStatePath(privileged,
> + &state_file) < 0) {
> + VIR_ERROR(_("Can't determine restart state file path"));
> + exit(EXIT_FAILURE);
> }
> + VIR_DEBUG("Decided on restart state file path '%s'",
> + state_file);
>
> /* Ensure the rundir exists (on tmpfs on some systems) */
> if (privileged) {
> @@ -1317,20 +1353,41 @@ int main(int argc, char **argv) {
> }
> umask(old_umask);
>
> - /* If we have a pidfile set, claim it now, exiting if already taken */
> - if ((pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0) {
> - ret = VIR_LOCK_DAEMON_ERR_PIDFILE;
> - goto cleanup;
> - }
> -
> - if ((rv = virLockDaemonPostExecRestart(privileged)) < 0) {
> + if ((rv = virLockDaemonPostExecRestart(state_file,
> + pid_file,
> + &pid_file_fd,
> + privileged)) < 0) {
> ret = VIR_LOCK_DAEMON_ERR_INIT;
> goto cleanup;
> }
>
> /* rv == 1, means we setup everything from saved state,
> - * so we only setup stuff from scratch if rv == 0 */
> + * so only (possibly) daemonize and setup stuff from
> + * scratch if rv == 0
> + */
> if (rv == 0) {
> + if (godaemon) {
> + char ebuf[1024];
> +
> + if (chdir("/") < 0) {
> + VIR_ERROR(_("cannot change to root directory: %s"),
> + virStrerror(errno, ebuf, sizeof(ebuf)));
> + goto cleanup;
> + }
> +
> + if ((statuswrite = virLockDaemonForkIntoBackground(argv[0])) < 0) {
> + VIR_ERROR(_("Failed to fork as daemon: %s"),
> + virStrerror(errno, ebuf, sizeof(ebuf)));
> + goto cleanup;
> + }
> + }
> +
> + /* If we have a pidfile set, claim it now, exiting if already taken */
> + if ((pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0) {
> + ret = VIR_LOCK_DAEMON_ERR_PIDFILE;
> + goto cleanup;
> + }
> +
> if (!(lockDaemon = virLockDaemonNew(config, privileged))) {
> ret = VIR_LOCK_DAEMON_ERR_INIT;
> goto cleanup;
> @@ -1388,7 +1445,9 @@ int main(int argc, char **argv) {
> virNetServerRun(lockDaemon->srv);
>
> if (execRestart &&
> - virLockDaemonPreExecRestart(lockDaemon->srv, argv) < 0)
> + virLockDaemonPreExecRestart(state_file,
> + lockDaemon->srv,
> + argv) < 0)
> ret = VIR_LOCK_DAEMON_ERR_REEXEC;
> else
> ret = 0;
> @@ -1410,6 +1469,7 @@ cleanup:
> virPidFileReleasePath(pid_file, pid_file_fd);
> VIR_FREE(pid_file);
> VIR_FREE(sock_file);
> + VIR_FREE(state_file);
> VIR_FREE(run_dir);
> return ret;
>
>
Other than that, the patch looks good to me. I'm squashing this in prior
to push:
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index adb1129..e047751 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -940,6 +940,7 @@ virLockDaemonExecRestartStatePath(bool privileged,
old_umask = umask(077);
if (virFileMakePath(rundir) < 0) {
umask(old_umask);
+ VIR_FREE(rundir);
goto error;
}
umask(old_umask);
ACK and sorry for late review.
Michal
More information about the libvir-list
mailing list