1:05 p.m.
A recent Coverity version update discovered some existing issues (and some
benign cases). This patch set cleans them all up.
John Ferlan (6):
VSMS: Coverity cleanups
libxkutil:pool_parsing: Coverity cleanups
libxkutil:device_parsing: Coverity cleanups
libxkutil/xml_parse_test: Coverity cleanup
RAFP: Coverity cleanup
EAFP: Coverity cleanup
libxkutil/device_parsing.c | 41 +++++++++++++++++--------------
libxkutil/pool_parsing.c | 39 +++++++++++++++--------------
libxkutil/xml_parse_test.c | 1 +
src/Virt_ElementAllocatedFromPool.c | 6 ++++-
src/Virt_ResourceAllocationFromPool.c | 7 ++++--
src/Virt_SettingsDefineCapabilities.c | 2 +-
src/Virt_VirtualSystemManagementService.c | 11 ++++++---
7 files changed, 63 insertions(+), 44 deletions(-)
--
1.8.4.2
1:05 p.m.
New subject: [libvirt] [PATCH 1/6] VSMS: Coverity cleanups
A new version of Coverity found a number of issues:
parse_ip_address(): FORWARD_NULL
- Benign issue regarding how 'tmp_ip' was compared against NULL for
the IPv6 processing and then used blindly later when strdup()'ing
into *ip. Rather than use NULL check, compare against return of 1
or more which indicates that something is there
update_system_settings(): RESOURCE_LEAK
- The 'uuid' value was being leaked if strdup()'d. Also rather than
strdup()'g and strdup()'d value and risking failure, just assign the
initially strdup()'d value and reinitialize uuid to NULL
fv_vssd_to_domain(): USE_AFTER_FREE
- The domain->os_info.fv.arch is free()'d only to be potentially
strdup()'d after processing the 'cu_get_str_prop()' for "Arch".
The complaint was that it was possible to not strdup() a new value
and thus possible to pass a free()'d value to get_default_machine().
Passing a NULL is not an issue as that is checked.
Additionally found by inspection, 'val' was not initialized to NULL,
so the setting of os_info.fv.arch may not be what was expected. Also,
after processing "Arch" it was not reinitialized to NULL so its
contents could potentially have been saved in os_info.fv.machine.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/Virt_VirtualSystemManagementService.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/Virt_VirtualSystemManagementService.c
b/src/Virt_VirtualSystemManagementService.c
index 5c7238f..b624d8c 100644
--- a/src/Virt_VirtualSystemManagementService.c
+++ b/src/Virt_VirtualSystemManagementService.c
@@ -464,7 +464,7 @@ static int fv_vssd_to_domain(CMPIInstance *inst,
{
int ret = 1;
int retr;
- const char *val;
+ const char *val = NULL;
const char *domtype = NULL;
const char *ostype = "hvm";
struct capabilities *capsinfo = NULL;
@@ -494,6 +494,7 @@ static int fv_vssd_to_domain(CMPIInstance *inst,
}
free(domain->os_info.fv.arch);
+ domain->os_info.fv.arch = NULL;
retr = cu_get_str_prop(inst, "Arch", &val);
if (retr != CMPI_RC_OK) {
if (capsinfo != NULL) { /* set default */
@@ -506,6 +507,8 @@ static int fv_vssd_to_domain(CMPIInstance *inst,
domain->os_info.fv.arch = strdup(val);
free(domain->os_info.fv.machine);
+ domain->os_info.fv.machine = NULL;
+ val = NULL;
retr = cu_get_str_prop(inst, "Machine", &val);
if (retr != CMPI_RC_OK) {
if (capsinfo != NULL && domtype != NULL) { /* set default */
@@ -1415,7 +1418,7 @@ static int parse_ip_address(const char *id,
if (strstr(id, "[") != NULL) {
/* its an ipv6 address */
ret = sscanf(id, "%a[^]]]:%as", &tmp_ip, &tmp_port);
- if (tmp_ip != NULL) {
+ if (ret >= 1) {
tmp_ip = realloc(tmp_ip, strlen(tmp_ip) + 2);
if (tmp_ip == NULL) {
ret = 0;
@@ -2798,7 +2801,8 @@ static CMPIStatus update_system_settings(const CMPIContext
*context,
}
if ((dominfo->uuid == NULL) || (STREQ(dominfo->uuid, ""))) {
- dominfo->uuid = strdup(uuid);
+ dominfo->uuid = uuid;
+ uuid = NULL;
} else if (!STREQ(uuid, dominfo->uuid)) {
cu_statusf(_BROKER, &s,
CMPI_RC_ERR_FAILED,
@@ -2829,6 +2833,7 @@ static CMPIStatus update_system_settings(const CMPIContext
*context,
}
out:
+ free(uuid);
free(xml);
virDomainFree(dom);
virConnectClose(conn);
--
1.8.4.2
1:18 p.m.
New subject: [libvirt] [PATCH 1/6] VSMS: Coverity cleanups
On 01/22/2014 12:05 PM, John Ferlan wrote:
A new version of Coverity found a number of issues:
Which repo is this against? It isn't libvirt.git; so you may want to
'git config format.subjectprefix ...' to automatically give a hint in
your subject line which repo it is for.
The patch itself looks okay, but I'm not sure where I'm giving my ACK :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:21 p.m.
New subject: [libvirt] [PATCH 1/6] VSMS: Coverity cleanups
On 01/22/2014 12:05 PM, John Ferlan wrote:
A new version of Coverity found a number of issues:
@@ -1415,7 +1418,7 @@ static int parse_ip_address(const char *id,
if (strstr(id, "[") != NULL) {
/* its an ipv6 address */
ret = sscanf(id, "%a[^]]]:%as", &tmp_ip, &tmp_port);
Unrelated to your cleanups, but also a problem: This use of sscanf is
non-portable, and very likely to crash on non-glibc. %a in C99 means to
scan a floating point number (similar to %f), which is _very different_
from glibc's older meaning of malloc'ing into the destination. %m is
the preferred spelling for the intent of the code here.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:27 p.m.
New subject: [libvirt] [PATCH 1/6] VSMS: Coverity cleanups
On 01/22/2014 02:21 PM, Eric Blake wrote:
On 01/22/2014 12:05 PM, John Ferlan wrote:
> A new version of Coverity found a number of issues:
>
> @@ -1415,7 +1418,7 @@ static int parse_ip_address(const char *id,
> if (strstr(id, "[") != NULL) {
> /* its an ipv6 address */
> ret = sscanf(id, "%a[^]]]:%as", &tmp_ip,
&tmp_port);
Unrelated to your cleanups, but also a problem: This use of sscanf is
non-portable, and very likely to crash on non-glibc. %a in C99 means to
scan a floating point number (similar to %f), which is _very different_
from glibc's older meaning of malloc'ing into the destination. %m is
the preferred spelling for the intent of the code here.
If you have seen my 0/6 update - I sent to the wrong list.
As for the portability - suffice to say the libvirt-cim code is nowhere
near as clean as libvirt (and friends). It's code that doesn't see a lot
of change...
Thanks for the heads up though - I will follow up there with a change to
use %m rather than %a. I read the man page and did wonder how/why %a
was used, but didn't dig into when the code was added.
John
1:05 p.m.
New subject: [libvirt] [PATCH 2/6] libxkutil:pool_parsing: Coverity cleanups
A new version of Coverity found a number of issues:
get_pool_from_xml(): FORWARD_NULL
parse_disk_pool(): RESOURCE_LEAK
- If parse_disk_pool() returned name == NULL, then the strdup() of
name into pool->id would dereference the NULL pointer leading to
a segfault.
Furthermore parse_disk_pool() had a few issues. First the 'type_str'
could be NULL, so that needs to be checked. Second, 'type_str' was
never free()'d (the get_attr_value returns a strdup()'d value).
Realizing all that resulted in a few extra changes to not strdup()
a value that we strdup()'d
Eventually get_pool_from_xml() will return to get_disk_pool() which
returns to diskpool_set_capacity() or _new_volume_template() which
both error out when return value != 0 (although, I did change the
latter to be more explicit and match the former).
Finally, even though the parsing of the element will only ever have
one "name" value - Coverity doesn't know that, so as a benign fix be
sure to not overwrite 'name' if "name" isn't already set.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
libxkutil/pool_parsing.c | 39 ++++++++++++++++++-----------------
src/Virt_SettingsDefineCapabilities.c | 2 +-
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/libxkutil/pool_parsing.c b/libxkutil/pool_parsing.c
index 922ff32..80bd4ca 100644
--- a/libxkutil/pool_parsing.c
+++ b/libxkutil/pool_parsing.c
@@ -194,15 +194,17 @@ char *get_disk_pool_type(uint16_t type)
}
-static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool)
+static char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool)
{
xmlNode **nodes = nsv->nodeTab;
xmlNode *child;
- const char *type_str = NULL;
- const char *name = NULL;
+ char *type_str = NULL;
+ char *name = NULL;
int type = 0;
type_str = get_attr_value(nodes[0], "type");
+ if (type_str == NULL)
+ return NULL;
if (STREQC(type_str, "dir"))
type = DISK_POOL_DIR;
@@ -220,12 +222,15 @@ static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool
*pool)
type = DISK_POOL_SCSI;
else
type = DISK_POOL_UNKNOWN;
+ free(type_str);
pool->pool_type = type;
-
+
for (child = nodes[0]->children; child != NULL; child = child->next) {
- if (XSTREQ(child->name, "name")) {
+ if (XSTREQ(child->name, "name") && name == NULL) {
name = get_node_content(child);
+ if (name == NULL)
+ return NULL;
} else if (XSTREQ(child->name, "target"))
parse_disk_target(child, pool);
else if (XSTREQ(child->name, "source"))
@@ -238,14 +243,18 @@ static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool
*pool)
int get_pool_from_xml(const char *xml, struct virt_pool *pool, int type)
{
int len;
- int ret = 0;
+ int ret = 1;
xmlDoc *xmldoc;
xmlXPathContext *xpathctx;
xmlXPathObject *xpathobj;
const xmlChar *xpathstr = (xmlChar *)"/pool";
- const char *name;
CU_DEBUG("Pool XML : %s", xml);
+
+ /* FIXME: Add support for parsing network pools */
+ if (type == CIM_RES_TYPE_NET)
+ return 0;
+
len = strlen(xml) + 1;
if ((xmldoc = xmlParseMemory(xml, len)) == NULL)
@@ -257,22 +266,14 @@ int get_pool_from_xml(const char *xml, struct virt_pool *pool, int
type)
if ((xpathobj = xmlXPathEvalExpression(xpathstr, xpathctx)) == NULL)
goto err3;
- /* FIXME: Add support for parsing network pools */
- if (type == CIM_RES_TYPE_NET) {
- ret = 0;
- goto err1;
- }
-
memset(pool, 0, sizeof(*pool));
- pool->type = CIM_RES_TYPE_DISK;
- name = parse_disk_pool(xpathobj->nodesetval,
- &(pool)->pool_info.disk);
- if (name == NULL)
+ pool->type = CIM_RES_TYPE_DISK;
+ pool->id = parse_disk_pool(xpathobj->nodesetval,
+ &(pool)->pool_info.disk);
+ if (pool->id != NULL)
ret = 0;
- pool->id = strdup(name);
-
xmlXPathFreeObject(xpathobj);
err3:
xmlXPathFreeContext(xpathctx);
diff --git a/src/Virt_SettingsDefineCapabilities.c
b/src/Virt_SettingsDefineCapabilities.c
index fe16e3f..756e46b 100644
--- a/src/Virt_SettingsDefineCapabilities.c
+++ b/src/Virt_SettingsDefineCapabilities.c
@@ -1376,7 +1376,7 @@ static CMPIStatus _new_volume_template(const CMPIObjectPath *ref,
}
ret = get_disk_pool(poolptr, &pool);
- if (ret == 1) {
+ if (ret != 0) {
virt_set_status(_BROKER, &s,
CMPI_RC_ERR_FAILED,
virStoragePoolGetConnect(poolptr),
--
1.8.4.2
1:05 p.m.
New subject: [libvirt] [PATCH 3/6] libxkutil:device_parsing: Coverity cleanups
A new version of Coverity found a number of issues:
parse_os(): RESOURCE_LEAK
- A benign issue due to using a for() loop in order to process the
XML fields. Although it's not possible to have a second entry with
the same text, Coverity doesn't know that so flagged each of the
get_attr_value() calls with a possible overwrite of allocated memory.
In order to "fix' that - just check for each being NULL prior to
the setting - a benign fix that shuts Coverity up
- A real issue was found - the 'loader' variable wasn't free()'d
parse_console_device(): RESOURCE_LEAK
- A benign issue due to using a for() loop in order to process the
XML fields results in 'target_port_ID' and 'udp_source_mode' being
flagged for possible overwrites. For the 'udp_source_mode' changed
the code to declare, use, free only within the scope of the case.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
libxkutil/device_parsing.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c
index 56e39c7..4dd9e58 100644
--- a/libxkutil/device_parsing.c
+++ b/libxkutil/device_parsing.c
@@ -853,7 +853,6 @@ static int parse_console_device(xmlNode *node, struct virt_device
**vdevs)
struct console_device *cdev = NULL;
char *source_type_str = NULL;
char *target_port_ID = NULL;
- char *udp_source_mode = NULL;
xmlNode *child = NULL;
@@ -875,7 +874,7 @@ static int parse_console_device(xmlNode *node, struct virt_device
**vdevs)
CU_DEBUG("console device type ID = %d", cdev->source_type);
for (child = node->children; child != NULL; child = child->next) {
- if (XSTREQ(child->name, "target")) {
+ if (XSTREQ(child->name, "target") && target_port_ID
== NULL) {
cdev->target_type = get_attr_value(child, "type");
CU_DEBUG("Console device target type = '%s'",
cdev->target_type ? : "NULL");
@@ -910,7 +909,9 @@ static int parse_console_device(xmlNode *node, struct virt_device
**vdevs)
get_attr_value(child, "path");
break;
case CIM_CHARDEV_SOURCE_TYPE_UDP:
- udp_source_mode = get_attr_value(child,
"mode");
+ {
+ char *udp_source_mode = get_attr_value(child,
+
"mode");
if (udp_source_mode == NULL)
goto err;
if (STREQC(udp_source_mode, "bind")) {
@@ -925,10 +926,13 @@ static int parse_console_device(xmlNode *node, struct virt_device
**vdevs)
get_attr_value(child,
"service");
} else {
CU_DEBUG("unknown udp mode: %s",
- udp_source_mode ? : "NULL");
+ udp_source_mode);
+ free(udp_source_mode);
goto err;
}
+ free(udp_source_mode);
break;
+ }
case CIM_CHARDEV_SOURCE_TYPE_TCP:
cdev->source_dev.tcp.mode =
get_attr_value(child, "mode");
@@ -965,14 +969,12 @@ static int parse_console_device(xmlNode *node, struct virt_device
**vdevs)
*vdevs = vdev;
free(source_type_str);
free(target_port_ID);
- free(udp_source_mode);
return 1;
err:
free(source_type_str);
free(target_port_ID);
- free(udp_source_mode);
cleanup_console_device(cdev);
free(vdev);
@@ -1500,33 +1502,35 @@ static int parse_os(struct domain *dominfo, xmlNode *os)
for (child = os->children; child != NULL; child = child->next) {
if (XSTREQ(child->name, "type")) {
STRPROP(dominfo, os_info.pv.type, child);
- arch = get_attr_value(child, "arch");
- machine = get_attr_value(child, "machine");
- } else if (XSTREQ(child->name, "kernel"))
+ if (arch == NULL)
+ arch = get_attr_value(child, "arch");
+ if (machine == NULL)
+ machine = get_attr_value(child, "machine");
+ } else if (XSTREQ(child->name, "kernel") && kernel
== NULL)
kernel = get_node_content(child);
- else if (XSTREQ(child->name, "initrd"))
+ else if (XSTREQ(child->name, "initrd") && initrd ==
NULL)
initrd = get_node_content(child);
- else if (XSTREQ(child->name, "cmdline"))
+ else if (XSTREQ(child->name, "cmdline") && cmdline
== NULL)
cmdline = get_node_content(child);
- else if (XSTREQ(child->name, "loader"))
+ else if (XSTREQ(child->name, "loader") && loader ==
NULL)
loader = get_node_content(child);
- else if (XSTREQ(child->name, "boot")) {
+ else if (XSTREQ(child->name, "boot") && boot ==
NULL) {
char **tmp_list = NULL;
- tmp_list = (char **)realloc(blist,
- (bl_size + 1) *
+ tmp_list = (char **)realloc(blist,
+ (bl_size + 1) *
sizeof(char *));
if (tmp_list == NULL) {
// Nothing you can do. Just go on.
CU_DEBUG("Could not alloc space for "
"boot device");
- continue;
+ continue;
}
blist = tmp_list;
-
+
blist[bl_size] = get_attr_value(child, "dev");
bl_size++;
- } else if (XSTREQ(child->name, "init"))
+ } else if (XSTREQ(child->name, "init") && init ==
NULL)
init = get_node_content(child);
}
@@ -1580,6 +1584,7 @@ static int parse_os(struct domain *dominfo, xmlNode *os)
free(kernel);
free(initrd);
free(cmdline);
+ free(loader);
free(boot);
free(init);
cleanup_bootlist(blist, bl_size);
--
1.8.4.2
1:05 p.m.
New subject: [libvirt] [PATCH 4/6] libxkutil/xml_parse_test: Coverity cleanup
A new version of Coverity found:
print_domxml(): RESOURCE_LEAK
- The 'xml' variable needs to be free()'d
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
libxkutil/xml_parse_test.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/libxkutil/xml_parse_test.c b/libxkutil/xml_parse_test.c
index 374bcf6..303880d 100644
--- a/libxkutil/xml_parse_test.c
+++ b/libxkutil/xml_parse_test.c
@@ -192,6 +192,7 @@ static void print_domxml(struct domain *dominfo,
printf("Failed to create system XML\n");
else
printf("%s\n", xml);
+ free(xml);
}
static char *read_from_file(FILE *file)
--
1.8.4.2
1:05 p.m.
New subject: [libvirt] [PATCH 5/6] RAFP: Coverity cleanup
A new version of Coverity found:
filter_by_pool(): RESOURCE_LEAK
- Because the code is run within a for() loop Coverity complains
that the returned 'poolid' is not free'd during each pass through
the loop. So even though it may not be fetched again, just free()
and reinitialize 'poolid' after usage
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/Virt_ResourceAllocationFromPool.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/Virt_ResourceAllocationFromPool.c
b/src/Virt_ResourceAllocationFromPool.c
index 7088900..7bee729 100644
--- a/src/Virt_ResourceAllocationFromPool.c
+++ b/src/Virt_ResourceAllocationFromPool.c
@@ -120,9 +120,12 @@ static int filter_by_pool(struct inst_list *dest,
poolid = pool_member_of(_BROKER, CLASSNAME(op), type, rasd_id);
if ((poolid != NULL) && STREQ(poolid, _poolid))
inst_list_add(dest, inst);
- }
- free(poolid);
+ if (poolid != NULL) {
+ free(poolid);
+ poolid = NULL;
+ }
+ }
return dest->cur;
}
--
1.8.4.2
1:05 p.m.
New subject: [libvirt] [PATCH 6/6] EAFP: Coverity cleanup
A new version of Coverity found:
get_dev_from_pool(): RESOURCE_LEAK
- Because the code is run within a for() loop Coverity complains
that the returned 'poolid' is not free'd during each pass through
the loop. So even though it may not be fetched again, just free()
and reinitialize 'poolid' after usage
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/Virt_ElementAllocatedFromPool.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/Virt_ElementAllocatedFromPool.c b/src/Virt_ElementAllocatedFromPool.c
index 03d856a..2c2f2d1 100644
--- a/src/Virt_ElementAllocatedFromPool.c
+++ b/src/Virt_ElementAllocatedFromPool.c
@@ -124,10 +124,14 @@ static CMPIStatus get_dev_from_pool(const CMPIObjectPath *ref,
poolid = pool_member_of(_BROKER, cn, type, dev_id);
if (poolid && STREQ(poolid, _poolid))
inst_list_add(list, inst);
+
+ if (poolid) {
+ free(poolid);
+ poolid = NULL;
+ }
}
out:
- free(poolid);
inst_list_free(&tmp);
return s;
--
1.8.4.2
1:18 p.m.
On 01/22/2014 02:05 PM, John Ferlan wrote:
A recent Coverity version update discovered some existing issues (and
some
benign cases). This patch set cleans them all up.
John Ferlan (6):
VSMS: Coverity cleanups
libxkutil:pool_parsing: Coverity cleanups
libxkutil:device_parsing: Coverity cleanups
libxkutil/xml_parse_test: Coverity cleanup
RAFP: Coverity cleanup
EAFP: Coverity cleanup
libxkutil/device_parsing.c | 41 +++++++++++++++++--------------
libxkutil/pool_parsing.c | 39 +++++++++++++++--------------
libxkutil/xml_parse_test.c | 1 +
src/Virt_ElementAllocatedFromPool.c | 6 ++++-
src/Virt_ResourceAllocationFromPool.c | 7 ++++--
src/Virt_SettingsDefineCapabilities.c | 2 +-
src/Virt_VirtualSystemManagementService.c | 11 ++++++---
7 files changed, 63 insertions(+), 44 deletions(-)
UGH - ignore the series - this is libvirt-cim and my fingers
cut-n-paste'd from the wrong place... Bad fingers - gonna have to punish
them
John
3929
days inactive
3929
days old
10 comments
2 participants
participants (2)
-
Eric Blake -
John Ferlan