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 <f...@n621.de>
---
  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 <ebl...@redhat.com>

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

Reply via email to