[libvirt] [PATCH] nwfilter: Don't crash when trying to add an nwfilter that's already being removed
Ján Tomko
jtomko at redhat.com
Tue Mar 19 15:53:36 UTC 2019
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>
More information about the libvir-list
mailing list