Re: [Qemu-devel] [PATCH for-2.12] os: truncate pidfile on creation

[Eric Blake]

On 03/20/2018 04:31 AM, Florian Larysch wrote:
> qemu_create_pidfile does not truncate the pidfile when it creates it,
> but rather overwrites its contents with the new pid. This works fine as
> long as the length of the pid doesn't decrease, but this might happen in
> case of wraparounds, causing pidfiles to contain trailing garbage which
> breaks operations such as 'kill $(cat pidfile)'.

Ouch.  Good analysis.

However, I have to wonder if we have the opposite problem - when the 
file exists (truncated) but has not yet been written, how often do we 
have a race where someone can see the empty file?

Should we be going even further and writing the pid into a temporary 
file and then rename(2)ing that file onto the real destination, so that 
if the pid file exists at all, it has stable contents that can't cause 
confusion?

> Instead, always truncate the file before writing it.
>
> Signed-off-by: Florian Larysch <address@hidden>
> ---
>   os-posix.c | 2 +-
>   os-win32.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/os-posix.c b/os-posix.c
> index b9c2343b1e..9a6b874180 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -301,7 +301,7 @@ int qemu_create_pidfile(const char *filename)
>       int len;
>       int fd;
>   
> -    fd = qemu_open(filename, O_RDWR | O_CREAT, 0600);
> +    fd = qemu_open(filename, O_RDWR | O_CREAT | O_TRUNC, 0600);

This part is right, if we don't care about the race with someone reading 
an empty file.

>       if (fd == -1) {
>           return -1;
>       }
> diff --git a/os-win32.c b/os-win32.c
> index 586a7c7d49..85dbad7af8 100644
> --- a/os-win32.c
> +++ b/os-win32.c
> @@ -108,7 +108,7 @@ int qemu_create_pidfile(const char *filename)
>       memset(&overlap, 0, sizeof(overlap));
>   
>       file = CreateFile(filename, GENERIC_WRITE, FILE_SHARE_READ, NULL,
> -                      OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
> +                      CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

I assume this is right (at least for looking comparable to the POSIX 
case), although I'm trusting you here.

Reviewed-by: Eric Blake <address@hidden>

Worth including in 2.12, unless we want to also avoid the race of an 
empty pidfile.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org