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@xxxxxxxxxx> --- 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. That way def is only consumed on success (which is what nwfilterBindingCreateXML expects). With that change: Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx> Jano
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
