[libvirt] [PATCH v2] qemu: Unlink temporary file on failure
Although virFDStreamOpenFile will unlink it once opened, when we hit error path, we must unlink it by hand. --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 09b2791..c78cdb7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom, endjob: VIR_FORCE_CLOSE(tmp_fd); - VIR_FREE(tmp); + if (tmp) { + unlink(tmp); + VIR_FREE(tmp); + } if (qemuDomainObjEndJob(driver, vm) == 0) vm = NULL; -- 1.7.3.4
On 08/02/2011 09:50 AM, Michal Privoznik wrote:
Although virFDStreamOpenFile will unlink it once opened, when we hit error path, we must unlink it by hand. --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 09b2791..c78cdb7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob: VIR_FORCE_CLOSE(tmp_fd); - VIR_FREE(tmp); + if (tmp) { + unlink(tmp); + VIR_FREE(tmp); + }
ACK. The VIR_FREE(tmp) could be unconditional, but it doesn't hurt to leave it conditional - at any rate, it won't trip the syntax-check for useless-if-before-free because we are doing more than just freeing tmp in the conditional. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/02/2011 09:59 AM, Eric Blake wrote:
On 08/02/2011 09:50 AM, Michal Privoznik wrote:
Although virFDStreamOpenFile will unlink it once opened, when we hit error path, we must unlink it by hand. --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 09b2791..c78cdb7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2931,7 +2931,10 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob: VIR_FORCE_CLOSE(tmp_fd); - VIR_FREE(tmp); + if (tmp) {
Wonky spacing.
+ unlink(tmp); + VIR_FREE(tmp); + }
ACK.
I went ahead and pushed this after fixing the spacing, so that it will be in 0.9.4. 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). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a race where it can open() a file after the parent process has unlink()d the same file, it makes more sense to make the callers both create and unlink, rather than the caller create and the stream unlink. * src/fdstream.h (virFDStreamOpenFile, virFDStreamCreateFile): Callers are responsible for deletion. * src/fdstream.c (virFDStreamOpenFileInternal): Don't leak created file on failure. (virFDStreamOpenFile, virFDStreamCreateFile): Drop parameter. * src/lxc/lxc_driver.c (lxcDomainOpenConsole): Update callers. * src/qemu/qemu_driver.c (qemuDomainScreenshot) (qemuDomainOpenConsole): Likewise. * src/storage/storage_driver.c (storageVolumeDownload) (storageVolumeUpload): Likewise. * src/uml/uml_driver.c (umlDomainOpenConsole): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise. * src/xen/xen_driver.c (xenUnifiedDomainOpenConsole): Likewise. --- This is the bigger patch, which I'm worried may be too invasive for 0.9.4 this late in the release cycle, but is cleaner overall. src/fdstream.c | 23 +++++++++-------------- src/fdstream.h | 6 ++---- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 11 ++++++----- src/storage/storage_driver.c | 4 ++-- src/uml/uml_driver.c | 2 +- src/vbox/vbox_tmpl.c | 8 ++------ src/xen/xen_driver.c | 2 +- 8 files changed, 24 insertions(+), 34 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index 2b7812f..b60162c 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -505,8 +505,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, unsigned long long offset, unsigned long long length, int oflags, - int mode, - bool delete) + int mode) { int fd = -1; int fds[2] = { -1, -1 }; @@ -514,8 +513,8 @@ virFDStreamOpenFileInternal(virStreamPtr st, virCommandPtr cmd = NULL; int errfd = -1; - VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o delete=%d", - st, path, oflags, offset, length, mode, delete); + VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o", + st, path, oflags, offset, length, mode); if (oflags & O_CREAT) fd = open(path, oflags, mode); @@ -593,9 +592,6 @@ virFDStreamOpenFileInternal(virStreamPtr st, if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0) goto error; - if (delete) - unlink(path); - return 0; error: @@ -603,6 +599,8 @@ error: VIR_FORCE_CLOSE(fds[0]); VIR_FORCE_CLOSE(fds[1]); VIR_FORCE_CLOSE(fd); + if (oflags & O_CREAT) + unlink(path); return -1; } @@ -610,8 +608,7 @@ int virFDStreamOpenFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int oflags, - bool delete) + int oflags) { if (oflags & O_CREAT) { streamsReportError(VIR_ERR_INTERNAL_ERROR, @@ -621,7 +618,7 @@ int virFDStreamOpenFile(virStreamPtr st, } return virFDStreamOpenFileInternal(st, path, offset, length, - oflags, 0, delete); + oflags, 0); } int virFDStreamCreateFile(virStreamPtr st, @@ -629,11 +626,9 @@ int virFDStreamCreateFile(virStreamPtr st, unsigned long long offset, unsigned long long length, int oflags, - mode_t mode, - bool delete) + mode_t mode) { return virFDStreamOpenFileInternal(st, path, offset, length, - oflags | O_CREAT, - mode, delete); + oflags | O_CREAT, mode); } diff --git a/src/fdstream.h b/src/fdstream.h index 76f0cd0..f9edb23 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -37,14 +37,12 @@ int virFDStreamOpenFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int oflags, - bool delete); + int oflags); int virFDStreamCreateFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, int oflags, - mode_t mode, - bool delete); + mode_t mode); #endif /* __VIR_FDSTREAM_H_ */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7d99d27..2d94309 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2889,7 +2889,7 @@ lxcDomainOpenConsole(virDomainPtr dom, } if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false) < 0) + 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..5c6d1b8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2862,6 +2862,7 @@ qemuDomainScreenshot(virDomainPtr dom, char *tmp = NULL; int tmp_fd = -1; char *ret = NULL; + bool unlink_tmp = false; virCheckFlags(0, NULL); @@ -2906,27 +2907,25 @@ qemuDomainScreenshot(virDomainPtr dom, virReportSystemError(errno, _("mkstemp(\"%s\") failed"), tmp); goto endjob; } + unlink_tmp = true; virSecurityManagerSetSavedStateLabel(qemu_driver->securityManager, vm, tmp); 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) { + if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY) < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to open stream")); - unlink(tmp); goto endjob; } @@ -2934,6 +2933,8 @@ qemuDomainScreenshot(virDomainPtr dom, endjob: VIR_FORCE_CLOSE(tmp_fd); + if (unlink_tmp) + unlink(tmp); VIR_FREE(tmp); if (qemuDomainObjEndJob(driver, vm) == 0) @@ -9259,7 +9260,7 @@ qemuDomainOpenConsole(virDomainPtr dom, } if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false) < 0) + 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9c353e3..6715790 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1614,7 +1614,7 @@ storageVolumeDownload(virStorageVolPtr obj, if (virFDStreamOpenFile(stream, vol->target.path, offset, length, - O_RDONLY, false) < 0) + O_RDONLY) < 0) goto out; ret = 0; @@ -1678,7 +1678,7 @@ storageVolumeUpload(virStorageVolPtr obj, if (virFDStreamOpenFile(stream, vol->target.path, offset, length, - O_WRONLY, false) < 0) + O_WRONLY) < 0) goto out; ret = 0; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 6d04120..a9cb7ef 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2295,7 +2295,7 @@ umlDomainOpenConsole(virDomainPtr dom, } if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false) < 0) + 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 66a0fe9..5c71729 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8713,7 +8713,6 @@ vboxDomainScreenshot(virDomainPtr dom, if (NS_FAILED(rc) || !width || !height) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to get screen resolution")); - unlink(tmp); goto endjob; } @@ -8724,7 +8723,6 @@ vboxDomainScreenshot(virDomainPtr dom, if (NS_FAILED(rc)) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to take screenshot")); - unlink(tmp); goto endjob; } @@ -8732,20 +8730,17 @@ 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) { + if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY) < 0) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to open stream")); - unlink(tmp); goto endjob; } @@ -8761,6 +8756,7 @@ endjob: } VIR_FORCE_CLOSE(tmp_fd); + unlink(tmp); VIR_FREE(tmp); VBOX_RELEASE(machine); vboxIIDUnalloc(&iid); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 76506fb..2c66143 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2164,7 +2164,7 @@ xenUnifiedDomainOpenConsole(virDomainPtr dom, } if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false) < 0) + 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; -- 1.7.4.4
On 08/02/2011 01:31 PM, Eric Blake wrote:
Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a race where it can open() a file after the parent process has unlink()d the same file, it makes more sense to make the callers both create and unlink, rather than the caller create and the stream unlink.
I wasn't paying attention to the messages/patches related to the race condition you reference, but this (caller creates and unlinks) definitely seems cleaner than the other way. Beyond that, the patch seems to be correct. ACK.
* src/fdstream.h (virFDStreamOpenFile, virFDStreamCreateFile): Callers are responsible for deletion. * src/fdstream.c (virFDStreamOpenFileInternal): Don't leak created file on failure. (virFDStreamOpenFile, virFDStreamCreateFile): Drop parameter. * src/lxc/lxc_driver.c (lxcDomainOpenConsole): Update callers. * src/qemu/qemu_driver.c (qemuDomainScreenshot) (qemuDomainOpenConsole): Likewise. * src/storage/storage_driver.c (storageVolumeDownload) (storageVolumeUpload): Likewise. * src/uml/uml_driver.c (umlDomainOpenConsole): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise. * src/xen/xen_driver.c (xenUnifiedDomainOpenConsole): Likewise. ---
This is the bigger patch, which I'm worried may be too invasive for 0.9.4 this late in the release cycle, but is cleaner overall.
src/fdstream.c | 23 +++++++++-------------- src/fdstream.h | 6 ++---- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 11 ++++++----- src/storage/storage_driver.c | 4 ++-- src/uml/uml_driver.c | 2 +- src/vbox/vbox_tmpl.c | 8 ++------ src/xen/xen_driver.c | 2 +- 8 files changed, 24 insertions(+), 34 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c index 2b7812f..b60162c 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -505,8 +505,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, unsigned long long offset, unsigned long long length, int oflags, - int mode, - bool delete) + int mode) { int fd = -1; int fds[2] = { -1, -1 }; @@ -514,8 +513,8 @@ virFDStreamOpenFileInternal(virStreamPtr st, virCommandPtr cmd = NULL; int errfd = -1;
- VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o delete=%d", - st, path, oflags, offset, length, mode, delete); + VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o", + st, path, oflags, offset, length, mode);
if (oflags& O_CREAT) fd = open(path, oflags, mode); @@ -593,9 +592,6 @@ virFDStreamOpenFileInternal(virStreamPtr st, if (virFDStreamOpenInternal(st, fd, cmd, errfd, length)< 0) goto error;
- if (delete) - unlink(path); - return 0;
error: @@ -603,6 +599,8 @@ error: VIR_FORCE_CLOSE(fds[0]); VIR_FORCE_CLOSE(fds[1]); VIR_FORCE_CLOSE(fd); + if (oflags& O_CREAT) + unlink(path); return -1; }
@@ -610,8 +608,7 @@ int virFDStreamOpenFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int oflags, - bool delete) + int oflags) { if (oflags& O_CREAT) { streamsReportError(VIR_ERR_INTERNAL_ERROR, @@ -621,7 +618,7 @@ int virFDStreamOpenFile(virStreamPtr st, } return virFDStreamOpenFileInternal(st, path, offset, length, - oflags, 0, delete); + oflags, 0); }
int virFDStreamCreateFile(virStreamPtr st, @@ -629,11 +626,9 @@ int virFDStreamCreateFile(virStreamPtr st, unsigned long long offset, unsigned long long length, int oflags, - mode_t mode, - bool delete) + mode_t mode) { return virFDStreamOpenFileInternal(st, path, offset, length, - oflags | O_CREAT, - mode, delete); + oflags | O_CREAT, mode); } diff --git a/src/fdstream.h b/src/fdstream.h index 76f0cd0..f9edb23 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -37,14 +37,12 @@ int virFDStreamOpenFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int oflags, - bool delete); + int oflags); int virFDStreamCreateFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, int oflags, - mode_t mode, - bool delete); + mode_t mode);
#endif /* __VIR_FDSTREAM_H_ */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7d99d27..2d94309 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2889,7 +2889,7 @@ lxcDomainOpenConsole(virDomainPtr dom, }
if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false)< 0) + 0, 0, O_RDWR)< 0) goto cleanup;
ret = 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0297159..5c6d1b8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2862,6 +2862,7 @@ qemuDomainScreenshot(virDomainPtr dom, char *tmp = NULL; int tmp_fd = -1; char *ret = NULL; + bool unlink_tmp = false;
virCheckFlags(0, NULL);
@@ -2906,27 +2907,25 @@ qemuDomainScreenshot(virDomainPtr dom, virReportSystemError(errno, _("mkstemp(\"%s\") failed"), tmp); goto endjob; } + unlink_tmp = true;
virSecurityManagerSetSavedStateLabel(qemu_driver->securityManager, vm, tmp);
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) { + if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY)< 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to open stream")); - unlink(tmp); goto endjob; }
@@ -2934,6 +2933,8 @@ qemuDomainScreenshot(virDomainPtr dom,
endjob: VIR_FORCE_CLOSE(tmp_fd); + if (unlink_tmp) + unlink(tmp); VIR_FREE(tmp);
if (qemuDomainObjEndJob(driver, vm) == 0) @@ -9259,7 +9260,7 @@ qemuDomainOpenConsole(virDomainPtr dom, }
if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false)< 0) + 0, 0, O_RDWR)< 0) goto cleanup;
ret = 0; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9c353e3..6715790 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1614,7 +1614,7 @@ storageVolumeDownload(virStorageVolPtr obj, if (virFDStreamOpenFile(stream, vol->target.path, offset, length, - O_RDONLY, false)< 0) + O_RDONLY)< 0) goto out;
ret = 0; @@ -1678,7 +1678,7 @@ storageVolumeUpload(virStorageVolPtr obj, if (virFDStreamOpenFile(stream, vol->target.path, offset, length, - O_WRONLY, false)< 0) + O_WRONLY)< 0) goto out;
ret = 0; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 6d04120..a9cb7ef 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2295,7 +2295,7 @@ umlDomainOpenConsole(virDomainPtr dom, }
if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false)< 0) + 0, 0, O_RDWR)< 0) goto cleanup;
ret = 0; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 66a0fe9..5c71729 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8713,7 +8713,6 @@ vboxDomainScreenshot(virDomainPtr dom, if (NS_FAILED(rc) || !width || !height) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to get screen resolution")); - unlink(tmp); goto endjob; }
@@ -8724,7 +8723,6 @@ vboxDomainScreenshot(virDomainPtr dom, if (NS_FAILED(rc)) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to take screenshot")); - unlink(tmp); goto endjob; }
@@ -8732,20 +8730,17 @@ 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) { + if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY)< 0) { vboxError(VIR_ERR_OPERATION_FAILED, "%s", _("unable to open stream")); - unlink(tmp); goto endjob; }
@@ -8761,6 +8756,7 @@ endjob: }
VIR_FORCE_CLOSE(tmp_fd); + unlink(tmp); VIR_FREE(tmp); VBOX_RELEASE(machine); vboxIIDUnalloc(&iid); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 76506fb..2c66143 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2164,7 +2164,7 @@ xenUnifiedDomainOpenConsole(virDomainPtr dom, }
if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR, false)< 0) + 0, 0, O_RDWR)< 0) goto cleanup;
ret = 0;
On 08/02/2011 01:18 PM, Laine Stump wrote:
On 08/02/2011 01:31 PM, Eric Blake wrote:
Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a race where it can open() a file after the parent process has unlink()d the same file, it makes more sense to make the callers both create and unlink, rather than the caller create and the stream unlink.
I wasn't paying attention to the messages/patches related to the race condition you reference,
Commit 1eb66479 plugged the race; commit 6a1f5f5 introduced the race in the first place. The problem was that if we use libvirt_iohelper, and the child process calls open(), but the parent process calls unlink() before the child process gets to run very far, then the child process will fail to open(). But by changing fdstream to pass the fd to libvirt-iohelper by fd inheritance instead of by name-wise open() calls, there is no longer an open() race, so we can once again unlink() in the parent.
but this (caller creates and unlinks) definitely seems cleaner than the other way. Beyond that, the patch seems to be correct. ACK.
Should this go in for 0.9.4, or am I correct in deferring it until after the release? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/02/2011 01:29 PM, Eric Blake wrote:
On 08/02/2011 01:18 PM, Laine Stump wrote:
On 08/02/2011 01:31 PM, Eric Blake wrote:
Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a race where it can open() a file after the parent process has unlink()d the same file, it makes more sense to make the callers both create and unlink, rather than the caller create and the stream unlink.
I wasn't paying attention to the messages/patches related to the race condition you reference,
Commit 1eb66479 plugged the race; commit 6a1f5f5 introduced the race in the first place.
The problem was that if we use libvirt_iohelper, and the child process calls open(), but the parent process calls unlink() before the child process gets to run very far, then the child process will fail to open(). But by changing fdstream to pass the fd to libvirt-iohelper by fd inheritance instead of by name-wise open() calls, there is no longer an open() race, so we can once again unlink() in the parent.
but this (caller creates and unlinks) definitely seems cleaner than the other way. Beyond that, the patch seems to be correct. ACK.
Should this go in for 0.9.4, or am I correct in deferring it until after the release?
Laine and I chatted some more on IRC: <eblake> laine: should I push both patches now (or even squash together), or delay the second patch till after the release? <laine> eblake: I don't see a problem with the second patch, I think I like it better. Not knowing the history of the race you mentioned and whether or not this code still touches it, I wouldn't single-handledly say to squash the two together, but if you're comfortable with it, then it looks good to me. <eblake> I'll update the commit message of the second; the race was first solved in 6a1f5f5 by making the parent not unlink() if libvirt_iohelper will also be open()ing the file <eblake> but solving it in that manner required passing delete=true all the way through the fdstream code <eblake> later, I pushed commit 1eb66479, which taught libvirt_iostream to operate on an inherited fd instead of calling open() <eblake> and in so doing, the child no longer needs to unlink() <eblake> so we no longer have the unlink() in parent prior to open()/unlink() in the child, and can get rid of the extra parameter Given that, I went ahead and updated the commit comment, then pushed both patches separately, to make it into 0.9.4. Revert 6a1f5f568f8. Now that libvirt_iohelper takes fds by inheritance rather than by open() (commit 1eb66479), there is no longer a race where the parent can unlink() a file prior to the iohelper open()ing the same file. From there, it makes more sense to have the callers both create and unlink, rather than the caller create and the stream unlink, since the latter was only needed when iohelper had to do the unlink. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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; }
participants (3)
-
Eric Blake -
Laine Stump -
Michal Privoznik