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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/19/19 4:53 PM, Ján Tomko wrote:
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.

How can this work? binding's structure is internal so the only way to do this would be by calling virNWFilterBindingObjSetDef(binding, NULL) which would free @def immediatelly and thus we wouldn't avoid double free. Can you shed more light what you have on mind please?

Thanks,
Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux