[PATCH v2] util: fix fd leak in qemu_write_pidfile()

[PATCH v2] util: fix fd leak in qemu_write_pidfile()

[Jie Wang]

if execute qemu_open success, have no branch to free the fd,
so unlink it inadvance, let it free by process exit.

Signed-off-by: Jie Wang <wangjie88@huawei.com>
---
 util/oslib-posix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 36820fec16..fa881f2ee8 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -131,6 +131,7 @@ bool qemu_write_pidfile(const char *path, Error **errp)
             error_setg_errno(errp, errno, "Cannot open pid file");
             return false;
         }
+        unlink(path);
 
         if (fstat(fd, &b) < 0) {
             error_setg_errno(errp, errno, "Cannot stat file");
-- 
2.23.0

Re: [PATCH v2] util: fix fd leak in qemu_write_pidfile()

[Peter Maydell]

On Mon, 10 May 2021 at 15:15, Jie Wang <wangjie88@huawei.com> wrote:
>
> if execute qemu_open success, have no branch to free the fd,
> so unlink it inadvance, let it free by process exit.
>
> Signed-off-by: Jie Wang <wangjie88@huawei.com>
> ---
>  util/oslib-posix.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 36820fec16..fa881f2ee8 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -131,6 +131,7 @@ bool qemu_write_pidfile(const char *path, Error **errp)
>              error_setg_errno(errp, errno, "Cannot open pid file");
>              return false;
>          }
> +        unlink(path);
>
>          if (fstat(fd, &b) < 0) {
>              error_setg_errno(errp, errno, "Cannot stat file");

This code change doesn't match the commit message -- the commit
message says it's trying to free a filedescriptor, but the code
change is unlinking a file.

Unlinking the file is definitely wrong, because the purpose of the
pidfile is to comminucate the QEMU PID to other processes -- if we
delete the file then those other processes can't find it. (The file
gets deleted when QEMU exits -- see qemu_unlink_pidfile() and the code
that calls it.)

thanks
-- PMM

Re: [PATCH v2] util: fix fd leak in qemu_write_pidfile()

[Daniel P. Berrangé]

On Mon, May 10, 2021 at 04:07:51PM +0100, Peter Maydell wrote:
> On Mon, 10 May 2021 at 15:15, Jie Wang <wangjie88@huawei.com> wrote:
> >
> > if execute qemu_open success, have no branch to free the fd,
> > so unlink it inadvance, let it free by process exit.
> >
> > Signed-off-by: Jie Wang <wangjie88@huawei.com>
> > ---
> >  util/oslib-posix.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index 36820fec16..fa881f2ee8 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -131,6 +131,7 @@ bool qemu_write_pidfile(const char *path, Error **errp)
> >              error_setg_errno(errp, errno, "Cannot open pid file");
> >              return false;
> >          }
> > +        unlink(path);
> >
> >          if (fstat(fd, &b) < 0) {
> >              error_setg_errno(errp, errno, "Cannot stat file");
> 
> This code change doesn't match the commit message -- the commit
> message says it's trying to free a filedescriptor, but the code
> change is unlinking a file.
> 
> Unlinking the file is definitely wrong, because the purpose of the
> pidfile is to comminucate the QEMU PID to other processes -- if we
> delete the file then those other processes can't find it. (The file
> gets deleted when QEMU exits -- see qemu_unlink_pidfile() and the code
> that calls it.)


We are also *intentionally* leaking the file descriptor. We have a lock
held on the FD and that lock must not be released until QEMU exits. We
thus leak the FD and rely on the OS to do cleanup when we exit.

We really ought to put a explicit comment in this method and people are
repeatedly trying to fix this non-bug.

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