[Qemu-devel] bug writing pidfile under unix (and fix)

[Qemu-devel] bug writing pidfile under unix (and fix)

[Jim Bailey]

Hello,

In qemu_create_pidfile (osdep.c:229) the current pid and a newline is
written to the pidfile. However, the pidfile isn't truncated, so if it
is longer than the length of the pid and the newline character you get
trailing junk that can really mess up scripts.

I noticed this when going from a 5 digit pid to a 3 digit pid, so it
can happen in regular operation, especially if the OS randomizes pids.

Truncating the file fixes the bug.

dgym


*** osdep.c.orig        Fri Nov  7 16:56:12 2008
--- osdep.c     Fri Nov  7 16:58:49 2008
***************
*** 236,241 ****
--- 236,243 ----
      len = snprintf(buffer, sizeof(buffer), "%ld\n", (long)getpid());
      if (write(fd, buffer, len) != len)
          return -1;
+ 
+     ftruncate(fd, len);
  #else
      HANDLE file;
      DWORD flags;

Re: [Qemu-devel] bug writing pidfile under unix (and fix)

[Paul Brook]

On Friday 07 November 2008, Jim Bailey wrote:
> Hello,
>
> In qemu_create_pidfile (osdep.c:229) the current pid and a newline is
> written to the pidfile. However, the pidfile isn't truncated, so if it
> is longer than the length of the pid and the newline character you get
> trailing junk that can really mess up scripts.

Shouldn't we just open with O_TRUNC ?

I also notice that we're not removing the file when we exit.

Paul

Re: [Qemu-devel] bug writing pidfile under unix (and fix)

[Jim Bailey]

Hi Paul,

Opening with O_TRUNC causes a problem. If you try and start qemu again
(by accident presumably) it opens the pidfile and then fails to acquire
a lock on it. It then exits with a useful message "Could not acquire
pidfile" and you know that is has just saved you from trashing your
images.

If we open the file with O_TRUNC the contents get discarded before the
lock is checked, so although a second instance wouldn't run, it would
still leave you with an empty pidfile for your still-running first
instance.

I hope that makes sense.

Removing the file on exit would also be a great feature.

dgym



On Fri, 7 Nov 2008 18:56:11 +0100
Paul Brook <paul@codesourcery.com> wrote:

> On Friday 07 November 2008, Jim Bailey wrote:
> > Hello,
> >
> > In qemu_create_pidfile (osdep.c:229) the current pid and a newline is
> > written to the pidfile. However, the pidfile isn't truncated, so if it
> > is longer than the length of the pid and the newline character you get
> > trailing junk that can really mess up scripts.
> 
> Shouldn't we just open with O_TRUNC ?
> 
> I also notice that we're not removing the file when we exit.
> 
> Paul

Re: [Qemu-devel] bug writing pidfile under unix (and fix)

[Anthony Liguori]

Paul Brook wrote:
> On Friday 07 November 2008, Jim Bailey wrote:
>   
>> Hello,
>>
>> In qemu_create_pidfile (osdep.c:229) the current pid and a newline is
>> written to the pidfile. However, the pidfile isn't truncated, so if it
>> is longer than the length of the pid and the newline character you get
>> trailing junk that can really mess up scripts.
>>     
>
> Shouldn't we just open with O_TRUNC ?
>   

Definitely.

> I also notice that we're not removing the file when we exit.
>   

It's not terribly useful to remove the file but it couldn't hurt.  An 
advisory lock is used to ensure that only one process is running at a 
given time.

Regards,

Anthony Liguori

> Paul
>
>
>   

Re: [Qemu-devel] bug writing pidfile under unix (and fix)

[Jim Bailey]

On Fri, 07 Nov 2008 14:15:54 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Paul Brook wrote:
> > On Friday 07 November 2008, Jim Bailey wrote:
> >   
> >> Hello,
> >>
> >> In qemu_create_pidfile (osdep.c:229) the current pid and a newline is
> >> written to the pidfile. However, the pidfile isn't truncated, so if it
> >> is longer than the length of the pid and the newline character you get
> >> trailing junk that can really mess up scripts.
> >>     
> >
> > Shouldn't we just open with O_TRUNC ?
> >   
> 
> Definitely.

No no no, definitely not, please see my earlier comment.

We don't want to wipe the contents of the file until AFTER checking the
advisory lock.

Regards,

dgym


> 
> > I also notice that we're not removing the file when we exit.
> >   
> 
> It's not terribly useful to remove the file but it couldn't hurt.  An 
> advisory lock is used to ensure that only one process is running at a 
> given time.
> 
> Regards,
> 
> Anthony Liguori
> 
> > Paul
> >
> >
> >   
> 

Re: [Qemu-devel] bug writing pidfile under unix (and fix)

[Anthony Liguori]

Jim Bailey wrote:
> Hello,
>
> In qemu_create_pidfile (osdep.c:229) the current pid and a newline is
> written to the pidfile. However, the pidfile isn't truncated, so if it
> is longer than the length of the pid and the newline character you get
> trailing junk that can really mess up scripts.
>
> I noticed this when going from a 5 digit pid to a 3 digit pid, so it
> can happen in regular operation, especially if the OS randomizes pids.
>
> Truncating the file fixes the bug.
>   

Needs a Signed-off-by.

Regards,

Anthony Liguori

> dgym
>
>
> *** osdep.c.orig        Fri Nov  7 16:56:12 2008
> --- osdep.c     Fri Nov  7 16:58:49 2008
> ***************
> *** 236,241 ****
> --- 236,243 ----
>       len = snprintf(buffer, sizeof(buffer), "%ld\n", (long)getpid());
>       if (write(fd, buffer, len) != len)
>           return -1;
> + 
> +     ftruncate(fd, len);
>   #else
>       HANDLE file;
>       DWORD flags;
>
>
>