[libvirt] [PATCH] virsh: Correctly initialize libvirt
virsh didn't call virInitialize(), which (among other things) initializes virLastErr thread local variable. As a result of that, virsh could just segfault in virEventRegisterDefaultImpl() since that is the first call that touches (resets) virLastErr. I have no idea what lucky coincidence made this bug visible but I was able to reproduce it in 100% cases but only in one specific environment which included building in sandbox. --- tools/virsh.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 2b16714..08e6905 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12819,14 +12819,20 @@ main(int argc, char **argv) char *defaultConn; bool ret = true; + memset(ctl, 0, sizeof(vshControl)); + ctl->imode = true; /* default is interactive mode */ + ctl->log_fd = -1; /* Initialize log file descriptor */ + if (!setlocale(LC_ALL, "")) { perror("setlocale"); /* failure to setup locale is not fatal */ } - if (!bindtextdomain(PACKAGE, LOCALEDIR)) { - perror("bindtextdomain"); + + if (virInitialize() < 0) { + vshError(ctl, "%s", _("Failed to initialize libvirt")); return EXIT_FAILURE; } + if (!textdomain(PACKAGE)) { perror("textdomain"); return EXIT_FAILURE; @@ -12837,10 +12843,6 @@ main(int argc, char **argv) else progname++; - memset(ctl, 0, sizeof(vshControl)); - ctl->imode = true; /* default is interactive mode */ - ctl->log_fd = -1; /* Initialize log file descriptor */ - if ((defaultConn = getenv("VIRSH_DEFAULT_CONNECT_URI"))) { ctl->name = vshStrdup(ctl, defaultConn); } -- 1.7.5.rc3
2011/5/9 Jiri Denemark <jdenemar@redhat.com>:
virsh didn't call virInitialize(), which (among other things) initializes virLastErr thread local variable. As a result of that, virsh could just segfault in virEventRegisterDefaultImpl() since that is the first call that touches (resets) virLastErr.
I have no idea what lucky coincidence made this bug visible but I was able to reproduce it in 100% cases but only in one specific environment which included building in sandbox.
Well, actually all public API that can be called as the first public API function ever called in a sound program calls virInitialize first internally. For example virConnectOpen. The documentation of virInitialize suggest to call it first, but doesn't require it. A fix in line with the current behavior would be to make virEventRegisterDefaultImpl and virEventRunDefaultImpl call virInitialize first too. But this doesn't fix another problem, a program could call virDomainFree(NULL) for some reason as the first public API call ever made in the process and libvirt would try to report an error but would segfault because of calling virResetLastError before virInitialize was called. So you found a much bigger problem and I'm not sure about the correct general solution. We could make calling virInitialize first mandatory but that might break existing applications. Or we make all public API functions call virInitialize first.
--- tools/virsh.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 2b16714..08e6905 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12819,14 +12819,20 @@ main(int argc, char **argv) char *defaultConn; bool ret = true;
+ memset(ctl, 0, sizeof(vshControl)); + ctl->imode = true; /* default is interactive mode */ + ctl->log_fd = -1; /* Initialize log file descriptor */ + if (!setlocale(LC_ALL, "")) { perror("setlocale"); /* failure to setup locale is not fatal */ } - if (!bindtextdomain(PACKAGE, LOCALEDIR)) { - perror("bindtextdomain");
Why did you remove the call to bindtextdomain? Matthias
On Mon, May 09, 2011 at 04:04:41PM +0200, Matthias Bolte wrote:
2011/5/9 Jiri Denemark <jdenemar@redhat.com>:
virsh didn't call virInitialize(), which (among other things) initializes virLastErr thread local variable. As a result of that, virsh could just segfault in virEventRegisterDefaultImpl() since that is the first call that touches (resets) virLastErr.
I have no idea what lucky coincidence made this bug visible but I was able to reproduce it in 100% cases but only in one specific environment which included building in sandbox.
Well, actually all public API that can be called as the first public API function ever called in a sound program calls virInitialize first internally. For example virConnectOpen. The documentation of virInitialize suggest to call it first, but doesn't require it.
A fix in line with the current behavior would be to make virEventRegisterDefaultImpl and virEventRunDefaultImpl call virInitialize first too.
Yep that's good.
But this doesn't fix another problem, a program could call virDomainFree(NULL) for some reason as the first public API call ever made in the process and libvirt would try to report an error but would segfault because of calling virResetLastError before virInitialize was called.
IMHO that would be a mis-use of the API because they should call virInitialize.
So you found a much bigger problem and I'm not sure about the correct general solution. We could make calling virInitialize first mandatory but that might break existing applications. Or we make all public API functions call virInitialize first.
I think we could just clarify the docs "Calling virInitialize is mandatory, unless your first API call is one of virConnectOpen* virEventRegisterDefaultImpl" Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, May 10, 2011 at 10:53:40 +0100, Daniel P. Berrange wrote:
On Mon, May 09, 2011 at 04:04:41PM +0200, Matthias Bolte wrote:
2011/5/9 Jiri Denemark <jdenemar@redhat.com>:
virsh didn't call virInitialize(), which (among other things) initializes virLastErr thread local variable. As a result of that, virsh could just segfault in virEventRegisterDefaultImpl() since that is the first call that touches (resets) virLastErr.
I have no idea what lucky coincidence made this bug visible but I was able to reproduce it in 100% cases but only in one specific environment which included building in sandbox.
Well, actually all public API that can be called as the first public API function ever called in a sound program calls virInitialize first internally. For example virConnectOpen. The documentation of virInitialize suggest to call it first, but doesn't require it.
A fix in line with the current behavior would be to make virEventRegisterDefaultImpl and virEventRunDefaultImpl call virInitialize first too.
Yep that's good.
I looked at this and I'm not sure it's worth it :-) We would need to move the two functions from util/event.c to libvirt.c, which on one hand makes sense since they are public APIs but on the other hand it's kind of nice that we have event related functions in event.c. Moreover, I'm not a big fan of making more APIs special in that they automagically call virInitialize, esp. since one may ask why virEventRegisterDefaultImpl is special but virEventRegisterImpl is not? And why other APIs are not special?
But this doesn't fix another problem, a program could call virDomainFree(NULL) for some reason as the first public API call ever made in the process and libvirt would try to report an error but would segfault because of calling virResetLastError before virInitialize was called.
IMHO that would be a mis-use of the API because they should call virInitialize.
So you found a much bigger problem and I'm not sure about the correct general solution. We could make calling virInitialize first mandatory but that might break existing applications. Or we make all public API functions call virInitialize first.
I think we could just clarify the docs
"Calling virInitialize is mandatory, unless your first API call is one of
virConnectOpen* virEventRegisterDefaultImpl"
I think leaving just virConnectOpen* APIs special should be enough. In more advanced code that doesn't just connect and do some stuff, there's no harm if we require users to call virInitialize first. So the following docs clarification would solve it: "Calling virInitialize is mandatory, unless your first API call is one of virConnectOpen*" Jirka
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Matthias Bolte