6:17 a.m.
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(a)redhat.com>
---
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
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.
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
}
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...
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.
John
virNWFilterBindingObjEndAPI(&binding);
return NULL;
}
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 26f10bd47f..a33f9e61b1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1101,6 +1101,7 @@ virNWFilterBindingObjParseFile;
virNWFilterBindingObjSave;
virNWFilterBindingObjSetDef;
virNWFilterBindingObjSetRemoving;
+virNWFilterBindingObjStealDef;
# conf/virnwfilterbindingobjlist.h