[libvirt] [PATCH 2/4] nwfilter: Fix potential locking problems on ObjLoad failure

[John Ferlan]

On 04/24/2016 07:22 PM, Cole Robinson wrote:
> In virNWFilterObjLoad we can still fail after virNWFilterObjAssignDef,
> but we don't unlock and free the created virNWFilterObjPtr in the
> cleanup path.
> 
> The bit we are trying to do after AssignDef is just STRDUP in the
> configFile path. However caching the configFile in the NWFilterObj
> is largely redundant and doesn't follow the same pattern we use
> for domain and network objects.
> 
> So just remove all the configFile caching which fixes the latent
> bug as a side effect.
> ---
>  src/conf/nwfilter_conf.c       | 62 ++++++++++++++++++++----------------------
>  src/conf/nwfilter_conf.h       |  5 ++--
>  src/nwfilter/nwfilter_driver.c |  6 ++--
>  3 files changed, 34 insertions(+), 39 deletions(-)
> 

Fine with the concept - seems as though there's still some streamlining
that can be done - possibly as a followup sometime.  Seeing as
virNWFilterConfigFile essentially does the same thing as virFileBuildPath.

Trying to compare with network (and domain) would then just be easier.
Those are the models I used for secrets.

ACK to what's here (extra credit saved for the future).

John

> diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> index ced8eb8..d02bbff 100644
> --- a/src/conf/nwfilter_conf.c
> +++ b/src/conf/nwfilter_conf.c
> @@ -353,8 +353,6 @@ virNWFilterObjFree(virNWFilterObjPtr obj)
>      virNWFilterDefFree(obj->def);
>      virNWFilterDefFree(obj->newDef);
>  
> -    VIR_FREE(obj->configFile);
> -
>      virMutexDestroy(&obj->lock);
>  
>      VIR_FREE(obj);
> @@ -3181,12 +3179,6 @@ virNWFilterObjLoad(virNWFilterObjListPtr nwfilters,
>          return NULL;
>      }
>  
> -    VIR_FREE(nwfilter->configFile); /* for driver reload */
> -    if (VIR_STRDUP(nwfilter->configFile, path) < 0) {
> -        virNWFilterDefFree(def);
> -        return NULL;
> -    }
> -
>      return nwfilter;
>  }
>  
> @@ -3234,60 +3226,66 @@ virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters,
>  
>  int
>  virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver,
> -                      virNWFilterObjPtr nwfilter,
>                        virNWFilterDefPtr def)
>  {
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      char *xml;
> -    int ret;
> -
> -    if (!nwfilter->configFile) {
> -        if (virFileMakePath(driver->configDir) < 0) {
> -            virReportSystemError(errno,
> -                                 _("cannot create config directory %s"),
> -                                 driver->configDir);
> -            return -1;
> -        }
> +    int ret = -1;
> +    char *configFile = NULL;
>  
> -        if (!(nwfilter->configFile = virFileBuildPath(driver->configDir,
> -                                                      def->name, ".xml"))) {
> -            return -1;
> -        }
> +    if (virFileMakePath(driver->configDir) < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot create config directory %s"),
> +                             driver->configDir);
> +        goto error;
> +    }
> +
> +    if (!(configFile = virFileBuildPath(driver->configDir,
> +                                        def->name, ".xml"))) {
> +        goto error;
>      }
>  
>      if (!(xml = virNWFilterDefFormat(def))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         "%s", _("failed to generate XML"));
> -        return -1;
> +        goto error;
>      }
>  
>      virUUIDFormat(def->uuid, uuidstr);
> -    ret = virXMLSaveFile(nwfilter->configFile,
> +    ret = virXMLSaveFile(configFile,
>                           virXMLPickShellSafeComment(def->name, uuidstr),
>                           "nwfilter-edit", xml);
>      VIR_FREE(xml);
>  
> + error:
> +    VIR_FREE(configFile);
>      return ret;
>  }
>  
>  
>  int
> -virNWFilterObjDeleteDef(virNWFilterObjPtr nwfilter)
> +virNWFilterObjDeleteDef(const char *configDir,
> +                        virNWFilterObjPtr nwfilter)
>  {
> -    if (!nwfilter->configFile) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("no config file for %s"), nwfilter->def->name);
> -        return -1;
> +    int ret = -1;
> +    char *configFile = NULL;
> +
> +    if (!(configFile = virFileBuildPath(configDir,
> +                                        nwfilter->def->name, ".xml"))) {
> +        goto error;
>      }
>  
> -    if (unlink(nwfilter->configFile) < 0) {
> +    if (unlink(configFile) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("cannot remove config for %s"),
>                         nwfilter->def->name);
> -        return -1;
> +        goto error;
>      }
>  
> -    return 0;
> +    ret = 0;
> + error:
> +    VIR_FREE(configFile);
> +    return ret;
>  }
>  
>  
> diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
> index 0211861..823cfa4 100644
> --- a/src/conf/nwfilter_conf.h
> +++ b/src/conf/nwfilter_conf.h
> @@ -551,7 +551,6 @@ typedef virNWFilterObj *virNWFilterObjPtr;
>  struct _virNWFilterObj {
>      virMutex lock;
>  
> -    char *configFile;
>      int active;
>      int wantRemoved;
>  
> @@ -612,10 +611,10 @@ virNWFilterObjPtr virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters,
>  
>  
>  int virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver,
> -                          virNWFilterObjPtr nwfilter,
>                            virNWFilterDefPtr def);
>  
> -int virNWFilterObjDeleteDef(virNWFilterObjPtr nwfilter);
> +int virNWFilterObjDeleteDef(const char *configDir,
> +                            virNWFilterObjPtr nwfilter);
>  
>  virNWFilterObjPtr virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
>                                            virNWFilterDefPtr def);
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 1a868b6..2828b28 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -554,7 +554,7 @@ nwfilterDefineXML(virConnectPtr conn,
>      if (!(nwfilter = virNWFilterObjAssignDef(&driver->nwfilters, def)))
>          goto cleanup;
>  
> -    if (virNWFilterObjSaveDef(driver, nwfilter, def) < 0) {
> +    if (virNWFilterObjSaveDef(driver, def) < 0) {
>          virNWFilterObjRemove(&driver->nwfilters, nwfilter);
>          def = NULL;
>          goto cleanup;
> @@ -602,11 +602,9 @@ nwfilterUndefine(virNWFilterPtr obj)
>          goto cleanup;
>      }
>  
> -    if (virNWFilterObjDeleteDef(nwfilter) < 0)
> +    if (virNWFilterObjDeleteDef(driver->configDir, nwfilter) < 0)
>          goto cleanup;
>  
> -    VIR_FREE(nwfilter->configFile);
> -
>      virNWFilterObjRemove(&driver->nwfilters, nwfilter);
>      nwfilter = NULL;
>      ret = 0;
>