Re: [libvirt] [PATCH] screenshot: don't unlink bogus file

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.

* 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; }