1. Patchew
  2. Libvirt
  3. View series

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

Michal Privoznik posted 1 patch 5 years, 1 month ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/5d88f12178fed08389b43900cd71b0b6b515129a.1552926459.git.mprivozn@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(-)
[libvirt] [PATCH] nwfilter: Don't crash when trying to add an nwfilter that's already being removed
Posted by Michal Privoznik 5 years, 1 month ago 
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@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)
 {
     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)
         goto error;
@@ -195,7 +196,7 @@ virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr bindings,
 
 virNWFilterBindingObjPtr
 virNWFilterBindingObjListAdd(virNWFilterBindingObjListPtr bindings,
-                             virNWFilterBindingDefPtr def)
+                             virNWFilterBindingDefPtr *def)
 {
     virNWFilterBindingObjPtr ret;
 
diff --git a/src/conf/virnwfilterbindingobjlist.h b/src/conf/virnwfilterbindingobjlist.h
index b0fb90f667..4047453634 100644
--- a/src/conf/virnwfilterbindingobjlist.h
+++ b/src/conf/virnwfilterbindingobjlist.h
@@ -35,7 +35,7 @@ virNWFilterBindingObjListFindByPortDev(virNWFilterBindingObjListPtr bindings,
 
 virNWFilterBindingObjPtr
 virNWFilterBindingObjListAdd(virNWFilterBindingObjListPtr bindings,
-                             virNWFilterBindingDefPtr def);
+                             virNWFilterBindingDefPtr *def);
 
 void
 virNWFilterBindingObjListRemove(virNWFilterBindingObjListPtr bindings,
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index fdfc6f48fa..8c2e987b5d 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -759,7 +759,7 @@ nwfilterBindingCreateXML(virConnectPtr conn,
         goto cleanup;
 
     obj = virNWFilterBindingObjListAdd(driver->bindings,
-                                       def);
+                                       &def);
     if (!obj)
         goto cleanup;
 
@@ -775,8 +775,7 @@ nwfilterBindingCreateXML(virConnectPtr conn,
     virNWFilterBindingObjSave(obj, driver->bindingDir);
 
  cleanup:
-    if (!obj)
-        virNWFilterBindingDefFree(def);
+    virNWFilterBindingDefFree(def);
     virNWFilterBindingObjEndAPI(&obj);
 
     return ret;
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Don't crash when trying to add an nwfilter that's already being removed
Posted by Ján Tomko 5 years, 1 month ago 
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@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@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Don't crash when trying to add an nwfilter that's already being removed
Posted by Michal Privoznik 5 years, 1 month ago 
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@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.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Don't crash when trying to add an nwfilter that's already being removed
Posted by Ján Tomko 5 years, 1 month ago 
On Tue, Mar 19, 2019 at 05:09:33PM +0100, Michal Privoznik wrote:
>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@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.
>
>How can this work? binding's structure is internal so the only way to do

Hmm, not good.
My other suggestions would be:
* create an ugly virNWFilterBindingObjResetDef API
* make another copy of the definition and make virNWFilterBindingObjListAdd
  always consume it

>diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>index fdfc6f48fa..8c2e987b5d 100644
>--- a/src/nwfilter/nwfilter_driver.c
>+++ b/src/nwfilter/nwfilter_driver.c
>@@ -759,7 +759,7 @@ nwfilterBindingCreateXML(virConnectPtr conn,
>         goto cleanup;
>
>     obj = virNWFilterBindingObjListAdd(driver->bindings,
>-                                       def);
>+                                       &def);
>     if (!obj)
>         goto cleanup;

As-is, def would be NULL even on success and dereferenced on the next
line.

Jano

>
>@@ -775,8 +775,7 @@ nwfilterBindingCreateXML(virConnectPtr conn,
>     virNWFilterBindingObjSave(obj, driver->bindingDir);
>
>  cleanup:
>-    if (!obj)
>-        virNWFilterBindingDefFree(def);
>+    virNWFilterBindingDefFree(def);
>     virNWFilterBindingObjEndAPI(&obj);
>
>     return ret;


>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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Don't crash when trying to add an nwfilter that's already being removed
Posted by Daniel P. Berrangé 5 years, 1 month ago 
On Tue, Mar 19, 2019 at 05:30:03PM +0100, Ján Tomko wrote:
> On Tue, Mar 19, 2019 at 05:09:33PM +0100, Michal Privoznik wrote:
> > 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@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.
> > 
> > How can this work? binding's structure is internal so the only way to do
> 
> Hmm, not good.
> My other suggestions would be:
> * create an ugly virNWFilterBindingObjResetDef API
> * make another copy of the definition and make virNWFilterBindingObjListAdd
>  always consume it

I'd add a virNWFilterBindingStealDef() API which returns the def
to the caller, setting binding->def to NULL.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.