Re: [libvirt] [PATCH 02/17] nwfilter: Fix possible corruption on failure path during LoadConfig

On 07/14/2017 01:50 AM, John Ferlan wrote: > > > On 07/13/2017 10:41 AM, Michal Privoznik wrote: >> On 06/02/2017 12:25 PM, John Ferlan wrote: >>> If the virNWFilterSaveConfig in virNWFilterObjListLoadConfig, then jumping >> >> s/,/ fails,/ >> >>> to the error label would free the @def owned by the object, but the object >>> is still on the list. >>> >>> Fix this by following proper procedure to clear @def and create a new >>> variable @objdef to handle the object's def after successful assignment. >>> >>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>> src/conf/virnwfilterobj.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c >>> index 0027d45..3fb6046 100644 >>> --- a/src/conf/virnwfilterobj.c >>> +++ b/src/conf/virnwfilterobj.c >>> @@ -485,6 +485,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, >>> { >>> virNWFilterDefPtr def = NULL; >>> virNWFilterObjPtr obj; >>> + virNWFilterDefPtr objdef; >>> char *configFile = NULL; >>> >>> if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) >>> @@ -503,10 +504,12 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, >>> >>> if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) >>> goto error; >>> + def = NULL; >>> + objdef = obj->def; >>> >>> /* We generated a UUID, make it permanent by saving the config to disk */ >>> - if (!def->uuid_specified && >>> - virNWFilterSaveConfig(configDir, def) < 0) >>> + if (!objdef->uuid_specified && >>> + virNWFilterSaveConfig(configDir, objdef) < 0) >>> goto error; >> >> This @objdef variable looks redundant to me. Can't we access obj->def >> directly? Esp. since you're introducing a variable just for a two times >> use. Then again, we often use obj->def->... when it comes to domain >> objects, why not here? >> > > If obj->def ever gets "consumed" by the virObject code, then obj->def->x > will fail miserably. That was the original design goal. I've since had > to scale back. I guess I could do "obj->def->uuid_specified" as it > doesn't really matter until the day obj->def-> is no longer accessible. > > As a preference - I like the extra local variable. In the long run the > compiler will do away with it, but for me it just reads better that way. > The deeper one gets into a->b->c->d[->e...] the more insane it gets. > Forcing one level of indirection is just more readable to me. Well, then don't run the following: libvirt.git $ git grep -np "\(\w\+\(->\|\.\)\)\{10\}\w\+"

I mean, that's one extreme, but we usually have "vm->def->name" so obj->def->uuid_specified shouldn't be a problem. But I don't care that much. So your call after all.