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

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 09:50:19AM -0500, Eric Blake wrote:
> 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.

No, it is unsafe - we rely on lockf() to get the mutual exclusion.
If a QEMU is running with pidfile locked, and its pid written into
it, then a 2nd QEMU comes along it will truncate the pidfile owned
by the original QEMU because the truncation happens before it has
tried to acquire the lock. The 2nd QEMU will still exit, but the
original QEMU's pid has now been lost.

We must call ftruncate() after lockf(), but before writing the new
pid into the file. That ensures there is no window in which it is
possible to see the new & old pids mixed together.


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 :|