On 08/18/2011 02:26 AM, Chip Vincent wrote:
On 08/11/2011 11:09 AM, Eduardo Lima (Etrunko) wrote:
# HG changeset patch # User Eduardo Lima (Etrunko)<eblima@br.ibm.com> # Date 1313075262 10800 # Node ID c0f20dbefb8fd73c278a619bbbd2efb567da07d4 # Parent 2b1b79a72ab0288d649399118dffce26fd05e066 device_parsing: Fixing potential leaks
As reported in https://bugzilla.redhat.com/show_bug.cgi?id=728245
line 106 - Function cleanup_virt_device does not free its parameter dev (this causes a lot of Coverity Resource leak warnings).
In future use cases, please consider using cleanup_virt_devices function instead of cleanup_virt_device if the virt_device structure is allocated dynamically.
Signed-off-by: Eduardo Lima (Etrunko)<eblima@br.ibm.com>
diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -829,12 +829,10 @@ if (ret<= 0) return ret;
- mdev = malloc(sizeof(*mdev)); + mdev = calloc(1, sizeof(*mdev));
What are the benefits to this change?
Well, nothing else than saving the memset below.
if (mdev == NULL) return 0;
- memset(mdev, 0, sizeof(*mdev)); -
This one.
/* We could get one or two memory devices back, depending on * if there is a currentMemory tag or not. Coalesce these * into a single device to return @@ -870,12 +868,10 @@ if (ret<= 0) return ret;
- proc_dev = malloc(sizeof(*proc_dev)); + proc_dev = calloc(1, sizeof(*proc_dev)); if (proc_dev == NULL) return 0;
- memset(proc_dev, 0, sizeof(*proc_dev)); - proc_dev->type = CIM_RES_TYPE_PROC; proc_dev->id = strdup("proc"); proc_dev->dev.vcpu.quantity = proc_devs[0].dev.vcpu.quantity; @@ -1031,8 +1027,6 @@ xmlNode **nodes = nsv->nodeTab; xmlNode *child;
- memset(dominfo, 0, sizeof(*dominfo)); - dominfo->typestr = get_attr_value(nodes[0], "type");
for (child = nodes[0]->children; child != NULL; child = child->next) { @@ -1098,7 +1092,7 @@ int ret;
CU_DEBUG("In get_dominfo_from_xml"); - *dominfo = malloc(sizeof(**dominfo)); + *dominfo = calloc(1, sizeof(**dominfo)); if (*dominfo == NULL) return 0;
diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -832,7 +832,7 @@ dominfo->dev_input = dev; break; default: - cleanup_virt_device(dev); + cleanup_virt_devices(&dev, 1); goto out; }
diff --git a/src/Virt_AppliedFilterList.c b/src/Virt_AppliedFilterList.c --- a/src/Virt_AppliedFilterList.c +++ b/src/Virt_AppliedFilterList.c @@ -516,7 +516,7 @@ free(net_name);
cleanup_filter(filter); - cleanup_virt_device(device); + cleanup_virt_devices(&device, 1);
It seems to me that these two commands should be equal? Wouldn't the correct change be to ensure that they are?
The only difference between those two functions is that the former does not free the struct device while the last does. It is useful to use the first in the case you don't allocate that struct dynamically, as said in the commit message.
virDomainFree(dom); virConnectClose(conn); @@ -626,7 +626,7 @@ free(net_name);
cleanup_filter(filter); - cleanup_virt_device(device); + cleanup_virt_devices(&device, 1);
virDomainFree(dom); virConnectClose(conn); diff --git a/src/Virt_Device.c b/src/Virt_Device.c --- a/src/Virt_Device.c +++ b/src/Virt_Device.c @@ -700,14 +700,15 @@ }
for (i = 0; i< count; i++) { - if (STREQC(device, list[i].id)) + if (STREQC(device, list[i].id)) { dev = virt_device_dup(&list[i]); + break; + }
- cleanup_virt_device(&list[i]); }
+ cleanup_virt_devices(&list, count); out: - free(list);
return dev; } @@ -785,7 +786,7 @@ &tmp_list); }
- cleanup_virt_device(dev); + cleanup_virt_devices(&dev, 1);
*_inst = tmp_list.list[0];
diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -664,7 +664,7 @@ char *host = NULL; char *devid = NULL; virConnectPtr conn = NULL; - struct virt_device *dev; + struct virt_device *dev = NULL;
conn = connect_by_classname(broker, CLASSNAME(reference),&s); if (conn == NULL) { @@ -700,8 +700,8 @@ "Failed to set instance properties"); else *_inst = inst; - - cleanup_virt_device(dev); + + cleanup_virt_devices(&dev, 1);
out: virConnectClose(conn); @@ -863,10 +863,7 @@
tmp_dev->id = strdup("proc");
- for (i = 0; i< count; i++) - cleanup_virt_device(&devs[i]); - - free(devs); + cleanup_virt_devices(&devs, count); devs = tmp_dev; count = 1; } @@ -877,9 +874,6 @@ CMPI_RC_ERR_FAILED, "Failed to get domain name");
- for (i = 0; i< count; i++) - cleanup_virt_device(&devs[i]); - goto out; }
@@ -893,12 +887,10 @@ properties); if (dev) inst_list_add(list, dev); - - cleanup_virt_device(&devs[i]); }
out: - free(devs); + cleanup_virt_devices(&devs, count); return s; }
diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -200,12 +200,6 @@
cleanup_virt_device(domain->dev_emu);
- domain->dev_emu = calloc(1, sizeof(*domain->dev_emu)); - if (domain->dev_emu == NULL) { - CU_DEBUG("Failed to allocate default emulator device"); - return false; - } -
Here is the use case for cleanup_virt_device. You already have memory for dev->emu but you want to update the information stored. No need to allocate new memory, just fill in the fields and you're done. If it is the case, it is possible to declare cleanup_virt_device static and ensure it is not called anywhere else in the code besides device_parsing.c. Best regards, Etrunko -- Eduardo de Barros Lima Software Engineer, Open Virtualization Linux Technology Center - IBM/Brazil eblima@br.ibm.com