[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] virtlockd: make re-exec more robust



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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]