On 10/26/26 16:05, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain
> it was sent from.
> Do not click links or open attachments unless it is an email you expected to
> receive.
> From: Denis V. Lunev <[email protected]>
>
> virObjectEventStateQueueRemote() checks state->timer before taking the
> state lock. If the last callback is removed in between,
> virObjectEventStateCleanupTimer() removes the timer and clears the
> queue, and the event is then queued with no timer to flush it. A
> callback registered later gets a new timer, but that timer is armed
> only when a push takes the queue from empty to one event, which never
> happens again. Neither the stale event nor anything queued after it is
> dispatched, and no client receives events until the daemon restarts.
>
> A client holding the last callback can trigger this by exiting: its
> connection drops the callbacks while the hypervisor reports the side
> effects of the exit, e.g. VNC_DISCONNECTED for a VNC session it had
> open.
>
> Check the timer under the lock.
>
> The test removes the last callback while another thread is blocked
> queueing an event, using a freecb that runs under the state lock, and
> then checks that a newly registered callback still receives events.
>
> Signed-off-by: Denis V. Lunev <[email protected]>
> ---
> Seen on a host where a management daemon was the only event consumer:
> after it was restarted with a VNC session open, libvirtd delivered no
> domain events to anyone, a fresh 'virsh event --all' included, until
> libvirtd itself was restarted. Restarting that daemon in a loop with a
> VM running reproduced it in the 11th round; gdb on libvirtd showed
> timer == -1 with queue->count == 1 right after the daemon exited.
>
> The new test failed 5 of 5 runs without the fix and passed 20 of 20
> with it. With the fix the event is simply dropped, so the test does not
> depend on thread timing. The whole test suite, syntax-check included,
> passes.
>
> object_event.c has had the unlocked check since it was split out of
> domain_event.c in v1.2.1, and domain_event.c had it before that.
>
> src/conf/object_event.c | 5 ++-
> tests/objecteventtest.c | 92 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/object_event.c b/src/conf/object_event.c
> index 5fb96cb7aa..8b0669d660 100644
> --- a/src/conf/object_event.c
> +++ b/src/conf/object_event.c
> @@ -727,13 +727,14 @@ virObjectEventStateQueueRemote(virObjectEventState
> *state,
> if (!event)
> return;
>
> + virObjectLock(state);
> +
> if (state->timer < 0) {
> + virObjectUnlock(state);
> virObjectUnref(event);
> return;
> }
>
> - virObjectLock(state);
> -
> event->remoteID = remoteID;
> if (virObjectEventQueuePush(state->queue, event) < 0) {
> VIR_DEBUG("Error adding event to queue");
> diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c
> index 7110f54139..6b43d3c44d 100644
> --- a/tests/objecteventtest.c
> +++ b/tests/objecteventtest.c
> @@ -22,6 +22,7 @@
> #include <unistd.h>
>
> #include "testutils.h"
> +#include "virthread.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> @@ -444,6 +445,94 @@ testDomainStartStopEvent(const void *data)
> return ret;
> }
>
> +typedef struct {
> + lifecycleEventCounter counter;
> + virConnectPtr conn;
> + virThread thread;
> + bool started;
> +} queueRaceData;
> +
> +static void
> +queueRaceDefine(void *opaque)
> +{
> + virDomainPtr dom = virDomainDefineXML(opaque, domainDef);
> +
> + if (dom)
> + virDomainFree(dom);
> +}
> +
> +/* Runs under the event state lock while the last callback is removed */
> +static void
> +queueRaceFree(void *opaque)
> +{
> + queueRaceData *race = opaque;
> +
> + if (virThreadCreate(&race->thread, true, queueRaceDefine, race->conn) <
> 0)
> + return;
> + race->started = true;
> + g_usleep(100 * 1000);
> +}
> +
> +static void
> +queueRaceWatchdog(int timer G_GNUC_UNUSED,
> + void *opaque G_GNUC_UNUSED)
> +{
> +}
> +
> +static int
> +testDomainEventQueueRace(const void *data)
> +{
> + const objecteventTest *test = data;
> + queueRaceData race = { .conn = test->conn };
> + lifecycleEventCounter counter = { 0 };
> + int eventId = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
> + virDomainPtr dom = NULL;
> + int timer = -1;
> + int id;
> + int ret = -1;
> +
> + id = virConnectDomainEventRegisterAny(test->conn, NULL, eventId,
> + VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb),
> + &race, queueRaceFree);
> + if (id < 0)
> + return -1;
> +
> + virConnectDomainEventDeregisterAny(test->conn, id);
> + if (!race.started)
> + return -1;
> + virThreadJoin(&race.thread);
> +
> + id = virConnectDomainEventRegisterAny(test->conn, NULL, eventId,
> + VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb),
> + &counter, NULL);
> + if (id < 0)
> + return -1;
> +
> + if (!(dom = virDomainLookupByName(test->conn, "test-domain")) ||
> + virDomainUndefine(dom) < 0)
> + goto cleanup;
> +
> + /* A stuck queue never fires, so do not wait for it forever */
> + if ((timer = virEventAddTimeout(1000, queueRaceWatchdog, NULL, NULL)) <
> 0)
> + goto cleanup;
> +
> + if (virEventRunDefaultImpl() < 0)
> + goto cleanup;
> +
> + if (counter.undefineEvents != 1 || counter.unexpectedEvents > 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + if (timer >= 0)
> + virEventRemoveTimeout(timer);
> + virConnectDomainEventDeregisterAny(test->conn, id);
> + if (dom)
> + virDomainFree(dom);
> +
> + return ret;
> +}
> +
> static int
> testNetworkCreateXML(const void *data)
> {
> @@ -804,6 +893,9 @@ mymain(void)
> ret = EXIT_FAILURE;
> if (virTestRun("Domain start stop events", testDomainStartStopEvent,
> &test) < 0)
> ret = EXIT_FAILURE;
> + if (virTestRun("Domain event queued while last callback is removed",
> + testDomainEventQueueRace, &test) < 0)
> + ret = EXIT_FAILURE;
>
> /* Network event tests */
> /* Tests requiring the test network not to be set up */
>
> base-commit: 4d514f852c44f05f34e024438a7be004822c059d
ping
