From: Peter Krempa <pkrempa@redhat.com>
Date: Thu, 5 Jun 2025 05:52:03 +0000 (+0200)
Subject: virsh: cmdEvent: Ensure that event callbacks are unregistered before returning
X-Git-Tag: v11.5.0-rc1~43
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c60b7cbe28cba3531ed4b0df4da7d9621531e838;p=thirdparty%2Flibvirt.git

virsh: cmdEvent: Ensure that event callbacks are unregistered before returning

Successful return from 'virConnectDomainEventDeregisterAny' does not
guarantee that there aren't still in-progress events being handled by
the callbacks. Since 'cmdEvent' passes in a slice from an array as the
private data of the callbacks, we must ensure that the array stays in
scope (it's auto-freed) for the whole time there are possible callbacks
being executed.

While in practice this doesn't happen as the callbacks are usually quick
enough to finish while unregistering stuff, placing a 'sleep(1)' into
e.g. 'virshEventLifecyclePrint' and starting a domain results in crash
of virsh with the following backtrace:

 Program terminated with signal SIGSEGV, Segmentation fault.
 #0  0x00005557b5cfd343 in virshEventPrintf (data=data@entry=0x5557db9619b0, fmt=fmt@entry=0x5557b5d5e527 "%s") at ../../../libvirt/tools/virsh-domain-event.c:252

 Thread 2 (Thread 0x7f59a54b7d00 (LWP 2097121)):
 #0  0x00007f59a6cadbf9 in __futex_abstimed_wait_common () at /lib64/libc.so.6
 #1  0x00007f59a6cb2cf3 in __pthread_clockjoin_ex () at /lib64/libc.so.6
 #2  0x00005557b5cd57f6 in virshDeinit (ctl=0x7ffc7b615140) at ../../../libvirt/tools/virsh.c:408
 #3  0x00005557b5cd5391 in main (argc=<optimized out>, argv=<optimized out>) at ../../../libvirt/tools/virsh.c:932

 Thread 1 (Thread 0x7f59a51a66c0 (LWP 2097122)):
 #0  0x00005557b5cfd343 in virshEventPrintf (data=data@entry=0x5557db9619b0, fmt=fmt@entry=0x5557b5d5e527 "%s") at ../../../libvirt/tools/virsh-domain-event.c:252
 #1  0x00005557b5cffa10 in virshEventPrint (data=0x5557db9619b0, buf=0x7f59a51a55c0) at ../../../libvirt/tools/virsh-domain-event.c:290
 #2  virshEventLifecyclePrint (conn=<optimized out>, dom=<optimized out>, event=<optimized out>, detail=<optimized out>, opaque=0x5557db9619b0) at ../../../libvirt/
 [snipped]

From the backtrace you can see that the 'main()' thread is already
shutting down virsh, which means that 'cmdEvent' terminated and the
private data was freed. The event loop thread is still execing the
callback which accesses the data.

To fix this add a condition and wait on all of the callbacks to be
unregistered first (their private data freeing function will be called).

This bug was observed when I've copied the event code for a new virsh
command which had a bit more involved callbacks.

Fixes: 99fa96c3907
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---

diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c
index 69a68d857d..9aa21b2e78 100644
--- a/tools/virsh-domain-event.c
+++ b/tools/virsh-domain-event.c
@@ -237,10 +237,23 @@ struct virshDomEventData {
     bool timestamp;
     virshDomainEventCallback *cb;
     int id;
+
+    virMutex *m; /* needed to signal that handler was unregistered for clean shutdown */
+    virCond *c;
 };
 typedef struct virshDomEventData virshDomEventData;
 
 
+static void
+virshDomEventDataUnregistered(virshDomEventData *d)
+{
+    g_auto(virLockGuard) name = virLockGuardLock(d->m);
+    /* signal that the handler was unregistered */
+    d->id = -1;
+    virCondSignal(d->c);
+}
+
+
 static void G_GNUC_PRINTF(2, 3)
 virshEventPrintf(virshDomEventData *data,
                  const char *fmt,
@@ -936,6 +949,8 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
     bool timestamp = vshCommandOptBool(cmd, "timestamp");
     int count = 0;
     virshControl *priv = ctl->privData;
+    g_auto(virMutex) m = VIR_MUTEX_INITIALIZER;
+    g_auto(virCond) c = VIR_COND_INITIALIZER;
 
     VSH_EXCLUSIVE_OPTIONS("all", "event");
     VSH_EXCLUSIVE_OPTIONS("list", "all");
@@ -969,6 +984,8 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
         data[ndata].count = &count;
         data[ndata].timestamp = timestamp;
         data[ndata].cb = &virshDomainEventCallbacks[i];
+        data[ndata].m = &m;
+        data[ndata].c = &c;
         data[ndata].id = -1;
         ndata++;
     }
@@ -994,7 +1011,7 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
                                                            data[i].event,
                                                            data[i].cb->cb,
                                                            &data[i],
-                                                           NULL)) < 0) {
+                                                           (virFreeCallback) virshDomEventDataUnregistered)) < 0) {
             /* When registering for all events: if the first
              * registration succeeds, silently ignore failures on all
              * later registrations on the assumption that the server
@@ -1022,14 +1039,27 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
         ret = true;
 
  cleanup:
-    vshEventCleanup(ctl);
     if (data) {
         for (i = 0; i < ndata; i++) {
             if (data[i].id >= 0 &&
                 virConnectDomainEventDeregisterAny(priv->conn, data[i].id) < 0)
                 ret = false;
         }
+
+        virMutexLock(&m);
+        while (true) {
+            for (i = 0; i < ndata; i++) {
+                if (data[i].id >= 0)
+                    break;
+            }
+
+            if (i == ndata ||
+                virCondWait(&c, &m) < 0)
+                break;
+        }
+        virMutexUnlock(&m);
     }
+    vshEventCleanup(ctl);
     return ret;
 }