[libvirt] [PATCH] Fix several memory leaks

[Ryota Ozaki]

Hi,

On Tue, Sep 1, 2009 at 7:49 PM, Daniel P. Berrange<berrange at redhat.com> wrote:
> On Tue, Sep 01, 2009 at 01:50:50AM +0900, Ryota Ozaki wrote:
>> index df275e6..17ba44a 100644
>> --- a/qemud/qemud.c
>> +++ b/qemud/qemud.c
>> @@ -1733,7 +1733,7 @@ readmore:
>>
>>          /* Possibly need to create another receive buffer */
>>          if ((client->nrequests < max_client_requests &&
>> -             VIR_ALLOC(client->rx) < 0)) {
>> +             !client->rx && VIR_ALLOC(client->rx) < 0)) {
>>              qemudDispatchClientFailure(client);
>>          } else {
>>              if (client->rx)
>
> This is not required. Although client->rx  looks like a list, it will
> only ever have a single entry in it. Since we have just called
> qemudClientMessageQueueServe() a few lines earlier, we know that
> client->rx is NULL at this point. So there is no leak by always
> calling VIR_ALLOC(client->rx);

As I said in the previous mail, this fix seems wrong because I cannot
reproduce the leak. I'll drop it.

(snip)

>> diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
>> index ca6d329..222e591 100644
>> --- a/src/storage_backend_fs.c
>> +++ b/src/storage_backend_fs.c
>> @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn,
>>
>>          vol->type = VIR_STORAGE_VOL_FILE;
>>          vol->target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value
>> is filled in during probe */
>> +        if (vol->target.path)
>> +            VIR_FREE(vol->target.path);
>>          if (virAsprintf(&vol->target.path, "%s/%s",
>>                          pool->def->target.path,
>>                          vol->name) == -1)
>
> This doesn't make any sense - we just allocated 'vol' a few lines
> earlier, so it can't possibly be non-NULL.

Yes, I'm wrong and will drop it.

>
>> @@ -1022,6 +1024,9 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
>>
>>      vol->type = VIR_STORAGE_VOL_FILE;
>>
>> +    if (vol->target.path)
>> +        VIR_FREE(vol->target.path);
>> +
>>      if (virAsprintf(&vol->target.path, "%s/%s",
>>                      pool->def->target.path,
>>                      vol->name) == -1) {
>
> THis one is possibly correct, though we would need the same for
> vol->key too.

Hmm, valgrind didn't pointed out about vol->key. vol->key is set
only on refreshing a volume and is NULL when coming here.
Nonetheless, I think adding VIR_FREE is good idea for preventing
the possible leak in the future. So I'll add it.

> Also there is no need to have 'if (xx)' before a
> call to VIR_FREE - it accepts NULLs happily.  'make syntax-check'
> would complain about this additional 'if'.

Oops, I did it, but did like 'make check; make syntax-check; make -C
tests valgrind'
and then lost the warning ;<


Thanks anyway and will send revised patch soon!
  ozaki-r