[libvirt] [PATCH 5/6] virtlockd: make re-exec more robust

[Michal Privoznik]

On 09.12.2013 07:23, Michael Chapman wrote:
> virtlockd's 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 when re-executing ourselves.
> 
> After re-execution, we must not attempt to daemonize again. It's not
> only unnecessary, but it also means we end up with the wrong PID and so
> we can't validate the state file.
> 
> Instead, build a new argv for the new program that does not include
> --daemon.
> 
> Signed-off-by: Michael Chapman <mike at very.puzzling.org>
> ---
>  src/locking/lock_daemon.c | 49 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index ae09ef8..32ca4d6 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -1000,6 +1000,7 @@ cleanup:
>  
>  static int
>  virLockDaemonPreExecRestart(virNetServerPtr srv,
> +                            const char *argv0,
>                              char **argv)
>  {
>      virJSONValuePtr child;
> @@ -1075,7 +1076,7 @@ virLockDaemonPreExecRestart(virNetServerPtr srv,
>          goto cleanup;
>      }
>  
> -    if (execv(argv[0], argv) < 0) {
> +    if (execvp(argv0, argv) < 0) {
>          virReportSystemError(errno, "%s",
>                               _("Unable to restart self"));

While this chunk is okay,

>          goto cleanup;
> @@ -1389,11 +1390,47 @@ int main(int argc, char **argv) {
>      virNetServerUpdateServices(lockDaemon->srv, true);
>      virNetServerRun(lockDaemon->srv);
>  
> -    if (execRestart &&
> -        virLockDaemonPreExecRestart(lockDaemon->srv, argv) < 0)
> -        ret = VIR_LOCK_DAEMON_ERR_REEXEC;
> -    else
> -        ret = 0;
> +    ret = VIR_LOCK_DAEMON_ERR_NONE;
> +    if (execRestart) {
> +        char **restart_argv = NULL;
> +        size_t count = 0;
> +
> +        if (VIR_EXPAND_N(restart_argv, count, 1) < 0)
> +            goto no_memory;
> +        if (VIR_STRDUP(restart_argv[count - 1], argv[0]) < 0)
> +            goto no_memory;
> +        if (verbose) {
> +            if (VIR_EXPAND_N(restart_argv, count, 1) < 0)
> +                goto no_memory;
> +            if (VIR_STRDUP(restart_argv[count - 1], "--verbose") < 0)
> +                goto no_memory;
> +        }
> +        if (remote_config_file && !implicit_conf) {
> +            if (VIR_EXPAND_N(restart_argv, count, 2) < 0)
> +                goto no_memory;
> +            if (VIR_STRDUP(restart_argv[count - 2], "--config") < 0)
> +                goto no_memory;
> +            if (VIR_STRDUP(restart_argv[count - 1], remote_config_file) < 0)
> +                goto no_memory;
> +        }
> +        if (pid_file) {
> +            if (VIR_EXPAND_N(restart_argv, count, 2) < 0)
> +                goto no_memory;
> +            if (VIR_STRDUP(restart_argv[count - 2], "--pid-file") < 0)
> +                goto no_memory;
> +            if (VIR_STRDUP(restart_argv[count - 1], pid_file) < 0)
> +                goto no_memory;
> +        }
> +        if (VIR_EXPAND_N(restart_argv, count, 1) < 0)
> +            goto no_memory;
> +
> +        if (virLockDaemonPreExecRestart(lockDaemon->srv, argv[0], restart_argv) < 0)
> +            ret = VIR_LOCK_DAEMON_ERR_REEXEC;
> +
> +        while (count)
> +            VIR_FREE(restart_argv[--count]);
> +        VIR_FREE(restart_argv);
> +    }
>  
>  cleanup:
>      virObjectUnref(lockProgram);
> 

This one isn't. It requires us to expand it whenever a new cmd line
argument is invented. How about copying all arguments but "--daemon" or
"-d"?

(Just an explicit note - I'm not pushing this one now)

Michal