[libvirt] [PATCH] conf: Fix crash with cleanup
There was a crash possible when both <boot dev... and <boot order... were specified due to virDomainDefParseBootXML() erroring out before setting *tmp (which was free'd in cleanup). As a fix, I created this cleanup that uses one pointer for all the temporary stored XPath strings and values, plus this pointer is correctly initialized to NULL. --- src/conf/domain_conf.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0cea8eb..f0c5d50 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8242,8 +8242,7 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, { xmlNodePtr *nodes = NULL; int i, n; - char *bootstr, *tmp; - char *useserial = NULL; + char *tmp = NULL; int ret = -1; unsigned long deviceBoot, serialPorts; @@ -8290,23 +8289,23 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK; } - bootstr = virXPathString("string(./os/bootmenu[1]/@enable)", ctxt); - if (bootstr) { - def->os.bootmenu = virDomainBootMenuTypeFromString(bootstr); + tmp = virXPathString("string(./os/bootmenu[1]/@enable)", ctxt); + if (tmp) { + def->os.bootmenu = virDomainBootMenuTypeFromString(tmp); if (def->os.bootmenu <= 0) { /* In order not to break misconfigured machines, this * should not emit an error, but rather set the bootmenu * to disabled */ VIR_WARN("disabling bootmenu due to unknown option '%s'", - bootstr); + tmp); def->os.bootmenu = VIR_DOMAIN_BOOT_MENU_DISABLED; } - VIR_FREE(bootstr); + VIR_FREE(tmp); } - useserial = virXPathString("string(./os/bios[1]/@useserial)", ctxt); - if (useserial) { - if (STREQ(useserial, "yes")) { + tmp = virXPathString("string(./os/bios[1]/@useserial)", ctxt); + if (tmp) { + if (STREQ(tmp, "yes")) { if (virXPathULong("count(./devices/serial)", ctxt, &serialPorts) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -8318,6 +8317,7 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, } else { def->os.bios.useserial = VIR_DOMAIN_BIOS_USESERIAL_NO; } + VIR_FREE(tmp); } tmp = virXPathString("string(./os/bios[1]/@rebootTimeout)", ctxt); @@ -8339,7 +8339,6 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, cleanup: VIR_FREE(tmp); - VIR_FREE(useserial); VIR_FREE(nodes); return ret; } -- 1.7.12.3
On 16.10.2012 11:19, Martin Kletzander wrote:
There was a crash possible when both <boot dev... and <boot order... were specified due to virDomainDefParseBootXML() erroring out before setting *tmp (which was free'd in cleanup). As a fix, I created this cleanup that uses one pointer for all the temporary stored XPath strings and values, plus this pointer is correctly initialized to NULL. --- src/conf/domain_conf.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
ACK Michal
On 10/16/2012 11:48 AM, Michal Privoznik wrote:
On 16.10.2012 11:19, Martin Kletzander wrote:
There was a crash possible when both <boot dev... and <boot order... were specified due to virDomainDefParseBootXML() erroring out before setting *tmp (which was free'd in cleanup). As a fix, I created this cleanup that uses one pointer for all the temporary stored XPath strings and values, plus this pointer is correctly initialized to NULL. --- src/conf/domain_conf.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
ACK
Michal
Thanks, pushed. Martin
On Tue, Oct 16, 2012 at 11:19:55AM +0200, Martin Kletzander wrote:
There was a crash possible when both <boot dev... and <boot order... were specified due to virDomainDefParseBootXML() erroring out before setting *tmp (which was free'd in cleanup). As a fix, I created this cleanup that uses one pointer for all the temporary stored XPath strings and values, plus this pointer is correctly initialized to NULL. --- src/conf/domain_conf.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
It would be nice to have a test to check for this scenario 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 10/16/2012 11:50 AM, Daniel P. Berrange wrote:
On Tue, Oct 16, 2012 at 11:19:55AM +0200, Martin Kletzander wrote:
There was a crash possible when both <boot dev... and <boot order... were specified due to virDomainDefParseBootXML() erroring out before setting *tmp (which was free'd in cleanup). As a fix, I created this cleanup that uses one pointer for all the temporary stored XPath strings and values, plus this pointer is correctly initialized to NULL. --- src/conf/domain_conf.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
It would be nice to have a test to check for this scenario
Daniel
Good point, patch follows. Martin
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik