Re: [libvirt] [PATCH] Fix several memory leaks

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?

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

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

-- Chris Lalancette