[PATCH] screenshot: don't unlink bogus file

[Eric Blake <eblake@xxxxxxxxxx>]

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

-- 
1.7.4.4

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list