Re: [Qemu-devel] [PATCH v4 20/29] util: add qemu_write_pidfile()

[Daniel P . Berrangé]

On Fri, Jul 13, 2018 at 03:09:07PM +0200, Marc-André Lureau wrote:
> There are variants of qemu_create_pidfile() in qemu-pr-helper and
> qemu-ga. Let's have a common implementation in libqemuutil.
> 
> The code is based from pr-helper write_pidfile(), but allows the
> caller to deal with error reporting and behaviour.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/qemu/osdep.h  |  3 ++-
>  os-posix.c            | 24 -------------------
>  os-win32.c            | 25 --------------------
>  qga/main.c            | 54 ++++++++-----------------------------------
>  scsi/qemu-pr-helper.c | 40 ++++----------------------------
>  util/oslib-posix.c    | 33 ++++++++++++++++++++++++++
>  util/oslib-win32.c    | 27 ++++++++++++++++++++++
>  vl.c                  |  4 ++--
>  8 files changed, 79 insertions(+), 131 deletions(-)



> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 13b6f8d776..da1d4a3201 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -88,6 +88,39 @@ int qemu_daemon(int nochdir, int noclose)
>      return daemon(nochdir, noclose);
>  }
>  
> +bool qemu_write_pidfile(const char *pidfile, Error **errp)
> +{
> +    int pidfd;
> +    char pidstr[32];
> +
> +    pidfd = qemu_open(pidfile, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR);
> +    if (pidfd == -1) {
> +        error_setg_errno(errp, errno, "Cannot open pid file");
> +        return false;
> +    }
> +
> +    if (lockf(pidfd, F_TLOCK, 0)) {
> +        error_setg_errno(errp, errno, "Cannot lock pid file");
> +        goto fail;
> +    }
> +    if (ftruncate(pidfd, 0)) {
> +        error_setg_errno(errp, errno, "Failed to truncate pid file");
> +        goto fail;
> +    }
> +
> +    snprintf(pidstr, sizeof(pidstr), "%d\n", getpid());
> +    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
> +        error_setg(errp, "Failed to write pid file");
> +        goto fail;
> +    }
> +    return true;
> +
> +fail:
> +    unlink(pidfile);

Danger,  Will Robinson !

We can get to this fail: label if we were unable to lockf() the
pidfile. ie someone else owns the pidfile, and we've now unlinked
the pidfile they own.


> +    close(pidfd);
> +    return false;
> +}
> +
>  void *qemu_oom_check(void *ptr)
>  {
>      if (ptr == NULL) {


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|