| Message ID | 20210510141148.3138904-1-wangjie88@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] util: fix fd leak in qemu_write_pidfile() | expand |
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
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
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");
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(+)