[libvirt PATCH] tests: fix double-lock of monitor in hotplug test

Daniel P. Berrangé posted 1 patch 4 years, 1 month ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200312183638.961024-1-berrange@redhat.com
tests/qemuhotplugtest.c | 5 +++++
1 file changed, 5 insertions(+)
[libvirt PATCH] tests: fix double-lock of monitor in hotplug test
Posted by Daniel P. Berrangé 4 years, 1 month ago
The qemuMonitorTestNew() function returns with the monitor object
locked, and expects it to still be locked when qemuMonitorTestFree
is called.  The qemuhotplug test, however, explicitly unlocks the
monitor, but then forgets to lock it again. As a result the
qemuMonitorTestFree function is unlocking a mutex it doesn't own.

This bug has existed forever, but since we use normal POSIX mutexes
and don't check the return value of pthread_mutex_lock/unlock we
didn't see the error. It was harmless until the switch to the per
monitor event loop which requires the thread synchronization to
work reliably, whereupon it started crashing.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemuhotplugtest.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 8b411d63f0..d9244dca44 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -337,6 +337,8 @@ testQemuHotplug(const void *data)
         ret = testQemuHotplugUpdate(vm, dev);
     }
 
+    virObjectLock(priv->mon);
+
  cleanup:
     VIR_FREE(domain_filename);
     VIR_FREE(device_filename);
@@ -378,6 +380,7 @@ static void
 testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data)
 {
     qemuDomainObjPrivatePtr priv;
+    qemuMonitorPtr mon;
 
     if (!data)
         return;
@@ -396,6 +399,8 @@ testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data)
         virObjectUnref(data->vm);
     }
 
+    mon = qemuMonitorTestGetMonitor(data->mon);
+    virObjectLock(mon);
     qemuMonitorTestFree(data->mon);
     VIR_FREE(data);
 }
-- 
2.24.1

Re: [libvirt PATCH] tests: fix double-lock of monitor in hotplug test
Posted by Peter Krempa 4 years, 1 month ago
On Thu, Mar 12, 2020 at 18:36:38 +0000, Daniel Berrange wrote:
> The qemuMonitorTestNew() function returns with the monitor object
> locked, and expects it to still be locked when qemuMonitorTestFree
> is called.  The qemuhotplug test, however, explicitly unlocks the
> monitor, but then forgets to lock it again. As a result the
> qemuMonitorTestFree function is unlocking a mutex it doesn't own.
> 
> This bug has existed forever, but since we use normal POSIX mutexes
> and don't check the return value of pthread_mutex_lock/unlock we
> didn't see the error. It was harmless until the switch to the per
> monitor event loop which requires the thread synchronization to
> work reliably, whereupon it started crashing.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qemuhotplugtest.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Peter Krempa <pkrempa@redhat.com>