[libvirt] [PATCH] Fix several memory leaks

[Ryota Ozaki]

Hi Chris,

On Tue, Sep 1, 2009 at 4:45 AM, Chris Lalancette<clalance at redhat.com> wrote:
> Ryota Ozaki wrote:
>> diff --git a/qemud/qemud.c b/qemud/qemud.c
>> 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)
>
> Hm, I'm not super familiar with this section of code, but this doesn't seem to
> be right.  How can client->rx ever be NULL here?  We dereference
> client->rx->bufferOffset very early on in the function, so we would have crashed
> before we got here.  What am I missing?

No, you are probably right. As Daniel also says, the fix seems wrong.

I actually used valgrind to find memory leaks and valgrind pointed out here.
However, now the leak cannot be reproduced anymore, so probably I missed
something...

I'll drop this fix in the next patch.

>
>> diff --git a/src/domain_conf.c b/src/domain_conf.c
>> index 1d2cc7c..4b64219 100644
>> --- a/src/domain_conf.c
>> +++ b/src/domain_conf.c
>> @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn,
>>   cleanup:
>>      if (fd != -1)
>>          close(fd);
>> +    VIR_FREE(configFile);
>>      return ret;
>>  }
>
> This one looks good.
>
>>
>> diff --git a/src/network_conf.c b/src/network_conf.c
>> index bb649a4..58a4f32 100644
>> --- a/src/network_conf.c
>> +++ b/src/network_conf.c
>> @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn,
>>  {
>>      char *configFile = NULL;
>>      char *autostartLink = NULL;
>> +    int ret = -1;
>>
>>      if ((configFile = virNetworkConfigFile(conn, configDir,
>> net->def->name)) == NULL)
>>          goto error;
>> @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn,
>>          goto error;
>>      }
>>
>> -    return 0;
>> +    ret = 0;
>>
>>  error:
>>      VIR_FREE(configFile);
>>      VIR_FREE(autostartLink);
>> -    return -1;
>> +    return ret;
>>  }
>
> This one looks good.
>
>>
>>  char *virNetworkConfigFile(virConnectPtr conn,
>> diff --git a/src/qemu_conf.c b/src/qemu_conf.c
>> index 22f5edd..32d6a48 100644
>> --- a/src/qemu_conf.c
>> +++ b/src/qemu_conf.c
>> @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
>>                           virDomainNetDefPtr net,
>>                           int qemuCmdFlags)
>>  {
>> -    char *brname;
>> +    char *brname = NULL;
>>      int err;
>>      int tapfd = -1;
>>      int vnet_hdr = 0;
>> @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
>>          if (brname == NULL)
>>              return -1;
>>      } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>> -        brname = net->data.bridge.brname;
>> +        brname = strdup(net->data.bridge.brname);
>>      } else {
>>          qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
>>                           _("Network type %d is not supported"), net->type);
>> @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
>>      if (!driver->brctl && (err = brInit(&driver->brctl))) {
>>          virReportSystemError(conn, err, "%s",
>>                               _("cannot initialize bridge support"));
>> -        return -1;
>> +        goto cleanup;
>>      }
>>
>>      if (!net->ifname ||
>> @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
>>          VIR_FREE(net->ifname);
>>          if (!(net->ifname = strdup("vnet%d"))) {
>>              virReportOOMError(conn);
>> -            return -1;
>> +            goto cleanup;
>>          }
>>          /* avoid exposing vnet%d in dumpxml or error outputs */
>>          template_ifname = 1;
>> @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
>>          }
>>          if (template_ifname)
>>              VIR_FREE(net->ifname);
>> -        return -1;
>> +        tapfd = -1;
>>      }
>>
>> +cleanup:
>> +    VIR_FREE(brname);
>> +
>>      return tapfd;
>>  }
>
> This generally looks OK.
>
>>
>> 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);
>
> Again, how can this ever be the case?  The first time through the loop vol is
> definitely NULL, so there's no possibility vol->target.path contains any
> allocated memory.  At the end of this loop, we assign the whole vol object to a
> slot in the pool->volumes array, set vol to NULL, and then subsequent iterations
> get a new vol object allocated.  So how can this ever be true?

I'm wrong again... I was too easy to add the fix along with the below fix.
I'll drop it as well.

>
>>          if (virAsprintf(&vol->target.path, "%s/%s",
>>                          pool->def->target.path,
>>                          vol->name) == -1)
>> @@ -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) {
>
> I'm not sure either way on this one, I didn't follow the code path back far
> enough.  What situations can lead to this?

This one will happen because here comes soon after virStorageVolDefParseString
and it (actually virStorageVolDefParseXML) always sets vol->target.path.


Thanks for your review!
  ozaki-r

>
> --
> Chris Lalancette
>