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

[Michal Privoznik]

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 at redhat.com>
>>> ---
>>>  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.

Michal