[libvirt] [PATCH] Fix memory leak in virDomainSnapshotDiskDefClear()

[Ján Tomko]

[cc-ing Peter as he invented the function]

On 03/03/2014 09:04 PM, Nehal J Wani wrote:
>>> ---
>>>  src/conf/snapshot_conf.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>>> index 12b0930..475525f 100644
>>> --- a/src/conf/snapshot_conf.c
>>> +++ b/src/conf/snapshot_conf.c
>>> @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
>>>  {
>>>      VIR_FREE(disk->name);
>>>      VIR_FREE(disk->file);
>>> +    virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
>>
>> This leaves nhosts and hosts at their original values, which seems to be OK
>> everywhere this function is called, but it might be a problem if someone tries
>> to reuse the disk definition instead of freeing it.
>>
>> I'd rather write this as:
>>
>> while (disk->nhosts)
>>     virDomainDiskHostDefClear(&disk->hosts[--def->nhosts])
>> VIR_FREE(disk->hosts)
>>
>> Jan
>>
> 
> 1. Since virDomainDiskHostDefFree() already calls VIR_FREE(hosts),
> shouldn't the modified patch just be:
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 12b0930..a233e8e 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -82,6 +82,8 @@
> virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
>  {
>      VIR_FREE(disk->name);
>      VIR_FREE(disk->file);
> +    virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
> +    disk->nhosts = 0;
>  }

This leaves the pointer to the freed memory in disk->hosts.

> 
>  void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
> 
> 2. I have a question. Why don't we have disk->nhosts = 0; each time
> someone calls virDomainDiskHostDefFree(disk->nhosts, disk->hosts) in
> ./src/conf/domain_conf.c, ./src/storage/storage_driver.c and
> ./src/qemu/qemu_conf.c?

In most cases either the whole def gets freed, or hosts and nhosts are
overwritten. I'm not sure if we overwrite it in all cases in
qemuTranslateDiskSourcePool or it needs to be added there too.

> Or we can change the definition of virDomainDiskHostDefFree to
> virDomainDiskHostDefFree(size_t *nhosts, virDomainDiskHostDefPtr hosts)
> and make *nhosts = 0 inside the function itself and make the necessary
> changes wherever this function is called?
> 

That looks more awkward than the original function to me.

I think it should be called virDomainDiskHostDefsFree instead, since it's
different from all the other DefFree functions, which only take one DefPtr
argument. The callers should make sure that the leftover values in
nhosts/hosts aren't reused.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140304/7da8b4c9/attachment-0001.sig>