[libvirt] [PATCH] xenDaemonDomainSetAutostart: avoid appearance of impropriety
Coverity noticed that of the 13 uses of sexpr_lookup, only this one was unchecked, and here, the result is dereferenced. It happens to be a false positive, due to the preceding sexpr_node check, but worth fixing: sexpr_node is just a very thin wrapper around sexpr_lookup so there's little justification for looking up the same string twice.
From a9ab34214cf9d247d39731563dcc70b8f1dc73b5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 17 Feb 2010 22:14:25 +0100 Subject: [PATCH] xenDaemonDomainSetAutostart: avoid appearance of impropriety
* src/xen/xend_internal.c (xenDaemonDomainSetAutostart): Rewrite to avoid dereferencing the result of sexpr_lookup. While in this particular case, it was guaranteed never to be NULL, due to the preceding "if sexpr_node(...)" guard, it's cleaner to skip the sexpr_node call altogether, and also saves a lookup. --- src/xen/xend_internal.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 88923c8..1f8b106 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4383,7 +4383,6 @@ xenDaemonDomainSetAutostart(virDomainPtr domain, int autostart) { struct sexpr *root, *autonode; - const char *autostr; char buf[4096]; int ret = -1; xenUnifiedPrivatePtr priv; @@ -4408,16 +4407,17 @@ xenDaemonDomainSetAutostart(virDomainPtr domain, return (-1); } - autostr = sexpr_node(root, "domain/on_xend_start"); - if (autostr) { - if (!STREQ(autostr, "ignore") && !STREQ(autostr, "start")) { + autonode = sexpr_lookup(root, "domain/on_xend_start"); + if (autonode) { + const char *val = (autonode->u.s.car->kind == SEXPR_VALUE + ? autonode->u.s.car->u.value : NULL); + if (!STREQ(val, "ignore") && !STREQ(val, "start")) { virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected value from on_xend_start")); goto error; } // Change the autostart value in place, then define the new sexpr - autonode = sexpr_lookup(root, "domain/on_xend_start"); VIR_FREE(autonode->u.s.car->u.value); autonode->u.s.car->u.value = (autostart ? strdup("start") : strdup("ignore")); -- 1.7.0.219.g6bb57
On Thu, Feb 18, 2010 at 07:20:17AM +0100, Jim Meyering wrote:
Coverity noticed that of the 13 uses of sexpr_lookup, only this one was unchecked, and here, the result is dereferenced. It happens to be a false positive, due to the preceding sexpr_node check, but worth fixing: sexpr_node is just a very thin wrapper around sexpr_lookup so there's little justification for looking up the same string twice.
From a9ab34214cf9d247d39731563dcc70b8f1dc73b5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 17 Feb 2010 22:14:25 +0100 Subject: [PATCH] xenDaemonDomainSetAutostart: avoid appearance of impropriety
* src/xen/xend_internal.c (xenDaemonDomainSetAutostart): Rewrite to avoid dereferencing the result of sexpr_lookup. While in this particular case, it was guaranteed never to be NULL, due to the preceding "if sexpr_node(...)" guard, it's cleaner to skip the sexpr_node call altogether, and also saves a lookup. --- src/xen/xend_internal.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 88923c8..1f8b106 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4383,7 +4383,6 @@ xenDaemonDomainSetAutostart(virDomainPtr domain, int autostart) { struct sexpr *root, *autonode; - const char *autostr; char buf[4096]; int ret = -1; xenUnifiedPrivatePtr priv; @@ -4408,16 +4407,17 @@ xenDaemonDomainSetAutostart(virDomainPtr domain, return (-1); }
- autostr = sexpr_node(root, "domain/on_xend_start"); - if (autostr) { - if (!STREQ(autostr, "ignore") && !STREQ(autostr, "start")) { + autonode = sexpr_lookup(root, "domain/on_xend_start"); + if (autonode) { + const char *val = (autonode->u.s.car->kind == SEXPR_VALUE + ? autonode->u.s.car->u.value : NULL); + if (!STREQ(val, "ignore") && !STREQ(val, "start")) { virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected value from on_xend_start")); goto error; }
// Change the autostart value in place, then define the new sexpr - autonode = sexpr_lookup(root, "domain/on_xend_start"); VIR_FREE(autonode->u.s.car->u.value); autonode->u.s.car->u.value = (autostart ? strdup("start") : strdup("ignore"));
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel Veillard -
Jim Meyering