Although virFDStreamOpenFile will unlink it once opened, when we hit error path, we must unlink it by hand. --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 09b2791..c78cdb7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom, endjob: VIR_FORCE_CLOSE(tmp_fd); - VIR_FREE(tmp); + if (tmp) { + unlink(tmp); + VIR_FREE(tmp); + }

Meanwhile, I wonder if we have a bigger bug - that is, should virFDStreamOpenInternal guarantee that the file is deleted when requested, even on failure paths, so that callers need not do the unlink()? That is, on the success path, we end up unlink()ing the same name twice, which is racy if the name is predictable (in the case of qemuDomainScreenshot, the name is temporary and supposedly safe).

Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a race where it can open() a file after the parent process has unlink()d the same file, it makes more sense to make the callers both create and unlink, rather than the caller create and the stream unlink.

* src/fdstream.h (virFDStreamOpenFile, virFDStreamCreateFile): Callers are responsible for deletion. * src/fdstream.c (virFDStreamOpenFileInternal): Don't leak created file on failure. (virFDStreamOpenFile, virFDStreamCreateFile): Drop parameter. * src/lxc/lxc_driver.c (lxcDomainOpenConsole): Update callers. * src/qemu/qemu_driver.c (qemuDomainScreenshot) (qemuDomainOpenConsole): Likewise. * src/storage/storage_driver.c (storageVolumeDownload) (storageVolumeUpload): Likewise. * src/uml/uml_driver.c (umlDomainOpenConsole): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise. * src/xen/xen_driver.c (xenUnifiedDomainOpenConsole): Likewise. --- This is the bigger patch, which I'm worried may be too invasive for 0.9.4 this late in the release cycle, but is cleaner overall. src/fdstream.c | 23 +++++++++-------------- src/fdstream.h | 6 ++---- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 11 ++++++----- src/storage/storage_driver.c | 4 ++-- src/uml/uml_driver.c | 2 +- src/vbox/vbox_tmpl.c | 8 ++------ src/xen/xen_driver.c | 2 +- 8 files changed, 24 insertions(+), 34 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index 2b7812f..b60162c 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -505,8 +505,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, unsigned long long offset, unsigned long long length, int oflags, - int mode, - bool delete) + int mode) { int fd = -1; int fds[2] = { -1, -1 }; @@ -514,8 +513,8 @@ virFDStreamOpenFileInternal(virStreamPtr st, virCommandPtr cmd = NULL; int errfd = -1; - VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o delete=%d", - st, path, oflags, offset, length, mode, delete); + VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o", + st, path, oflags, offset, length, mode); if (oflags& O_CREAT) fd = open(path, oflags, mode); @@ -593,9 +592,6 @@ virFDStreamOpenFileInternal(virStreamPtr st, if (virFDStreamOpenInternal(st, fd, cmd, errfd, length)< 0) goto error; - if (delete) - unlink(path); - return 0; error: @@ -603,6 +599,8 @@ error: VIR_FORCE_CLOSE(fds[0]); VIR_FORCE_CLOSE(fds[1]); VIR_FORCE_CLOSE(fd); + if (oflags& O_CREAT) + unlink(path); return -1; } @@ -610,8 +608,7 @@ int virFDStreamOpenFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int oflags, - bool delete) + int oflags) { if (oflags& O_CREAT) { streamsReportError(VIR_ERR_INTERNAL_ERROR, @@ -621,7 +618,7 @@ int virFDStreamOpenFile(virStreamPtr st, } return virFDStreamOpenFileInternal(st, path, offset, length, - oflags, 0, delete); + oflags, 0); } int virFDStreamCreateFile(virStreamPtr st, @@ -629,11 +626,9 @@ int virFDStreamCreateFile(virStreamPtr st, unsigned long long offset, unsigned long long length, int oflags, - mode_t mode, - bool delete) + mode_t mode) { return virFDStreamOpenFileInternal(st, path, offset, length, - oflags | O_CREAT, - mode, delete); + oflags | O_CREAT, mode); } diff --git a/src/fdstream.h b/src/fdstream.h index 76f0cd0..f9edb23 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -37,14 +37,12 @@ int virFDStreamOpenFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int oflags, - bool delete); + int oflags); int virFDStreamCreateFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, int oflags, - mode_t mode, - bool delete); + mode_t mode); #endif /* __VIR_FDSTREAM_H_ */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7d99d27..2d94309 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2889,7 +2889,7 @@ lxcDomainOpenConsole(virDomainPtr dom, } if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false)< 0) + 0, 0, O_RDWR)< 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..5c6d1b8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2862,6 +2862,7 @@ qemuDomainScreenshot(virDomainPtr dom, char *tmp = NULL; int tmp_fd = -1; char *ret = NULL; + bool unlink_tmp = false; virCheckFlags(0, NULL); @@ -2906,27 +2907,25 @@ qemuDomainScreenshot(virDomainPtr dom, virReportSystemError(errno, _("mkstemp(\"%s\") failed"), tmp); goto endjob; } + unlink_tmp = true; virSecurityManagerSetSavedStateLabel(qemu_driver->securityManager, vm, tmp); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorScreendump(priv->mon, tmp)< 0) { qemuDomainObjExitMonitor(driver, vm); - unlink(tmp); goto endjob; } qemuDomainObjExitMonitor(driver, vm); if (VIR_CLOSE(tmp_fd)< 0) { virReportSystemError(errno, _("unable to close %s"), tmp); - unlink(tmp); goto endjob; } - if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY, true)< 0) { + if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY)< 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to open stream")); - unlink(tmp); goto endjob; } @@ -2934,6 +2933,8 @@ qemuDomainScreenshot(virDomainPtr dom, endjob: VIR_FORCE_CLOSE(tmp_fd); + if (unlink_tmp) + unlink(tmp); VIR_FREE(tmp); if (qemuDomainObjEndJob(driver, vm) == 0) @@ -9259,7 +9260,7 @@ qemuDomainOpenConsole(virDomainPtr dom, } if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false)< 0) + 0, 0, O_RDWR)< 0) goto cleanup; ret = 0; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9c353e3..6715790 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1614,7 +1614,7 @@ storageVolumeDownload(virStorageVolPtr obj, if (virFDStreamOpenFile(stream, vol->target.path, offset, length, - O_RDONLY, false)< 0) + O_RDONLY)< 0) goto out; ret = 0; @@ -1678,7 +1678,7 @@ storageVolumeUpload(virStorageVolPtr obj, if (virFDStreamOpenFile(stream, vol->target.path, offset, length, - O_WRONLY, false)< 0) + O_WRONLY)< 0) goto out; ret = 0; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 6d04120..a9cb7ef 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2295,7 +2295,7 @@ umlDomainOpenConsole(virDomainPtr dom, } if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false)< 0) + 0, 0, O_RDWR)< 0) goto cleanup; ret = 0; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 66a0fe9..5c71729 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8713,7 +8713,6 @@ vboxDomainScreenshot(virDomainPtr dom, if (NS_FAILED(rc) || !width || !height) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to get screen resolution")); - unlink(tmp); goto endjob; } @@ -8724,7 +8723,6 @@ vboxDomainScreenshot(virDomainPtr dom, if (NS_FAILED(rc)) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to take screenshot")); - unlink(tmp); goto endjob; } @@ -8732,20 +8730,17 @@ vboxDomainScreenshot(virDomainPtr dom, screenDataSize)< 0) { virReportSystemError(errno, _("unable to write data " "to '%s'"), tmp); - unlink(tmp); goto endjob; } if (VIR_CLOSE(tmp_fd)< 0) { virReportSystemError(errno, _("unable to close %s"), tmp); - unlink(tmp); goto endjob; } - if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY, true)< 0) { + if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY)< 0) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to open stream")); - unlink(tmp); goto endjob; } @@ -8761,6 +8756,7 @@ endjob: } VIR_FORCE_CLOSE(tmp_fd); + unlink(tmp); VIR_FREE(tmp); VBOX_RELEASE(machine); vboxIIDUnalloc(&iid); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 76506fb..2c66143 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2164,7 +2164,7 @@ xenUnifiedDomainOpenConsole(virDomainPtr dom, } if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false)< 0) + 0, 0, O_RDWR)< 0) goto cleanup; ret = 0;