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

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(a)redhat.com&gt; >> --- >> 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

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;

@@ -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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list