Re: [libvirt] Availability of Release Candidate 3 for 1.0.0
Hi, I was invited to show up here regarding BZ 871756. I'm the maintainer of libvirt on Alpine Linux [1], an uclibc-based distro. If you need more info, testing, etc. just let me know. Cheers, leonardo P.S. Sorry if I've broken the thread [1] http://alpinelinux.org/
On 10/31/2012 01:29 PM, Leonardo Arena wrote:
Hi,
I was invited to show up here regarding BZ 871756. I'm the maintainer of libvirt on Alpine Linux [1], an uclibc-based distro.
Hi, thanks for looking at it. It's good to hear there is only one function missing when building on uclibc. Although it's a pity there is no replacement for that in uclibc, there is in gnulib (which we use, anyway) and I think that adding a gnulib module would be safer, but probably I'm not the best one to decide upon that. Anyone else to correct me? In case anyone wants to have a look, here are the links to ease the search: Bug: https://bugzilla.redhat.com/show_bug.cgi?id=871756 Diff: https://bugzilla.redhat.com/attachment.cgi?id=636010
If you need more info, testing, etc. just let me know.
If you could try that with the gnulib module and send it as a patch, we could maybe have it in 1.0.0. Martin
Cheers, leonardo
P.S. Sorry if I've broken the thread
On 10/31/2012 06:29 AM, Leonardo Arena wrote:
Hi,
I was invited to show up here regarding BZ 871756. I'm the maintainer of libvirt on Alpine Linux [1], an uclibc-based distro.
If you need more info, testing, etc. just let me know.
--- a/src/util/logging.c +++ b/src/util/logging.c @@ -58,6 +58,11 @@
#define VIR_FROM_THIS VIR_FROM_NONE
+#ifdef __UCLIBC__ +/* uclibc does not implement mkostemp GNU extention */ +#define mkostemp(x,y) mkstemp(x) +#endif + VIR_ENUM_DECL(virLogSource) VIR_ENUM_IMPL(virLogSource, VIR_LOG_FROM_LAST, "file",
NACK. Rather, we should be using gnulib's mkostemp - I'm working on the patch, and will post it shortly. If you could test my version, that would be much appreciated. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
https://bugzilla.redhat.com/show_bug.cgi?id=871756 Commit cd1e8d1 assumed that systems new enough to have journald also have mkostemp; but this is not true for uclibc. For that matter, use of mkstemp[s] is unsafe in a multi-threaded program. We should prefer mkostemp[s] in the first place. * bootstrap.conf (gnulib_modules): Add mkostemp, mkostemps; drop mkstemp and mkstemps. * cfg.mk (sc_prohibit_mkstemp): New syntax check. * tools/virsh.c (vshEditWriteToTempFile): Adjust caller. * src/qemu/qemu_driver.c (qemuDomainScreenshot) (qemudDomainMemoryPeek): Likewise. * src/secret/secret_driver.c (replaceFile): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise. --- bootstrap.conf | 4 ++-- cfg.mk | 6 ++++++ src/qemu/qemu_driver.c | 8 ++++---- src/secret/secret_driver.c | 4 ++-- src/vbox/vbox_tmpl.c | 4 ++-- tools/virsh.c | 2 +- 6 files changed, 17 insertions(+), 11 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 5d391fd..59dd258 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -69,8 +69,8 @@ listen localeconv maintainer-makefile manywarnings -mkstemp -mkstemps +mkostemp +mkostemps mktempd net_if netdb diff --git a/cfg.mk b/cfg.mk index 50e6a50..cda04e4 100644 --- a/cfg.mk +++ b/cfg.mk @@ -339,6 +339,12 @@ sc_prohibit_fork_wrappers: halt='use virCommand for child processes' \ $(_sc_search_regexp) +# Prefer mkostemp with O_CLOEXEC. +sc_prohibit_mkstemp: + @prohibit='[^"]\<mkstemps? *\(' \ + halt='use mkostemp with O_CLOEXEC instead of mkstemp' \ + $(_sc_search_regexp) + # access with X_OK accepts directories, but we can't exec() those. # access with F_OK or R_OK is okay, though. sc_prohibit_access_xok: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3980c10..5baa1e7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3485,8 +3485,8 @@ qemuDomainScreenshot(virDomainPtr dom, goto endjob; } - if ((tmp_fd = mkstemp(tmp)) == -1) { - virReportSystemError(errno, _("mkstemp(\"%s\") failed"), tmp); + if ((tmp_fd = mkostemp(tmp, O_CLOEXEC)) == -1) { + virReportSystemError(errno, _("mkostemp(\"%s\") failed"), tmp); goto endjob; } unlink_tmp = true; @@ -9230,9 +9230,9 @@ qemudDomainMemoryPeek (virDomainPtr dom, } /* Create a temporary filename. */ - if ((fd = mkstemp (tmp)) == -1) { + if ((fd = mkostemp(tmp, O_CLOEXEC)) == -1) { virReportSystemError(errno, - _("mkstemp(\"%s\") failed"), tmp); + _("mkostemp(\"%s\") failed"), tmp); goto endjob; } diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 9ce1e33..51e1e46 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -171,9 +171,9 @@ replaceFile(const char *filename, void *data, size_t size) virReportOOMError(); goto cleanup; } - fd = mkstemp (tmp_path); + fd = mkostemp(tmp_path, O_CLOEXEC); if (fd == -1) { - virReportSystemError(errno, _("mkstemp('%s') failed"), tmp_path); + virReportSystemError(errno, _("mkostemp('%s') failed"), tmp_path); goto cleanup; } if (fchmod(fd, S_IRUSR | S_IWUSR) != 0) { diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 32a903e..6f245da 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -9157,8 +9157,8 @@ vboxDomainScreenshot(virDomainPtr dom, return NULL; } - if ((tmp_fd = mkstemp(tmp)) == -1) { - virReportSystemError(errno, _("mkstemp(\"%s\") failed"), tmp); + if ((tmp_fd = mkostemp(tmp, O_CLOEXEC)) == -1) { + virReportSystemError(errno, _("mkostemp(\"%s\") failed"), tmp); VIR_FREE(tmp); VBOX_RELEASE(machine); return NULL; diff --git a/tools/virsh.c b/tools/virsh.c index f0ec625..5388c9e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -565,7 +565,7 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc) vshError(ctl, "%s", _("out of memory")); return NULL; } - fd = mkstemps(ret, 4); + fd = mkostemps(ret, 4, O_CLOEXEC); if (fd == -1) { vshError(ctl, _("mkstemps: failed to create temporary file: %s"), virStrerror(errno, ebuf, sizeof(ebuf))); -- 1.7.11.7
On 10/31/2012 03:42 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=871756
Commit cd1e8d1 assumed that systems new enough to have journald also have mkostemp; but this is not true for uclibc.
For that matter, use of mkstemp[s] is unsafe in a multi-threaded program. We should prefer mkostemp[s] in the first place.
* bootstrap.conf (gnulib_modules): Add mkostemp, mkostemps; drop mkstemp and mkstemps. * cfg.mk (sc_prohibit_mkstemp): New syntax check. * tools/virsh.c (vshEditWriteToTempFile): Adjust caller. * src/qemu/qemu_driver.c (qemuDomainScreenshot) (qemudDomainMemoryPeek): Likewise. * src/secret/secret_driver.c (replaceFile): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise. --- bootstrap.conf | 4 ++-- cfg.mk | 6 ++++++ src/qemu/qemu_driver.c | 8 ++++---- src/secret/secret_driver.c | 4 ++-- src/vbox/vbox_tmpl.c | 4 ++-- tools/virsh.c | 2 +- 6 files changed, 17 insertions(+), 11 deletions(-)
[...]
diff --git a/tools/virsh.c b/tools/virsh.c index f0ec625..5388c9e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -565,7 +565,7 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc) vshError(ctl, "%s", _("out of memory")); return NULL; } - fd = mkstemps(ret, 4); + fd = mkostemps(ret, 4, O_CLOEXEC); if (fd == -1) { vshError(ctl, _("mkstemps: failed to create temporary file: %s"),
This message should be changed as well.
virStrerror(errno, ebuf, sizeof(ebuf)));
ACK with that changed. Martin
On 10/31/2012 09:45 AM, Martin Kletzander wrote:
On 10/31/2012 03:42 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=871756
Commit cd1e8d1 assumed that systems new enough to have journald also have mkostemp; but this is not true for uclibc.
For that matter, use of mkstemp[s] is unsafe in a multi-threaded program. We should prefer mkostemp[s] in the first place.
- fd = mkstemps(ret, 4); + fd = mkostemps(ret, 4, O_CLOEXEC); if (fd == -1) { vshError(ctl, _("mkstemps: failed to create temporary file: %s"),
This message should be changed as well.
virStrerror(errno, ebuf, sizeof(ebuf)));
ACK with that changed.
Fixed and pushed. Hopefully Leonardo can give libvirt.git a test (since we probably won't have any more rc builds between now and 1.0.0 on Friday). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, 2012-10-31 at 10:10 -0600, Eric Blake wrote:
On 10/31/2012 09:45 AM, Martin Kletzander wrote:
On 10/31/2012 03:42 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=871756
Commit cd1e8d1 assumed that systems new enough to have journald also have mkostemp; but this is not true for uclibc.
For that matter, use of mkstemp[s] is unsafe in a multi-threaded program. We should prefer mkostemp[s] in the first place.
- fd = mkstemps(ret, 4); + fd = mkostemps(ret, 4, O_CLOEXEC); if (fd == -1) { vshError(ctl, _("mkstemps: failed to create temporary file: %s"),
This message should be changed as well.
virStrerror(errno, ebuf, sizeof(ebuf)));
ACK with that changed.
Fixed and pushed. Hopefully Leonardo can give libvirt.git a test (since we probably won't have any more rc builds between now and 1.0.0 on Friday).
Unfortunately I'm having some issues in making the distribution, due to automake affected by CVE-2012-3386, and bootstrapping in Alpine (with fixed automake) has some other issues. If there's anyone that can send me a tarball, I'll test it asap. Thank you -leonardo
On 11/01/2012 02:32 PM, Leonardo Arena wrote:
On Wed, 2012-10-31 at 10:10 -0600, Eric Blake wrote:
On 10/31/2012 09:45 AM, Martin Kletzander wrote:
On 10/31/2012 03:42 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=871756
Commit cd1e8d1 assumed that systems new enough to have journald also have mkostemp; but this is not true for uclibc.
For that matter, use of mkstemp[s] is unsafe in a multi-threaded program. We should prefer mkostemp[s] in the first place.
- fd = mkstemps(ret, 4); + fd = mkostemps(ret, 4, O_CLOEXEC); if (fd == -1) { vshError(ctl, _("mkstemps: failed to create temporary file: %s"),
This message should be changed as well.
virStrerror(errno, ebuf, sizeof(ebuf)));
ACK with that changed.
Fixed and pushed. Hopefully Leonardo can give libvirt.git a test (since we probably won't have any more rc builds between now and 1.0.0 on Friday).
Unfortunately I'm having some issues in making the distribution, due to automake affected by CVE-2012-3386, and bootstrapping in Alpine (with fixed automake) has some other issues. If there's anyone that can send me a tarball, I'll test it asap.
Thank you
-leonardo
Feel free to grab this one: http://people.redhat.com/mkletzan/libvirt-0.10.2.tar.gz I hope doing 'make dist' was enough =) Martin
On Thu, 2012-11-01 at 14:51 +0100, Martin Kletzander wrote:
On 11/01/2012 02:32 PM, Leonardo Arena wrote:
On Wed, 2012-10-31 at 10:10 -0600, Eric Blake wrote:
On 10/31/2012 09:45 AM, Martin Kletzander wrote:
On 10/31/2012 03:42 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=871756
Commit cd1e8d1 assumed that systems new enough to have journald also have mkostemp; but this is not true for uclibc.
For that matter, use of mkstemp[s] is unsafe in a multi-threaded program. We should prefer mkostemp[s] in the first place.
- fd = mkstemps(ret, 4); + fd = mkostemps(ret, 4, O_CLOEXEC); if (fd == -1) { vshError(ctl, _("mkstemps: failed to create temporary file: %s"),
This message should be changed as well.
virStrerror(errno, ebuf, sizeof(ebuf)));
ACK with that changed.
Fixed and pushed. Hopefully Leonardo can give libvirt.git a test (since we probably won't have any more rc builds between now and 1.0.0 on Friday).
Unfortunately I'm having some issues in making the distribution, due to automake affected by CVE-2012-3386, and bootstrapping in Alpine (with fixed automake) has some other issues. If there's anyone that can send me a tarball, I'll test it asap.
Thank you
-leonardo
Feel free to grab this one: http://people.redhat.com/mkletzan/libvirt-0.10.2.tar.gz
I hope doing 'make dist' was enough =)
Martin
It worked. I've closed BZ871756. Thanks to all. - leonardo
On 11/01/2012 04:53 PM, Leonardo Arena wrote:
On Thu, 2012-11-01 at 14:51 +0100, Martin Kletzander wrote:
On 11/01/2012 02:32 PM, Leonardo Arena wrote:
On Wed, 2012-10-31 at 10:10 -0600, Eric Blake wrote:
On 10/31/2012 09:45 AM, Martin Kletzander wrote:
On 10/31/2012 03:42 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=871756
Commit cd1e8d1 assumed that systems new enough to have journald also have mkostemp; but this is not true for uclibc.
For that matter, use of mkstemp[s] is unsafe in a multi-threaded program. We should prefer mkostemp[s] in the first place.
- fd = mkstemps(ret, 4); + fd = mkostemps(ret, 4, O_CLOEXEC); if (fd == -1) { vshError(ctl, _("mkstemps: failed to create temporary file: %s"),
This message should be changed as well.
virStrerror(errno, ebuf, sizeof(ebuf)));
ACK with that changed.
Fixed and pushed. Hopefully Leonardo can give libvirt.git a test (since we probably won't have any more rc builds between now and 1.0.0 on Friday).
Unfortunately I'm having some issues in making the distribution, due to automake affected by CVE-2012-3386, and bootstrapping in Alpine (with fixed automake) has some other issues. If there's anyone that can send me a tarball, I'll test it asap.
Thank you
-leonardo
Feel free to grab this one: http://people.redhat.com/mkletzan/libvirt-0.10.2.tar.gz
I hope doing 'make dist' was enough =)
Martin
It worked. I've closed BZ871756.
Thanks to all.
- leonardo
And thanks to you as well. Let us know how the 1.0.0 turns out as it is already released. Have a nice day, Martin
On Fri, 2012-11-02 at 09:08 +0100, Martin Kletzander wrote:
On 11/01/2012 04:53 PM, Leonardo Arena wrote:
On Thu, 2012-11-01 at 14:51 +0100, Martin Kletzander wrote:
On 11/01/2012 02:32 PM, Leonardo Arena wrote:
On Wed, 2012-10-31 at 10:10 -0600, Eric Blake wrote:
On 10/31/2012 09:45 AM, Martin Kletzander wrote:
On 10/31/2012 03:42 PM, Eric Blake wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=871756 > > Commit cd1e8d1 assumed that systems new enough to have journald > also have mkostemp; but this is not true for uclibc. > > For that matter, use of mkstemp[s] is unsafe in a multi-threaded > program. We should prefer mkostemp[s] in the first place.
> - fd = mkstemps(ret, 4); > + fd = mkostemps(ret, 4, O_CLOEXEC); > if (fd == -1) { > vshError(ctl, _("mkstemps: failed to create temporary file: %s"),
This message should be changed as well.
> virStrerror(errno, ebuf, sizeof(ebuf))); >
ACK with that changed.
Fixed and pushed. Hopefully Leonardo can give libvirt.git a test (since we probably won't have any more rc builds between now and 1.0.0 on Friday).
Unfortunately I'm having some issues in making the distribution, due to automake affected by CVE-2012-3386, and bootstrapping in Alpine (with fixed automake) has some other issues. If there's anyone that can send me a tarball, I'll test it asap.
Thank you
-leonardo
Feel free to grab this one: http://people.redhat.com/mkletzan/libvirt-0.10.2.tar.gz
I hope doing 'make dist' was enough =)
Martin
It worked. I've closed BZ871756.
Thanks to all.
- leonardo
And thanks to you as well. Let us know how the 1.0.0 turns out as it is already released.
I've already pushed it to Alpine "edge" repository this morning. It will be included in Alpine 2.5 release, scheduled next week. Best regards, - leonardo
On 11/02/2012 03:39 AM, Leonardo Arena wrote:
It worked. I've closed BZ871756.
Thanks to all.
- leonardo
And thanks to you as well. Let us know how the 1.0.0 turns out as it is already released.
I've already pushed it to Alpine "edge" repository this morning. It will be included in Alpine 2.5 release, scheduled next week.
Alas, 1.0.0 is slightly broken on Alpine. You need to manually remove the mistaken __UCLIBC__ hunk in src/util/logging.c, so that you are properly using mkostemp from gnulib and not discarding O_CLOEXEC. It has since been fixed in libvirt.git (this regression cost us three additional commits to get straightened out: 30b398d, bd0cb27, d055498). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Nov 2, 2012 at 4:16 PM, Eric Blake <eblake@redhat.com> wrote:
On 11/02/2012 03:39 AM, Leonardo Arena wrote:
It worked. I've closed BZ871756.
Thanks to all.
- leonardo
And thanks to you as well. Let us know how the 1.0.0 turns out as it is already released.
I've already pushed it to Alpine "edge" repository this morning. It will be included in Alpine 2.5 release, scheduled next week.
Alas, 1.0.0 is slightly broken on Alpine. You need to manually remove the mistaken __UCLIBC__ hunk in src/util/logging.c, so that you are properly using mkostemp from gnulib and not discarding O_CLOEXEC. It has since been fixed in libvirt.git (this regression cost us three additional commits to get straightened out: 30b398d, bd0cb27, d055498).
Yes, I realized that. Thanks for pointing it out though. Kind regards, - leonardo
participants (3)
-
Eric Blake -
Leonardo Arena -
Martin Kletzander