On Tue, Mar 19, 2019 at 05:30:03PM +0100, Ján Tomko wrote: > 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 <mpriv...@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
I'd add a virNWFilterBindingStealDef() API which returns the def to the caller, setting binding->def to NULL. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
