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@xxxxxxxxxx>
> 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list