+1 Sharad Mishra Open Virtualization Linux Technology Center IBM libvirt-cim-bounces@redhat.com wrote on 11/03/2011 10:48:29 AM:

"Eduardo Lima (Etrunko)" <eblima@linux.vnet.ibm.com> Sent by: libvirt-cim-bounces@redhat.com

11/03/2011 10:48 AM

Please respond to List for discussion and development of libvirt CIM <libvirt-cim@redhat.com>

To

libvirt-cim@redhat.com

cc

"Eduardo Lima \(Etrunko\)" <eblima@br.ibm.com>

Subject

[Libvirt-cim] [PATCH 1/8] acl_parsing: Avoid NULL dereference

From: Eduardo Lima (Etrunko) <eblima@br.ibm.com>

As revealed by Coverity scan report:

https://bugzilla.redhat.com/show_bug.cgi?id=728245#c8 https://bugzilla.redhat.com/attachment.cgi?id=530435

Error: REVERSE_INULL: acl_parsing.c:172: deref_ptr: Directly dereferencing pointer "filters". acl_parsing.c:174: check_after_deref: Dereferencing "filters" before a null check.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- libxkutil/acl_parsing.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/libxkutil/acl_parsing.c b/libxkutil/acl_parsing.c index 935e642..7cde1f0 100644 --- a/libxkutil/acl_parsing.c +++ b/libxkutil/acl_parsing.c @@ -151,11 +151,13 @@ void cleanup_filter(struct acl_filter *filter) void cleanup_filters(struct acl_filter **filters, int count) { int i; - struct acl_filter *_filters = *filters; + struct acl_filter *_filters;

if((filters == NULL) || (*filters == NULL) || (count == 0)) return;

+ _filters = *filters; + for (i = 0; i < count; i++) cleanup_filter(&_filters[i]);

-- 1.7.4.4

_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim

+1 Sharad Mishra Open Virtualization Linux Technology Center IBM libvirt-cim-bounces@redhat.com wrote on 11/03/2011 10:48:30 AM:

"Eduardo Lima (Etrunko)" <eblima@linux.vnet.ibm.com> Sent by: libvirt-cim-bounces@redhat.com

11/03/2011 10:48 AM

Please respond to List for discussion and development of libvirt CIM <libvirt-cim@redhat.com>

To

libvirt-cim@redhat.com

cc

"Eduardo Lima \(Etrunko\)" <eblima@br.ibm.com>

Subject

[Libvirt-cim] [PATCH 2/8] pool_parsing: Avoid NULL dereferences

From: Eduardo Lima (Etrunko) <eblima@br.ibm.com>

As revealed by Coverity scan report:

https://bugzilla.redhat.com/show_bug.cgi?id=728245#c8 https://bugzilla.redhat.com/attachment.cgi?id=530435

Error: REVERSE_INULL: pool_parsing.c:74: deref_ptr: Directly dereferencing pointer "pool". pool_parsing.c:76: check_after_deref: Dereferencing "pool" before a null check.

Error: REVERSE_INULL: pool_parsing.c:99: deref_ptr: Directly dereferencing pointer "res". pool_parsing.c:101: check_after_deref: Dereferencing "res" before a null check.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- libxkutil/pool_parsing.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/libxkutil/pool_parsing.c b/libxkutil/pool_parsing.c index f7a5a2c..f73b0fd 100644 --- a/libxkutil/pool_parsing.c +++ b/libxkutil/pool_parsing.c @@ -71,11 +71,12 @@ static void cleanup_disk_pool(struct disk_pool pool) {

void cleanup_virt_pool(struct virt_pool **pool) { - struct virt_pool *_pool = *pool; + struct virt_pool *_pool;

if ((pool == NULL) || (*pool == NULL)) return; - + + _pool = *pool; if (_pool->type == CIM_RES_TYPE_NET) cleanup_net_pool(_pool->pool_info.net); else if (_pool->type == CIM_RES_TYPE_DISK) @@ -96,11 +97,13 @@ static void cleanup_image_res(struct storage_vol vol)

void cleanup_virt_pool_res(struct virt_pool_res **res) { - struct virt_pool_res *_res = *res; + struct virt_pool_res *_res;

if ((res == NULL) || (*res == NULL)) return;

+ _res = *res; + if (_res->type == CIM_RES_TYPE_IMAGE) cleanup_image_res(_res->res.storage_vol);

-- 1.7.4.4

_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim

"Eduardo Lima (Etrunko)" <eblima@linux.vnet.ibm.com> Sent by: libvirt-cim-bounces@redhat.com

11/03/2011 10:48 AM

Please respond to List for discussion and development of libvirt CIM <libvirt-cim@redhat.com>

To

libvirt-cim@redhat.com

cc

"Eduardo Lima \(Etrunko\)" <eblima@br.ibm.com>

Subject

[Libvirt-cim] [PATCH 3/8] device_parsing: Avoid NULL dereferences

From: Eduardo Lima (Etrunko) <eblima@br.ibm.com>

As revealed by Coverity scan report:

https://bugzilla.redhat.com/show_bug.cgi?id=728245#c8 https://bugzilla.redhat.com/attachment.cgi?id=530435

Error: REVERSE_INULL: device_parsing.c:1153: deref_ptr: Directly dereferencing pointer "dominfo". device_parsing.c:1155: check_after_deref: Dereferencing "dominfo" before a null check.

Error: FORWARD_NULL: device_parsing.c:226: assign_zero: Assigning: "ddev" = 0. device_parsing.c:284: var_deref_model: Passing null variable "ddev" to function "cleanup_disk_device", which dereferences it. device_parsing.c:54: deref_parm: Directly dereferencing parameter "dev".

Error: FORWARD_NULL: device_parsing.c:170: assign_zero: Assigning: "ddev" = 0. device_parsing.c:217: var_deref_model: Passing null variable "ddev" to function "cleanup_disk_device", which dereferences it. device_parsing.c:54: deref_parm: Directly dereferencing parameter "dev".

Error: FORWARD_NULL: device_parsing.c:457: assign_zero: Assigning: "edev" = 0. device_parsing.c:475: var_deref_model: Passing null variable "edev" to function "cleanup_emu_device", which dereferences it. device_parsing.c:88: deref_parm: Directly dereferencing parameter "dev".

Error: FORWARD_NULL: device_parsing.c:516: assign_zero: Assigning: "gdev" = 0. device_parsing.c:579: var_deref_model: Passing null variable "gdev" to function "cleanup_graphics_device", which dereferences it. device_parsing.c:93: deref_parm: Directly dereferencing parameter "dev".

Error: FORWARD_NULL: device_parsing.c:588: assign_zero: Assigning: "idev" = 0. device_parsing.c:615: var_deref_model: Passing null variable "idev" to function "cleanup_input_device", which dereferences it. device_parsing.c:102: deref_parm: Directly dereferencing parameter "dev".

Error: FORWARD_NULL: device_parsing.c:310: var_compare_op: Comparing "vsi_dev" to null implies

+1 Sharad Mishra Open Virtualization Linux Technology Center IBM libvirt-cim-bounces@redhat.com wrote on 11/03/2011 10:48:31 AM: that

"vsi_dev" might be null. device_parsing.c:344: var_deref_model: Passing null variable "vsi_dev" to function "cleanup_vsi_device", which dereferences

it.

device_parsing.c:66: deref_parm: Directly dereferencing parameter "dev".

Error: FORWARD_NULL: device_parsing.c:352: assign_zero: Assigning: "ndev" = 0. device_parsing.c:416: var_deref_model: Passing null variable "ndev" to function "cleanup_net_device", which dereferences it. device_parsing.c:77: deref_parm: Directly dereferencing parameter "dev".

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- libxkutil/device_parsing.c | 21 ++++++++++++++++++++- 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index d25461e..371838f 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -54,6 +54,9 @@ typedef int (*dev_parse_func_t)(xmlNode *, struct virt_device **);

static void cleanup_disk_device(struct disk_device *dev) { + if (dev == NULL) + return; + free(dev->type); free(dev->device); free(dev->driver); @@ -66,6 +69,9 @@ static void cleanup_disk_device(struct disk_device *dev)

static void cleanup_vsi_device(struct vsi_device *dev) { + if (dev == NULL) + return; + free(dev->vsi_type); free(dev->manager_id); free(dev->type_id); @@ -77,6 +83,9 @@ static void cleanup_vsi_device(struct vsi_device *dev)

static void cleanup_net_device(struct net_device *dev) { + if (dev == NULL) + return; + free(dev->type); free(dev->mac); free(dev->source); @@ -88,6 +97,9 @@ static void cleanup_net_device(struct net_device *dev)

static void cleanup_emu_device(struct emu_device *dev) { + if (dev == NULL) + return; + free(dev->path); }

@@ -108,6 +120,9 @@ static void cleanup_sdl_device(struct graphics_device *dev)

static void cleanup_graphics_device(struct graphics_device *dev) { + if (dev == NULL) + return; + if (STREQC(dev->type, "sdl")) { cleanup_sdl_device(dev); } @@ -119,6 +134,9 @@ static void cleanup_graphics_device(struct graphics_device *dev)

static void cleanup_input_device(struct input_device *dev) { + if (dev == NULL) + return; + free(dev->type); free(dev->bus); } @@ -1207,11 +1225,12 @@ int get_dominfo(virDomainPtr dom, struct domain **dominfo)

void cleanup_dominfo(struct domain **dominfo) { - struct domain *dom = *dominfo; + struct domain *dom;

if ((dominfo == NULL) || (*dominfo == NULL)) return;

+ dom = *dominfo; free(dom->name); free(dom->uuid); free(dom->bootloader); -- 1.7.4.4

_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim

+1 On 11/03/2011 01:48 PM, Eduardo Lima (Etrunko) wrote:

From: Eduardo Lima (Etrunko)<eblima@br.ibm.com>

As revealed by Coverity scan report

https://bugzilla.redhat.com/show_bug.cgi?id=728245#c8 https://bugzilla.redhat.com/attachment.cgi?id=530435

This is the same case that was fixed in commit 57765ed. The function cleanup_filter() does not free the variable itself. In future uses, consider using cleanup_filters() instead.

Signed-off-by: Eduardo Lima (Etrunko)<eblima@br.ibm.com> --- libxkutil/acl_parsing.c | 5 ++++- src/Virt_AppliedFilterList.c | 8 ++++---- src/Virt_EntriesInFilterList.c | 2 +- src/Virt_FilterEntry.c | 2 +- src/Virt_FilterList.c | 6 +++--- src/Virt_NestedFilterList.c | 17 +++++++---------- 6 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/libxkutil/acl_parsing.c b/libxkutil/acl_parsing.c index 7cde1f0..5b6d7bb 100644 --- a/libxkutil/acl_parsing.c +++ b/libxkutil/acl_parsing.c @@ -357,8 +357,10 @@ static int parse_acl_filter(xmlNode *fnode, struct acl_filter *filter) if (parse_acl_rule(child, rule) == 0) goto err;

- if (append_filter_rule(filter, rule) == 0) + if (append_filter_rule(filter, rule) == 0) { + cleanup_rule(rule); goto err; + } } else if (XSTREQ(child->name, "filterref")) { filter_ref = get_attr_value(child, "filter"); @@ -504,6 +506,7 @@ int get_filters( break;

memcpy(&filters[i], filter, sizeof(*filter)); + free(filter); }

*list = filters; diff --git a/src/Virt_AppliedFilterList.c b/src/Virt_AppliedFilterList.c index 6dad3cb..6567118 100644 --- a/src/Virt_AppliedFilterList.c +++ b/src/Virt_AppliedFilterList.c @@ -193,7 +193,7 @@ static CMPIStatus list_to_net( if (filter == NULL) goto out;

- cleanup_filter(filter); + cleanup_filters(&filter, 1);

/* get domains */ dcount = get_domain_list(conn,&doms); @@ -331,7 +331,7 @@ static CMPIStatus net_to_list( filter, &instance);

- cleanup_filter(filter); + cleanup_filters(&filter, 1);

if (instance != NULL) inst_list_add(list, instance); @@ -515,7 +515,7 @@ static CMPIStatus CreateInstance( free(domain_name); free(net_name);

- cleanup_filter(filter); + cleanup_filters(&filter, 1); cleanup_virt_devices(&device, 1);

virDomainFree(dom); @@ -625,7 +625,7 @@ static CMPIStatus DeleteInstance( free(domain_name); free(net_name);

- cleanup_filter(filter); + cleanup_filters(&filter, 1); cleanup_virt_devices(&device, 1);

virDomainFree(dom); diff --git a/src/Virt_EntriesInFilterList.c b/src/Virt_EntriesInFilterList.c index 5b95dd3..2c8ac47 100644 --- a/src/Virt_EntriesInFilterList.c +++ b/src/Virt_EntriesInFilterList.c @@ -83,7 +83,7 @@ static CMPIStatus list_to_rule( } }

- cleanup_filter(filter); + cleanup_filters(&filter, 1);

out: virConnectClose(conn); diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index ad87689..acc3d61 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -687,7 +687,7 @@ CMPIStatus get_rule_by_ref( &s); out: free(filter_name); - cleanup_filter(filter); + cleanup_filters(&filter, 1); virConnectClose(conn);

return s; diff --git a/src/Virt_FilterList.c b/src/Virt_FilterList.c index 5df2a92..35d18a9 100644 --- a/src/Virt_FilterList.c +++ b/src/Virt_FilterList.c @@ -177,7 +177,7 @@ CMPIStatus get_filter_by_ref(const CMPIBroker *broker, s = instance_from_filter(broker, context, reference, filter, instance);

out: - cleanup_filter(filter); + cleanup_filters(&filter, 1); virConnectClose(conn);

return s; @@ -320,7 +320,7 @@ static CMPIStatus CreateInstance( CU_DEBUG("CreateInstance complete");

out: - cleanup_filter(filter); + cleanup_filters(&filter, 1); virConnectClose(conn);

return s; @@ -361,7 +361,7 @@ static CMPIStatus DeleteInstance( delete_filter(conn, filter);

out: - cleanup_filter(filter); + cleanup_filters(&filter, 1); virConnectClose(conn);

return s; diff --git a/src/Virt_NestedFilterList.c b/src/Virt_NestedFilterList.c index a129f01..894cd7c 100644 --- a/src/Virt_NestedFilterList.c +++ b/src/Virt_NestedFilterList.c @@ -155,13 +155,11 @@ static CMPIStatus parent_to_child( inst_list_add(list, instance); }

- cleanup_filter(child_filter); - - child_filter = NULL; + cleanup_filters(&child_filter, 1); instance = NULL; }

- cleanup_filter(parent_filter); + cleanup_filters(&parent_filter, 1);

out: virConnectClose(conn); @@ -223,10 +221,9 @@ static CMPIStatus child_to_parent(

}

- cleanup_filter(&_list[i]); }

- free(_list); + cleanup_filters(&_list, count);

out: virConnectClose(conn); @@ -390,8 +387,8 @@ static CMPIStatus CreateInstance( CU_DEBUG("CreateInstance completed");

out: - cleanup_filter(parent_filter); - cleanup_filter(child_filter); + cleanup_filters(&parent_filter, 1); + cleanup_filters(&child_filter, 1); virConnectClose(conn);

return s; @@ -481,8 +478,8 @@ static CMPIStatus DeleteInstance( CU_DEBUG("CreateInstance completed");

out: - cleanup_filter(parent_filter); - cleanup_filter(child_filter); + cleanup_filters(&parent_filter, 1); + cleanup_filters(&child_filter, 1); virConnectClose(conn);

return s;

-- Chip Vincent Open Virtualization IBM Linux Technology Center cvincent@linux.vnet.ibm.com

+1 On 11/03/2011 01:48 PM, Eduardo Lima (Etrunko) wrote:

From: Eduardo Lima (Etrunko)<eblima@br.ibm.com>

As revealed by Coverity scan report:

https://bugzilla.redhat.com/show_bug.cgi?id=728245#c8 https://bugzilla.redhat.com/attachment.cgi?id=530435

Error: FORWARD_NULL: Virt_DevicePool.c:999: var_compare_op: Comparing "netnames" to null implies that "netnames" might be null. Virt_DevicePool.c:1019: var_deref_op: Dereferencing null variable "netnames".

Error: FORWARD_NULL: Virt_ComputerSystemIndication.c:259: var_compare_op: Comparing "ind" to null implies that "ind" might be null. Virt_ComputerSystemIndication.c:267: var_deref_op: Dereferencing null variable "ind".

Error: FORWARD_NULL: Virt_ResourcePoolConfigurationService.c:759: var_compare_op: Comparing "op" to null implies that "op" might be null. Virt_ResourcePoolConfigurationService.c:779: var_deref_op: Dereferencing null variable "op".

Error: FORWARD_NULL: Virt_ResourcePoolConfigurationService.c:1028: var_compare_op: Comparing "res" to null implies that "res" might be null. Virt_ResourcePoolConfigurationService.c:1035: var_deref_op: Dereferencing null variable "res".

Error: FORWARD_NULL: Virt_VSMigrationService.c:766: var_compare_op: Comparing "ref" to null implies that "ref" might be null. Virt_VSMigrationService.c:796: var_deref_op: Dereferencing null variable "ref".

Error: FORWARD_NULL: Virt_VirtualSystemManagementService.c:1274: var_compare_op: Comparing "op" to null implies that "op" might be null. Virt_VirtualSystemManagementService.c:1292: var_deref_op: Dereferencing null variable "op".

Error: FORWARD_NULL: Virt_VirtualSystemSnapshotService.c:353: var_compare_op: Comparing "ctx" to null implies that "ctx" might be null. Virt_VirtualSystemSnapshotService.c:376: var_deref_model: Passing null variable "ctx" to function "snap_job_free", which dereferences it. Virt_VirtualSystemSnapshotService.c:66: deref_parm: Directly dereferencing parameter "ctx".

Error: REVERSE_INULL: Virt_ResourcePoolConfigurationService.c:173: deref_ptr: Directly dereferencing pointer "path_list". Virt_ResourcePoolConfigurationService.c:174: check_after_deref: Dereferencing "path_list" before a null check.

Signed-off-by: Eduardo Lima (Etrunko)<eblima@br.ibm.com> --- src/Virt_ComputerSystemIndication.c | 1 + src/Virt_DevicePool.c | 8 +++++--- src/Virt_ResourcePoolConfigurationService.c | 6 +++--- src/Virt_VSMigrationService.c | 9 +++++++-- src/Virt_VirtualSystemManagementService.c | 2 +- src/Virt_VirtualSystemSnapshotService.c | 3 +++ 6 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index c057a0c..337f20a 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -262,6 +262,7 @@ static bool _do_indication(const CMPIBroker *broker, prefix, ind_type_name); ret = false; + goto out; }

ind_op = CMGetObjectPath(ind,&s); diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 7112a9d..a41a378 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -1015,9 +1015,11 @@ static CMPIStatus netpool_instance(virConnectPtr conn, }

out: - for (i = 0; i< nets; i++) - free(netnames[i]); - free(netnames); + if (netnames != NULL) { + for (i = 0; i< nets; i++) + free(netnames[i]); + free(netnames); + }

return s; } diff --git a/src/Virt_ResourcePoolConfigurationService.c b/src/Virt_ResourcePoolConfigurationService.c index 3908c9c..7e76032 100644 --- a/src/Virt_ResourcePoolConfigurationService.c +++ b/src/Virt_ResourcePoolConfigurationService.c @@ -171,7 +171,7 @@ static char *get_dev_paths(CMPIInstance *inst, return "Unable to get DevicePaths array count";

*path_list = calloc(ct, sizeof(char *)); - if (path_list == NULL) + if (*path_list == NULL) return "Failed to alloc space for device paths";

*count = ct; @@ -775,7 +775,7 @@ static const char *rasd_to_res(CMPIInstance *inst, msg = "This function does not support this resource type";

out: - if (msg) + if (msg&& op) CU_DEBUG("rasd_to_res(%s): %s", CLASSNAME(op), msg);

return msg; @@ -1025,7 +1025,7 @@ static CMPIStatus delete_resource_in_pool(CMPIMethodMI *self, goto out;

res = CMGetObjectPath(resource,&s); - if ((res == NULL)&& (s.rc != CMPI_RC_OK)) { + if ((res == NULL) || (s.rc != CMPI_RC_OK)) { cu_statusf(_BROKER,&s, CMPI_RC_ERR_FAILED, "Unable to get ObjectPath of Resource instance"); diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index ee79a96..414feda 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -761,10 +761,15 @@ static bool raise_indication(const CMPIContext *context,

/* This is a workaround for Pegasus, it loses its objectpath by CMGetObjectPath. So set it back. */ - inst->ft->setObjectPath((CMPIInstance *)inst, ref); + if (ref != NULL) + inst->ft->setObjectPath((CMPIInstance *)inst, ref);

if ((ref == NULL) || (s.rc != CMPI_RC_OK)) { CU_DEBUG("Failed to get job reference"); + cu_statusf(_BROKER,&s, + CMPI_RC_ERR_FAILED, + "Failed to get job reference"); + goto out; } else { s = get_host_system_properties(&host, &ccname, @@ -798,7 +803,7 @@ static bool raise_indication(const CMPIContext *context, s = stdi_raise_indication(_BROKER, context, type, ns, ind);

free(type); - + out: return s.rc == CMPI_RC_OK; }

diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 5edc65f..fa1e266 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -1526,7 +1526,7 @@ static const char *rasd_to_vdev(CMPIInstance *inst, else msg = _sysvirt_rasd_to_vdev(inst, dev, type, ns); out: - if (msg) + if (msg&& op) CU_DEBUG("rasd_to_vdev(%s): %s", CLASSNAME(op), msg);

return msg; diff --git a/src/Virt_VirtualSystemSnapshotService.c b/src/Virt_VirtualSystemSnapshotService.c index e7789cb..898fa57 100644 --- a/src/Virt_VirtualSystemSnapshotService.c +++ b/src/Virt_VirtualSystemSnapshotService.c @@ -63,6 +63,9 @@ struct snap_context {

static void snap_job_free(struct snap_context *ctx) { + if (ctx == NULL) + return; + free(ctx->domain); free(ctx->save_path); free(ctx->ref_ns);

-- Chip Vincent Open Virtualization IBM Linux Technology Center cvincent@linux.vnet.ibm.com

"Eduardo Lima (Etrunko)" <eblima@linux.vnet.ibm.com> Sent by: libvirt-cim-bounces@redhat.com

11/03/2011 10:48 AM

Please respond to List for discussion and development of libvirt CIM <libvirt-cim@redhat.com>

To

libvirt-cim@redhat.com

cc

"Eduardo Lima \(Etrunko\)" <eblima@br.ibm.com>

Subject

[Libvirt-cim] [PATCH 6/8] Fix misuse of signed variables

From: Eduardo Lima (Etrunko) <eblima@br.ibm.com>

As revealed by Coverity scan report:

https://bugzilla.redhat.com/show_bug.cgi?id=728245#c8 https://bugzilla.redhat.com/attachment.cgi?id=530435

Error: NEGATIVE_RETURNS: infostore.c:183: negative_return_fn: Function "open(filename, 66, 384)" returns a negative number. infostore.c:183: var_assign: Assigning: signed variable "isc->fd" = "open". infostore.c:214: negative_returns: "isc->fd" is passed to a parameter

+1 that

cannot be negative. infostore.c:53: neg_sink_parm_call: Passing "ctx->fd" to "close", which

cannot

accept a negative.

Error: NEGATIVE_RETURNS: Virt_ComputerSystemIndication.c:518: negative_return_fn: Function "platform_from_class(args->

classname)"

returns a negative number. Virt_ComputerSystemIndication.c:503: return_negative_constant: Explicitly returning negative value "-1". Virt_ComputerSystemIndication.c:518: var_assign: Assigning: signed

variable

"platform" = "platform_from_class". Virt_ComputerSystemIndication.c:539: negative_returns: Using variable "platform" as an index to array

"active_filters".

Error: NEGATIVE_RETURNS: Virt_ComputerSystemIndication.c:518: negative_return_fn: Function "platform_from_class(args->

classname)"

returns a negative number. Virt_ComputerSystemIndication.c:503: return_negative_constant: Explicitly returning negative value "-1". Virt_ComputerSystemIndication.c:518: var_assign: Assigning: signed

variable

"platform" = "platform_from_class". Virt_ComputerSystemIndication.c:596: negative_returns: Using variable "platform" as an index to array "thread_id".

Error: NEGATIVE_RETURNS: Virt_VSMigrationService.c:512: negative_return_fn: Function "mkstemp (filename)" returns a negative number. Virt_VSMigrationService.c:512: var_assign: Assigning: signed variable "fd" = "mkstemp". Virt_VSMigrationService.c:547: negative_returns: "fd" is passed to

aparameter

that cannot be negative.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- libxkutil/infostore.c | 4 +++- src/Virt_ComputerSystemIndication.c | 12 +++++++++--- src/Virt_VSMigrationService.c | 3 ++- 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/libxkutil/infostore.c b/libxkutil/infostore.c index 716dedd..dd1e38c 100644 --- a/libxkutil/infostore.c +++ b/libxkutil/infostore.c @@ -50,7 +50,9 @@ static void infostore_cleanup_ctx(struct infostore_ctx

*ctx)

{ xmlXPathFreeContext(ctx->xpathctx); xmlFreeDoc(ctx->doc); - close(ctx->fd); + + if (ctx->fd >= 0) + close(ctx->fd);

free(ctx); } diff --git a/src/Virt_ComputerSystemIndication.c b/src/ Virt_ComputerSystemIndication.c index 337f20a..9b3b80b 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -518,11 +518,14 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) char *prefix = class_prefix_name(args->classname); int platform = platform_from_class(args->classname);

+ if (prefix == NULL || platform == -1) + goto init_out; + conn = connect_by_classname(_BROKER, args->classname, &s); if (conn == NULL) { CU_DEBUG("Unable to start lifecycle thread: " "Failed to connect (cn: %s)", args->classname); - goto out; + goto conn_out; }

pthread_mutex_lock(&lifecycle_mutex); @@ -591,16 +594,19 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) } }

- out: CU_DEBUG("Exiting CSI event loop (%s)", prefix);

thread_id[platform] = 0;

pthread_mutex_unlock(&lifecycle_mutex); stdi_free_ind_args(&args); - free(prefix); + + conn_out: virConnectClose(conn);

+ init_out: + free(prefix); + return NULL; }

diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 414feda..be9bb7c 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -544,7 +544,8 @@ static char *write_params(CMPIArray *array) if (file != NULL) fclose(file);

- close(fd); + if (fd >= 0) + close(fd);

return filename; } -- 1.7.4.4

_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim

Looks good except for the indentation.

"Eduardo Lima (Etrunko)" <eblima@linux.vnet.ibm.com> Sent by: libvirt-cim-bounces@redhat.com

11/03/2011 10:48 AM

Please respond to List for discussion and development of libvirt CIM <libvirt-cim@redhat.com>

To

libvirt-cim@redhat.com

cc

"Eduardo Lima \(Etrunko\)" <eblima@br.ibm.com>

Subject

[Libvirt-cim] [PATCH 7/8] Fix possible use of uninitialized variables

From: Eduardo Lima (Etrunko) <eblima@br.ibm.com>

As revealed by Coverity scan report:

https://bugzilla.redhat.com/show_bug.cgi?id=728245#c8 https://bugzilla.redhat.com/attachment.cgi?id=530435

Error: UNINIT: Virt_ComputerSystemIndication.c:435: var_decl: Declaring variable "affected_inst" without initializer. Virt_ComputerSystemIndication.c:478: uninit_use: Using uninitialized value "affected_inst".

Error: UNINIT: Virt_ElementCapabilities.c:86: var_decl: Declaring variable "_inst" without initializer. Virt_ElementCapabilities.c:117: uninit_use: Using uninitialized value "_inst".

Error: UNINIT: Virt_ElementCapabilities.c:132: var_decl: Declaring variable "_inst" without initializer. Virt_ElementCapabilities.c:160: uninit_use: Using uninitialized value "_inst".

Error: UNINIT: Virt_VirtualSystemManagementService.c:2516: var_decl: Declaring variable "s" without initializer. Virt_VirtualSystemManagementService.c:2619: uninit_use: Using uninitialized value "s": field "s".msg is uninitialized.

Error: UNINIT: Virt_VirtualSystemSnapshotService.c:490: var_decl: Declaring variable "s" without initializer. Virt_VirtualSystemSnapshotService.c:509: uninit_use: Using uninitialized value "s": field "s".msg is uninitialized.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- src/Virt_ComputerSystemIndication.c | 3 +++ src/Virt_ElementCapabilities.c | 4 ++-- src/Virt_VirtualSystemManagementService.c | 4 ++-- 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/Virt_ComputerSystemIndication.c b/src/ Virt_ComputerSystemIndication.c index 9b3b80b..a00444d 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -471,6 +471,9 @@ static bool async_ind(CMPIContext *context, CU_DEBUG("Could not recreate guest instance"); goto out; } + } else { + CU_DEBUG("Unrecognized indication type"); + goto out; }

/* FIXME: We are unable to get the previous CS instance after it has diff --git a/src/Virt_ElementCapabilities.c b/src/Virt_ElementCapabilities.c index d74bf93..6bd846a 100644 --- a/src/Virt_ElementCapabilities.c +++ b/src/Virt_ElementCapabilities.c @@ -83,7 +83,7 @@ static CMPIStatus validate_caps_get_service_or_rp (const CMPIContext *context, CMPIInstance **inst) { CMPIStatus s = {CMPI_RC_OK, NULL}; - CMPIInstance *_inst; + CMPIInstance *_inst = NULL; char* classname;

classname = class_base_name(CLASSNAME(ref)); @@ -129,7 +129,7 @@ static CMPIStatus validate_service_get_caps (const CMPIContext *context, CMPIInstance **inst) { CMPIStatus s = {CMPI_RC_OK, NULL}; - CMPIInstance *_inst; + CMPIInstance *_inst = NULL;

indentation

char* classname;

classname = class_base_name(CLASSNAME(ref)); diff --git a/src/Virt_VirtualSystemManagementService.c b/src/ Virt_VirtualSystemManagementService.c index fa1e266..21979c3 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -387,7 +387,7 @@ static int bootord_vssd_to_domain(CMPIInstance *inst, CMPICount i; CMPICount bl_size; CMPIArray *bootlist; - CMPIStatus s; + CMPIStatus s = { CMPI_RC_OK, NULL }; CMPIData boot_elem; char **tmp_str_arr;

@@ -2467,7 +2467,7 @@ static CMPIStatus _resource_dynamic(struct domain *dominfo, enum ResourceAction action, const char *refcn) { - CMPIStatus s; + CMPIStatus s = { CMPI_RC_OK, NULL }; virConnectPtr conn; virDomainPtr dom; int (*func)(virDomainPtr, struct virt_device *); -- 1.7.4.4

_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim

On 11/03/2011 08:31 PM, Sharad Mishra wrote: [snip]

...

diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index ee20895..4cca75b 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -1292,6 +1292,7 @@ static const char *storage_vol_xml(xmlNodePtr root, goto out;

free(string); + string = NULL;

shouldn't you be removing the free?

...

if (vol->cap_units != NULL) { xmlAttrPtr tmp = NULL; @@ -1309,6 +1310,7 @@ static const char *storage_vol_xml(xmlNodePtr root, goto out;

free(string); + string = NULL;

same here.

My understanding is that in error cases, the code will end up calling free(string) again in the "out" block. Thus setting the variable string to NULL after each free() call actually protects against double free. -- Eduardo de Barros Lima Software Engineer, Open Virtualization Linux Technology Center - IBM/Brazil eblima@br.ibm.com