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

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 05:21:16PM +0100, Florian Larysch wrote:
> On Tue, Mar 20, 2018 at 04:02:44PM +0000, Daniel P. Berrangé wrote:
> > 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.
> 
> That's correct, thanks for pointing it out.
> 
> > 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.
> 
> I'll send a revised version doing exactly that.
> 
> From my reading of the Windows API documentation, this might not be a
> problem there: The file is opened with FILE_SHARE_READ, which prohibits
> opening the file in a writable mode and CREATE_ALWAYS will only recreate
> the file if it is writable.

I don't think that's correct either I'm afraid.

FILE_SHARE_READ indicates whether other processes are allowed to open
the file in read-only mode, while we have it open for write.

We're passing GENERIC_WRITE so QEMU has it open for write, thus your
change will again cause the 2nd QEMU to blindly replace the file
owned by the original QEMU.

That said, I think the existing pidfile code for win32 is totally broken
because QEMU is closing it immediately after writing its pid. So the only
mutual exclusion is taking place in the tiny time window while QEMU is
writing the pid.

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