[PATCH] virnwfilterbindingobj: Fix virNWFilterBindingObjNew()

Michal Privoznik posted 1 patch 2 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/2d59155c7fb329037fc228d6bf7dd601c4c3e7f4.1643711953.git.mprivozn@redhat.com
src/conf/virnwfilterbindingobj.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH] virnwfilterbindingobj: Fix virNWFilterBindingObjNew()
Posted by Michal Privoznik 2 years, 3 months ago
The idea behind virNWFilterBindingObjNew() is to create and
return an object of virNWFilterBindingObjClass class. The class
is virObjectLockable (and the corresponding
_virNWFilterBindingObj structure has virObjectLockable parent).
But for some reason plain virObjectNew() is called. This is wrong
because the mutex in the parent is left uninitialized.

Next, the returned object is not locked. This is wrong because in
some cases the returned object is added onto a list of bindings
and then passed to virNWFilterBindingObjEndAPI() which unlocks it
right away. This is potentially dangerous because we might just
have unlocked the object for another thread.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/virnwfilterbindingobj.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/conf/virnwfilterbindingobj.c b/src/conf/virnwfilterbindingobj.c
index acea240b5d..d387af68c0 100644
--- a/src/conf/virnwfilterbindingobj.c
+++ b/src/conf/virnwfilterbindingobj.c
@@ -57,10 +57,15 @@ VIR_ONCE_GLOBAL_INIT(virNWFilterBindingObj);
 virNWFilterBindingObj *
 virNWFilterBindingObjNew(void)
 {
+    virNWFilterBindingObj *ret;
     if (virNWFilterBindingObjInitialize() < 0)
         return NULL;
 
-    return virObjectNew(virNWFilterBindingObjClass);
+    if (!(ret = virObjectLockableNew(virNWFilterBindingObjClass)))
+        return NULL;
+
+    virObjectLock(ret);
+    return ret;
 }
 
 
-- 
2.34.1

Re: [PATCH] virnwfilterbindingobj: Fix virNWFilterBindingObjNew()
Posted by Ján Tomko 2 years, 3 months ago
On a Tuesday in 2022, Michal Privoznik wrote:
>The idea behind virNWFilterBindingObjNew() is to create and
>return an object of virNWFilterBindingObjClass class. The class
>is virObjectLockable (and the corresponding
>_virNWFilterBindingObj structure has virObjectLockable parent).
>But for some reason plain virObjectNew() is called. This is wrong
>because the mutex in the parent is left uninitialized.
>
>Next, the returned object is not locked. This is wrong because in
>some cases the returned object is added onto a list of bindings
>and then passed to virNWFilterBindingObjEndAPI() which unlocks it
>right away. This is potentially dangerous because we might just
>have unlocked the object for another thread.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/conf/virnwfilterbindingobj.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano