[libvirt] [PATCH] xm_internal.c: remove misleading dead code
This code triggered a warning about possible NULL-dereference. Actually it's more about being self-contradictory, since the NULL-dereference is not possible.
From 796e3a3254cb08fc20bce790997b5787dab51902 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 14 Dec 2009 21:37:54 +0100 Subject: [PATCH] xm_internal.c: remove misleading dead code
* src/xen/xm_internal.c (xenXMConfigGetULong): Remove useless and misleading test (always false) for val->str == NULL before code that always dereferences val->str. "val" comes from virConfGetValue, and at that point, val->str is guaranteed to be non-NULL. (xenXMConfigGetBool): Likewise. --- src/xen/xm_internal.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 3dc655b..944d5d5 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -152,9 +152,6 @@ static int xenXMConfigGetBool(virConnectPtr conn, if (val->type == VIR_CONF_LONG) { *value = val->l ? 1 : 0; } else if (val->type == VIR_CONF_STRING) { - if (!val->str) { - *value = def; - } *value = STREQ(val->str, "1") ? 1 : 0; } else { xenXMError(conn, VIR_ERR_INTERNAL_ERROR, @@ -183,9 +180,6 @@ static int xenXMConfigGetULong(virConnectPtr conn, *value = val->l; } else if (val->type == VIR_CONF_STRING) { char *ret; - if (!val->str) { - *value = def; - } *value = strtol(val->str, &ret, 10); if (ret == val->str) { xenXMError(conn, VIR_ERR_INTERNAL_ERROR, -- 1.6.6.rc2.275.g51e2d
On Mon, Dec 14, 2009 at 09:40:59PM +0100, Jim Meyering wrote:
This code triggered a warning about possible NULL-dereference. Actually it's more about being self-contradictory, since the NULL-dereference is not possible.
From 796e3a3254cb08fc20bce790997b5787dab51902 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 14 Dec 2009 21:37:54 +0100 Subject: [PATCH] xm_internal.c: remove misleading dead code
* src/xen/xm_internal.c (xenXMConfigGetULong): Remove useless and misleading test (always false) for val->str == NULL before code that always dereferences val->str. "val" comes from virConfGetValue, and at that point, val->str is guaranteed to be non-NULL. (xenXMConfigGetBool): Likewise.
It isn't entirely clear to me that the virConf class guarnetees val->str to be non-NULL.
--- src/xen/xm_internal.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 3dc655b..944d5d5 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -152,9 +152,6 @@ static int xenXMConfigGetBool(virConnectPtr conn, if (val->type == VIR_CONF_LONG) { *value = val->l ? 1 : 0; } else if (val->type == VIR_CONF_STRING) { - if (!val->str) { - *value = def; - } *value = STREQ(val->str, "1") ? 1 : 0;
I'd just add in 'else' here instead of removing it
} else { xenXMError(conn, VIR_ERR_INTERNAL_ERROR, @@ -183,9 +180,6 @@ static int xenXMConfigGetULong(virConnectPtr conn, *value = val->l; } else if (val->type == VIR_CONF_STRING) { char *ret; - if (!val->str) { - *value = def; - } *value = strtol(val->str, &ret, 10); if (ret == val->str) { xenXMError(conn, VIR_ERR_INTERNAL_ERROR, --
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Mon, Dec 14, 2009 at 09:40:59PM +0100, Jim Meyering wrote:
This code triggered a warning about possible NULL-dereference. Actually it's more about being self-contradictory, since the NULL-dereference is not possible.
From 796e3a3254cb08fc20bce790997b5787dab51902 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 14 Dec 2009 21:37:54 +0100 Subject: [PATCH] xm_internal.c: remove misleading dead code
* src/xen/xm_internal.c (xenXMConfigGetULong): Remove useless and misleading test (always false) for val->str == NULL before code that always dereferences val->str. "val" comes from virConfGetValue, and at that point, val->str is guaranteed to be non-NULL. (xenXMConfigGetBool): Likewise.
It isn't entirely clear to me that the virConf class guarnetees val->str to be non-NULL.
virConfParseValue ensures that non val->str it produces is never NULL. As you pointed out, there's still the possibility that some internal caller of virConfSetValue could mistakenly introduce an invalid entry. How about amending my patch with this addition to prevent that? diff --git a/src/util/conf.c b/src/util/conf.c index 0c7e556..60cf0b4 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -858,6 +858,9 @@ virConfSetValue (virConfPtr conf, { virConfEntryPtr cur, prev = NULL; + if (value && value->type == VIR_CONF_STRING && value->str == NULL) + return -1; + cur = conf->entries; while (cur != NULL) { if ((cur->name != NULL) && (STREQ(cur->name, setting))) {
On Tue, Dec 15, 2009 at 04:49:27PM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Mon, Dec 14, 2009 at 09:40:59PM +0100, Jim Meyering wrote:
This code triggered a warning about possible NULL-dereference. Actually it's more about being self-contradictory, since the NULL-dereference is not possible.
From 796e3a3254cb08fc20bce790997b5787dab51902 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 14 Dec 2009 21:37:54 +0100 Subject: [PATCH] xm_internal.c: remove misleading dead code
* src/xen/xm_internal.c (xenXMConfigGetULong): Remove useless and misleading test (always false) for val->str == NULL before code that always dereferences val->str. "val" comes from virConfGetValue, and at that point, val->str is guaranteed to be non-NULL. (xenXMConfigGetBool): Likewise.
It isn't entirely clear to me that the virConf class guarnetees val->str to be non-NULL.
virConfParseValue ensures that non val->str it produces is never NULL.
As you pointed out, there's still the possibility that some internal caller of virConfSetValue could mistakenly introduce an invalid entry. How about amending my patch with this addition to prevent that?
diff --git a/src/util/conf.c b/src/util/conf.c index 0c7e556..60cf0b4 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -858,6 +858,9 @@ virConfSetValue (virConfPtr conf, { virConfEntryPtr cur, prev = NULL;
+ if (value && value->type == VIR_CONF_STRING && value->str == NULL) + return -1; + cur = conf->entries; while (cur != NULL) { if ((cur->name != NULL) && (STREQ(cur->name, setting))) {
Yeah, that looks like a good safety net Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Dec 14, 2009 at 09:40:59PM +0100, Jim Meyering wrote:
This code triggered a warning about possible NULL-dereference. Actually it's more about being self-contradictory, since the NULL-dereference is not possible.
From 796e3a3254cb08fc20bce790997b5787dab51902 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 14 Dec 2009 21:37:54 +0100 Subject: [PATCH] xm_internal.c: remove misleading dead code
* src/xen/xm_internal.c (xenXMConfigGetULong): Remove useless and misleading test (always false) for val->str == NULL before code that always dereferences val->str. "val" comes from virConfGetValue, and at that point, val->str is guaranteed to be non-NULL. (xenXMConfigGetBool): Likewise. --- src/xen/xm_internal.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 3dc655b..944d5d5 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -152,9 +152,6 @@ static int xenXMConfigGetBool(virConnectPtr conn, if (val->type == VIR_CONF_LONG) { *value = val->l ? 1 : 0; } else if (val->type == VIR_CONF_STRING) { - if (!val->str) { - *value = def; - } *value = STREQ(val->str, "1") ? 1 : 0; } else { xenXMError(conn, VIR_ERR_INTERNAL_ERROR, @@ -183,9 +180,6 @@ static int xenXMConfigGetULong(virConnectPtr conn, *value = val->l; } else if (val->type == VIR_CONF_STRING) { char *ret; - if (!val->str) { - *value = def; - } *value = strtol(val->str, &ret, 10); if (ret == val->str) { xenXMError(conn, VIR_ERR_INTERNAL_ERROR,
In any case it looks like superfluous code since the next statement overrides the target value ... so it's at best useless. I bet it's old code that we forgot to remove. 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 (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering