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

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.