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

[Ján Tomko]

On Tue, Mar 19, 2019 at 05:09:33PM +0100, Michal Privoznik wrote:
>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

Hmm, not good.
My other suggestions would be:
* create an ugly virNWFilterBindingObjResetDef API
* make another copy of the definition and make virNWFilterBindingObjListAdd
  always consume it

>diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>index fdfc6f48fa..8c2e987b5d 100644
>--- a/src/nwfilter/nwfilter_driver.c
>+++ b/src/nwfilter/nwfilter_driver.c
>@@ -759,7 +759,7 @@ nwfilterBindingCreateXML(virConnectPtr conn,
>         goto cleanup;
>
>     obj = virNWFilterBindingObjListAdd(driver->bindings,
>-                                       def);
>+                                       &def);
>     if (!obj)
>         goto cleanup;

As-is, def would be NULL even on success and dereferenced on the next
line.

Jano

>
>@@ -775,8 +775,7 @@ nwfilterBindingCreateXML(virConnectPtr conn,
>     virNWFilterBindingObjSave(obj, driver->bindingDir);
>
>  cleanup:
>-    if (!obj)
>-        virNWFilterBindingDefFree(def);
>+    virNWFilterBindingDefFree(def);
>     virNWFilterBindingObjEndAPI(&obj);
>
>     return ret;


>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
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190319/5d00f48f/attachment-0001.sig>