On 08/02/2011 01:11 PM, Eric Blake wrote:
The previous qemu patch could end up calling unlink(tmp) before tmp was the name of a valid file (unlinking a fileXXXXXX template instead), or calling unlink(tmp) twice on success (once here, and once at the end of the stream). Meanwhile, vbox also suffered from the same leaked tmp file bug.
ACK.
* 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; }