[Libvirt-cim] [PATCH] device_parsing: Fixing potential leaks

Thu Aug 18 14:05:06 UTC 2011

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 at 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 at 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 at br.ibm.com