On 3/20/19 9:11 AM, Michal Privoznik wrote: > On 3/20/19 12:17 PM, John Ferlan wrote: >> >> >> On 3/20/19 6:31 AM, 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. >>> >>> The solution is to unset binding->def in case of failure so it's >>> not freed in step 5). >>> >>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >>> --- >>> >>> Technically, this is a v2 of: >>> >>> https://www.redhat.com/archives/libvir-list/2019-March/msg01209.html >>> >>> But since this one implements different approach than v1 I'm not marking >>> it as such. >>> >>> src/conf/virnwfilterbindingobj.c | 10 ++++++++++ >>> src/conf/virnwfilterbindingobj.h | 3 +++ >>> src/conf/virnwfilterbindingobjlist.c | 4 ++++ >>> src/libvirt_private.syms | 1 + >>> 4 files changed, 18 insertions(+) >>> >> >> Why wouldn't your other series take care of this in a more efficient >> way?, e.g. patch 3/3: >> >> https://www.redhat.com/archives/libvir-list/2019-March/msg01321.html >> > > These are separate issues really. virHashAddEntry() can fail in more > cases than just 'Duplicate key'. For instance, on OOM. The patches you > reference help us prevent tickling this bug in case of 'Duplicate key' > but not really for OOM. > Ah... right... you could update the commit message above to indicate key error or oom error (just for posterity) >> Isn't the problem that we can get to the virNWFilterBindingObjSetDef and >> virNWFilterBindingObjListAddObjLocked because of the same portdevname >> key name which causes the virHashAddEntry to fail. >> >> If we fail after virNWFilterBindingObjListFindByPortDevLocked because >> the other patch recognized that the @binding was in the process of being >> removed, then we'd never create a new @obj with our new @def that had >> the duplicated portdevname key. > > Again, patch you refer to will only prevent from tickling this bug. > Right - just trying to complete my thought. >> >> >>> diff --git a/src/conf/virnwfilterbindingobj.c >>> b/src/conf/virnwfilterbindingobj.c >>> index 23978d4207..68afb9c434 100644 >>> --- a/src/conf/virnwfilterbindingobj.c >>> +++ b/src/conf/virnwfilterbindingobj.c >>> @@ -88,6 +88,16 @@ >>> virNWFilterBindingObjSetDef(virNWFilterBindingObjPtr obj, >>> } >>> +virNWFilterBindingDefPtr >>> +virNWFilterBindingObjStealDef(virNWFilterBindingObjPtr obj) >>> +{ >>> + virNWFilterBindingDefPtr def; >>> + >>> + VIR_STEAL_PTR(def, obj->def); >>> + return def; >>> +} >>> + >>> + >>> bool >>> virNWFilterBindingObjGetRemoving(virNWFilterBindingObjPtr obj) >>> { >>> diff --git a/src/conf/virnwfilterbindingobj.h >>> b/src/conf/virnwfilterbindingobj.h >>> index 8e5fbee35f..b26bb3c8ec 100644 >>> --- a/src/conf/virnwfilterbindingobj.h >>> +++ b/src/conf/virnwfilterbindingobj.h >>> @@ -39,6 +39,9 @@ void >>> virNWFilterBindingObjSetDef(virNWFilterBindingObjPtr obj, >>> virNWFilterBindingDefPtr def); >>> +virNWFilterBindingDefPtr >>> +virNWFilterBindingObjStealDef(virNWFilterBindingObjPtr obj); >>> + >>> bool >>> virNWFilterBindingObjGetRemoving(virNWFilterBindingObjPtr obj); >>> diff --git a/src/conf/virnwfilterbindingobjlist.c >>> b/src/conf/virnwfilterbindingobjlist.c >>> index 06ccbf53af..4ee2c1b194 100644 >>> --- a/src/conf/virnwfilterbindingobjlist.c >>> +++ b/src/conf/virnwfilterbindingobjlist.c >>> @@ -167,6 +167,7 @@ >>> virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr >>> bindings, >>> virNWFilterBindingDefPtr def) >>> { >>> virNWFilterBindingObjPtr binding; >>> + bool stealDef = false; >>> /* See if a binding with matching portdev already exists */ >>> if ((binding = virNWFilterBindingObjListFindByPortDevLocked( >>> @@ -181,6 +182,7 @@ >>> virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr >>> bindings, >>> goto error; >>> virNWFilterBindingObjSetDef(binding, def); >>> + stealDef = true; >>> if (virNWFilterBindingObjListAddObjLocked(bindings, binding) >>> < 0) >>> goto error; >>> @@ -188,6 +190,8 @@ >>> virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr >>> bindings, >>> return binding; >>> error: >>> + if (stealDef) >>> + virNWFilterBindingObjStealDef(binding); >> >> I think this is no different than modifying the failure path for >> virNWFilterBindingObjListAddObjLocked to clear out @def acknowledging >> our caller would free @def upon failure return. >> >> if !AddObj() { >> ObjSet(binding, NULL) >> goto error >> } > > As said in review for v1, virNWFilterBindingObjSetDef() calls > virNWFilterBindingDefFree(binding->def) and only after that it sets > binding->def to desired value. This approach won't help. > Ah right - mind block. Trying to recall other places and the order of when setting ->def occurs - for some reason I recall it always occurred after the Hash insertion was successful, hence the subsequent comment of changing order. That's more of a consistency type thing. I'm fine with this approach and see Jano already R-By'd it. Didn't want to leave you hanging with any unresolved questions/concerns. John >> >> Alternatively, reorder the code to do the ObjSetDef after successful >> return from virNWFilterBindingObjListAddLocked which would take >> def->portdevname as a "const char *key" type parameter; however, there >> could be some affect with virNWFilterBindingObjListLoadStatus >> processing. In particular how virNWFilterBindingObjParseXML sets >> obj->def... > > That's an alternative approach. There are two (or more) ways to fix this > bug. I've chosen one of them. What advantages does the other have that > it should be done that way? > >> >> BTW: When I read the create a StealDef I was thinking more in the terms >> of "if binding is in the process of being removed", then use the >> StealDef to return the current @binding->def so it could be Free'd and >> clear the removing bit. There may need to be some obj ref counting magic >> to make sure some other thread in the process of deleting the object >> doesn't do something bad or logic in that thread to recognize the @obj >> is no longer being deleted. I didn't dig into the removing code and >> there's not enough coffee in me yet to think more clearly about that. > > In general, I don't think such logic is desired. Just look at my > reproducer (1/3 from the linked series). One thread is doing 'destroy' > the other is doing 'create'. There can't be any logic that would make > both APIs happy, can it? One of the pair has to fail. > > Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
