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

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(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 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