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

On 08/11/2011 11:09 AM, Eduardo Lima (Etrunko) wrote: > # HG changeset patch > # User Eduardo Lima (Etrunko)&lt;eblima(a)br.ibm.com&gt; > # 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)&lt;eblima(a)br.ibm.com&gt; > > 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?

> if (mdev == NULL) > return 0; > > - memset(mdev, 0, sizeof(*mdev)); > -

> /* 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?

> > 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; > - } > -