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

[Ján Tomko]

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 at 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.

That way def is only consumed on success (which is what
nwfilterBindingCreateXML expects).

With that change:
Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190319/3be61660/attachment-0001.sig>