* src/qemu/qemu_driver.c (qemuDomainScreenshot): Don't unlink on
success, or on invalid name.
* src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Don't leak temp file.
---
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).
I audited all callers, and there were only two that passed delete=true.
Of those two, I found another bug (calling unlink() too soon).
Additionally, remember that the reason we passed delete=true in the
first place was due to libvirt_iohelper doing the unlink in a child
process; where a race condition required that we not do the unlink in
the parent. But now that libvirt_iohelper receives its fd by
inheritance, I think we can revert the delete parameter in the
first place. But that's invasive enough to delay to post-release.
Meanwhile, I still think this is worth applying pre-release.
src/qemu/qemu_driver.c | 8 ++++----
src/vbox/vbox_tmpl.c | 5 +++++
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5e2c903..0297159 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2912,18 +2912,21 @@ qemuDomainScreenshot(virDomainPtr dom,
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) {
qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("unable to open stream"));
+ unlink(tmp);
goto endjob;
}
@@ -2931,10 +2934,7 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob:
VIR_FORCE_CLOSE(tmp_fd);
- if (tmp) {
- unlink(tmp);
- VIR_FREE(tmp);
- }
+ VIR_FREE(tmp);
if (qemuDomainObjEndJob(driver, vm) == 0)
vm = NULL;
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index a7d354e..66a0fe9 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -8713,6 +8713,7 @@ vboxDomainScreenshot(virDomainPtr dom,
if (NS_FAILED(rc) || !width || !height) {
vboxError(VIR_ERR_OPERATION_FAILED, "%s",
_("unable to get screen resolution"));
+ unlink(tmp);
goto endjob;
}
@@ -8723,6 +8724,7 @@ vboxDomainScreenshot(virDomainPtr dom,
if (NS_FAILED(rc)) {
vboxError(VIR_ERR_OPERATION_FAILED, "%s",
_("failed to take screenshot"));
+ unlink(tmp);
goto endjob;
}
@@ -8730,17 +8732,20 @@ 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) {
vboxError(VIR_ERR_OPERATION_FAILED, "%s",
_("unable to open stream"));
+ unlink(tmp);
goto endjob;
}