[libvirt PATCH] tests: fix deadlock in eventtest

Pavel Hrdina 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/bb218940e893100ac71f34431d702013f85867da.1580991724.git.phrdina@redhat.com
tests/eventtest.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
[libvirt PATCH] tests: fix deadlock in eventtest
Posted by Pavel Hrdina 4 years, 1 month ago
There is a race deadlock in eventtest after the recent rewrite to drop
GNULIB from libvirt code base.

The issue happens when the callbacks testPipeReader() or testTimer()
are called before waitEvents() starts waiting on `eventThreadCond`.
It will never happen because the callbacks are already done and there
is nothing that will signal the condition again.

Reported-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 tests/eventtest.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/eventtest.c b/tests/eventtest.c
index 9855b578fb..fc814922f2 100644
--- a/tests/eventtest.c
+++ b/tests/eventtest.c
@@ -43,6 +43,7 @@ VIR_LOG_INIT("tests.eventtest");
 
 static pthread_mutex_t eventThreadMutex = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t eventThreadCond = PTHREAD_COND_INITIALIZER;
+static bool eventThreadSignaled;
 
 static struct handleInfo {
     int pipeFD[2];
@@ -138,8 +139,9 @@ testPipeReader(int watch, int fd, int events, void *data)
         virEventRemoveHandle(info->delete);
 
  cleanup:
-    pthread_mutex_unlock(&eventThreadMutex);
     pthread_cond_signal(&eventThreadCond);
+    eventThreadSignaled = true;
+    pthread_mutex_unlock(&eventThreadMutex);
 }
 
 
@@ -164,8 +166,9 @@ testTimer(int timer, void *data)
         virEventRemoveTimeout(info->delete);
 
  cleanup:
-    pthread_mutex_unlock(&eventThreadMutex);
     pthread_cond_signal(&eventThreadCond);
+    eventThreadSignaled = true;
+    pthread_mutex_unlock(&eventThreadMutex);
 }
 
 G_GNUC_NORETURN static void *eventThreadLoop(void *data G_GNUC_UNUSED) {
@@ -185,7 +188,10 @@ waitEvents(int nhandle, int ntimer)
     VIR_DEBUG("Wait events nhandle %d ntimer %d",
               nhandle, ntimer);
     while (ngothandle != nhandle || ngottimer != ntimer) {
-        pthread_cond_wait(&eventThreadCond, &eventThreadMutex);
+        while (!eventThreadSignaled)
+            pthread_cond_wait(&eventThreadCond, &eventThreadMutex);
+
+        eventThreadSignaled = 0;
 
         ngothandle = ngottimer = 0;
         for (i = 0; i < NUM_FDS; i++) {
-- 
2.24.1

Re: [libvirt PATCH] tests: fix deadlock in eventtest
Posted by Daniel P. Berrangé 4 years, 1 month ago
On Thu, Feb 06, 2020 at 01:22:28PM +0100, Pavel Hrdina wrote:
> There is a race deadlock in eventtest after the recent rewrite to drop
> GNULIB from libvirt code base.
> 
> The issue happens when the callbacks testPipeReader() or testTimer()
> are called before waitEvents() starts waiting on `eventThreadCond`.
> It will never happen because the callbacks are already done and there
> is nothing that will signal the condition again.
> 
> Reported-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  tests/eventtest.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [libvirt PATCH] tests: fix deadlock in eventtest
Posted by Peter Krempa 4 years, 1 month ago
On Thu, Feb 06, 2020 at 13:22:28 +0100, Pavel Hrdina wrote:
> There is a race deadlock in eventtest after the recent rewrite to drop
> GNULIB from libvirt code base.
> 
> The issue happens when the callbacks testPipeReader() or testTimer()
> are called before waitEvents() starts waiting on `eventThreadCond`.
> It will never happen because the callbacks are already done and there
> is nothing that will signal the condition again.
> 
> Reported-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  tests/eventtest.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

[...]

> @@ -185,7 +188,10 @@ waitEvents(int nhandle, int ntimer)
>      VIR_DEBUG("Wait events nhandle %d ntimer %d",
>                nhandle, ntimer);
>      while (ngothandle != nhandle || ngottimer != ntimer) {
> -        pthread_cond_wait(&eventThreadCond, &eventThreadMutex);
> +        while (!eventThreadSignaled)
> +            pthread_cond_wait(&eventThreadCond, &eventThreadMutex);
> +
> +        eventThreadSignaled = 0;

s/0/false/

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

>  
>          ngothandle = ngottimer = 0;
>          for (i = 0; i < NUM_FDS; i++) {
> -- 
> 2.24.1
>