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

[Laine Stump]

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