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)'.

Instead, always truncate the file before writing it.

Note that the order is important here: We cannot simply use O_TRUNC in
the open() call because another qemu process might truncate the pidfile
of a process that is still running before reaching the lockf() barrier.

The Windows version suffers from a similar problem, but as it does not
provide effective mutual exclusion anyway (because the file handle is
closed immediately after writing to it), adopting this behavior still
seems to be an improvement, as it at least prevents garbeled pidfiles.

Signed-off-by: Florian Larysch <fl@n621.de>
---
 os-posix.c | 6 ++++++
 os-win32.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/os-posix.c b/os-posix.c
index b9c2343b1e..f2318aef55 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -309,6 +309,12 @@ int qemu_create_pidfile(const char *filename)
         close(fd);
         return -1;
     }
+
+    if (ftruncate(fd, 0)) {
+        close(fd);
+        return -1;
+    }
+
     len = snprintf(buffer, sizeof(buffer), FMT_pid "\n", getpid());
     if (write(fd, buffer, len) != len) {
         close(fd);
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);
 
     if (file == INVALID_HANDLE_VALUE) {
         return -1;
-- 
2.16.2

On 03/20/2018 11:58 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)'.
> 
> Instead, always truncate the file before writing it.
> 
> Note that the order is important here: We cannot simply use O_TRUNC in
> the open() call because another qemu process might truncate the pidfile
> of a process that is still running before reaching the lockf() barrier.
> 
> The Windows version suffers from a similar problem, but as it does not
> provide effective mutual exclusion anyway (because the file handle is
> closed immediately after writing to it), adopting this behavior still
> seems to be an improvement, as it at least prevents garbeled pidfiles.

s/garbeled/garbled/

> 
> Signed-off-by: Florian Larysch <fl@n621.de>
> ---
>   os-posix.c | 6 ++++++
>   os-win32.c | 2 +-
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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