[libvirt] [PATCH] Fix several memory leaks

[Daniel P. Berrange]

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

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

ACK

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

ACK

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

ACK

> 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.

> @@ -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. 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'.

Regards.
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|