[PATCH RESEND 1/5] virCommand: Actually acquire pidfile instead of just writing it

[Marc-André Lureau]

Hi


On Mon, Mar 23, 2020 at 5:14 PM Michal Privoznik <mprivozn at redhat.com> wrote:
>
> Our virCommand module allows us to set a pidfile for commands we
> want to spawn. The caller constructs the string of pidfile path
> and then uses virCommandSetPidFile() to tell the module to write
> the pidfile once the command is ran. This usually works, but has
> two flaws:
>
> 1) the child process does not hold the pidfile open & locked.
> Therefore, the caller (or anybody else) can't use our fancy
> virPidFileForceCleanupPath() function to kill the command
> afterwards. Also, for everybody else on the system it's
> needlessly harder to check if the pid from the pidfile is still
> alive or not.
>
> 2) if the caller ever makes a mistake and passes the same pidfile
> path for two different commands, the start of the second command
> will overwrite the pidfile even though the first command might
> still be running.
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau at redhat.com>


> ---
>  docs/internals/command.html.in |  4 ++-
>  src/util/vircommand.c          | 56 +++++++++++++++++++++++++++++-----
>  tests/commanddata/test4.log    |  1 +
>  3 files changed, 52 insertions(+), 9 deletions(-)
>
> diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
> index 8a9061e70f..823d89cc71 100644
> --- a/docs/internals/command.html.in
> +++ b/docs/internals/command.html.in
> @@ -226,7 +226,9 @@ virCommandSetPidFile(cmd, "/var/run/dnsmasq.pid");
>
>      <p>
>        This PID file is guaranteed to be written before
> -      the intermediate process exits.
> +      the intermediate process exits. Moreover, the daemonized
> +      process will inherit the FD of the opened and locked PID
> +      file.
>      </p>
>
>      <h3><a id="privs">Reducing command privileges</a></h3>
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 9ffa0cf49b..77078d09fb 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -612,6 +612,7 @@ virExec(virCommandPtr cmd)
>      int null = -1;
>      int pipeout[2] = {-1, -1};
>      int pipeerr[2] = {-1, -1};
> +    int pipesync[2] = {-1, -1};
>      int childin = cmd->infd;
>      int childout = -1;
>      int childerr = -1;
> @@ -745,9 +746,15 @@ virExec(virCommandPtr cmd)
>      /* Initialize full logging for a while */
>      virLogSetFromEnv();
>
> +    if (cmd->pidfile &&
> +        virPipe(pipesync) < 0)
> +        goto fork_error;
> +
>      /* Daemonize as late as possible, so the parent process can detect
>       * the above errors with wait* */
>      if (cmd->flags & VIR_EXEC_DAEMON) {
> +        char c;
> +
>          if (setsid() < 0) {
>              virReportSystemError(errno,
>                                   "%s", _("cannot become session leader"));
> @@ -768,12 +775,13 @@ virExec(virCommandPtr cmd)
>          }
>
>          if (pid > 0) {
> -            if (cmd->pidfile && (virPidFileWritePath(cmd->pidfile, pid) < 0)) {
> -                if (virProcessKillPainfully(pid, true) >= 0)
> -                    virReportSystemError(errno,
> -                                         _("could not write pidfile %s for %d"),
> -                                         cmd->pidfile, pid);
> -                goto fork_error;
> +            /* The parent expect us to have written the pid file before
> +             * exiting. Wait here for the child to write it and signal us. */
> +            if (cmd->pidfile &&
> +                saferead(pipesync[0], &c, sizeof(c)) != sizeof(c)) {
> +                virReportSystemError(errno, "%s",
> +                                     _("Unable to wait for child process"));
> +                _exit(EXIT_FAILURE);
>              }
>              _exit(EXIT_SUCCESS);
>          }
> @@ -788,6 +796,37 @@ virExec(virCommandPtr cmd)
>      if (cmd->setMaxCore &&
>          virProcessSetMaxCoreSize(0, cmd->maxCore) < 0)
>          goto fork_error;
> +    if (cmd->pidfile) {
> +        VIR_AUTOCLOSE pidfilefd = -1;
> +        int newpidfilefd = -1;
> +        char c;
> +
> +        pidfilefd = virPidFileAcquirePath(cmd->pidfile, false, getpid());
> +        if (pidfilefd < 0)
> +            goto fork_error;
> +        if (virSetInherit(pidfilefd, true) < 0) {
> +            virReportSystemError(errno, "%s",
> +                                 _("Cannot disable close-on-exec flag"));
> +            goto fork_error;
> +        }
> +
> +        c = '1';
> +        if (safewrite(pipesync[1], &c, sizeof(c)) != sizeof(c)) {
> +            virReportSystemError(errno, "%s", _("Unable to notify child process"));
> +            goto fork_error;
> +        }
> +        VIR_FORCE_CLOSE(pipesync[0]);
> +        VIR_FORCE_CLOSE(pipesync[1]);
> +
> +        /* This is here only to move the pidfilefd
> +         * to the lowest possible number. */
> +        if ((newpidfilefd = dup(pidfilefd)) < 0) {
> +            virReportSystemError(errno, "%s", _("Unable to dup FD"));
> +            goto fork_error;
> +        }
> +
> +        /* newpidfilefd is intentionally leaked. */
> +    }
>
>      if (cmd->hook) {
>          VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque);
> @@ -1080,8 +1119,9 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd)
>   * @cmd: the command to modify
>   * @pidfile: filename to use
>   *
> - * Save the child PID in a pidfile.  The pidfile will be populated
> - * before the exec of the child.
> + * Save the child PID in a pidfile. The pidfile will be populated before the
> + * exec of the child and the child will inherit opened and locked FD to the
> + * pidfile.
>   */
>  void
>  virCommandSetPidFile(virCommandPtr cmd, const char *pidfile)
> diff --git a/tests/commanddata/test4.log b/tests/commanddata/test4.log
> index f170be204e..5820f28307 100644
> --- a/tests/commanddata/test4.log
> +++ b/tests/commanddata/test4.log
> @@ -9,6 +9,7 @@ ENV:USER=test
>  FD:0
>  FD:1
>  FD:2
> +FD:3
>  DAEMON:yes
>  CWD:/
>  UMASK:0022
> --
> 2.24.1
>


--
Marc-André Lureau