[libvirt] [PATCH] nwfilter: Don't crash when trying to add an nwfilter that's already being removed

[Michal Privoznik]

On 3/19/19 4:53 PM, Ján Tomko wrote:
> On Mon, Mar 18, 2019 at 05:27:39PM +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1686927
>>
>> When trying to create a nwfilter binding via
>> nwfilterBindingCreateXML() we may encounter a crash. The sequence
>> of functions called is as follows:
>>
>> 1) nwfilterBindingCreateXML() parses the XML and calls
>> virNWFilterBindingObjListAdd() which calls
>> virNWFilterBindingObjListAddLocked()
>>
>> 2) Here, @binding is not found because binding->remove is set.
>>
>> 3) Therefore, controls continue with creating new @binding,
>> setting its def to the one from 1) and adding it to the hash
>> table.
>>
>> 4) This fails, because the binding is still in the hash table
>> (duplicate key is detected).
>>
>> 5) The control jumps to 'error' label where
>> virNWFilterBindingObjEndAPI() is called which frees the binding
>> definition passed.
>>
>> 6) Error is propagated to the caller, which calls
>> virNWFilterBindingDefFree() over the definition again.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/conf/virnwfilterbindingobjlist.c | 11 ++++++-----
>> src/conf/virnwfilterbindingobjlist.h |  2 +-
>> src/nwfilter/nwfilter_driver.c       |  5 ++---
>> 3 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterbindingobjlist.c 
>> b/src/conf/virnwfilterbindingobjlist.c
>> index 06ccbf53af..87189da642 100644
>> --- a/src/conf/virnwfilterbindingobjlist.c
>> +++ b/src/conf/virnwfilterbindingobjlist.c
>> @@ -164,23 +164,24 @@ 
>> virNWFilterBindingObjListAddObjLocked(virNWFilterBindingObjListPtr 
>> bindings,
>>  */
>> static virNWFilterBindingObjPtr
>> virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr bindings,
>> -                                   virNWFilterBindingDefPtr def)
>> +                                   virNWFilterBindingDefPtr *def)
> 
> Rather than adding another return value to the whole chain,
> 
>> {
>>     virNWFilterBindingObjPtr binding;
>>
>>     /* See if a binding with matching portdev already exists */
>>     if ((binding = virNWFilterBindingObjListFindByPortDevLocked(
>> -             bindings, def->portdevname))) {
>> +             bindings, (*def)->portdevname))) {
>>         virReportError(VIR_ERR_OPERATION_FAILED,
>>                        _("binding '%s' already exists"),
>> -                       def->portdevname);
>> +                       (*def)->portdevname);
>>         goto error;
>>     }
>>
>>     if (!(binding = virNWFilterBindingObjNew()))
>>         goto error;
>>
>> -    virNWFilterBindingObjSetDef(binding, def);
>> +    virNWFilterBindingObjSetDef(binding, *def);
>> +    *def = NULL;
>>
>>     if (virNWFilterBindingObjListAddObjLocked(bindings, binding) < 0)
> 
> I'd simply set
>    binding->def = NULL;
> here, to match what virDomainObjListAddLocked does.

How can this work? binding's structure is internal so the only way to do 
this would be by calling virNWFilterBindingObjSetDef(binding, NULL) 
which would free @def immediatelly and thus we wouldn't avoid double 
free. Can you shed more light what you have on mind please?

Thanks,
Michal