[libvirt PATCH 0/2] glib: Avoid yet another crash due to race in event loop code
Similarly to commit 0db4743645b7 we need to fix the other event loop as well. To be completely honest I do not completely understand all the details in the original workaround, so I just hope this is the best way to fix the issue. Hopefully-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1931331 Martin Kletzander (2): util: Move glib event loop workaround to glibcompat glib: Use safe glib event workaround in other event loops src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_monitor.c | 2 +- src/util/glibcompat.c | 35 +++++++++++++++++++++++++++++++++++ src/util/glibcompat.h | 11 +++++++++++ src/util/vireventglib.c | 30 ------------------------------ src/util/vireventthread.c | 2 +- 6 files changed, 49 insertions(+), 33 deletions(-) -- 2.30.1
This way it can be used from other places as well. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/glibcompat.c | 35 +++++++++++++++++++++++++++++++++++ src/util/glibcompat.h | 11 +++++++++++ src/util/vireventglib.c | 30 ------------------------------ 3 files changed, 46 insertions(+), 30 deletions(-) diff --git a/src/util/glibcompat.c b/src/util/glibcompat.c index 9f0f7f015cad..76fda18a62d8 100644 --- a/src/util/glibcompat.c +++ b/src/util/glibcompat.c @@ -211,3 +211,38 @@ vir_g_strdup_vprintf(const char *msg, va_list args) abort(); return ret; } + + +/* + * If the last reference to a GSource is released in a non-main + * thread we're exposed to a race condition that causes a + * crash: + * + * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358 + * + * Thus we're using an idle func to release our ref... + * + * ...but this imposes a significant performance penalty on + * I/O intensive workloads which are sensitive to the iterations + * of the event loop, so avoid the workaround if we know we have + * new enough glib. + * + * The function below is used from a header file definition. + * + * Drop when min glib >= 2.64.0 + */ +#if GLIB_CHECK_VERSION(2, 64, 0) +# define g_vir_source_unref_safe(source) g_source_unref(source) +#else +# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle, source) + +gboolean +virEventGLibSourceUnrefIdle(gpointer data) +{ + GSource *src = data; + + g_source_unref(src); + + return FALSE; +} +#endif diff --git a/src/util/glibcompat.h b/src/util/glibcompat.h index 6463c4179a7e..9c528432742b 100644 --- a/src/util/glibcompat.h +++ b/src/util/glibcompat.h @@ -84,3 +84,14 @@ char *vir_g_strdup_vprintf(const char *msg, va_list args) #define g_canonicalize_filename vir_g_canonicalize_filename #undef g_fsync #define g_fsync vir_g_fsync + +/* Drop when min glib >= 2.64.0 */ +#if GLIB_CHECK_VERSION(2, 64, 0) +# define g_vir_source_unref_safe(source) g_source_unref(source) +#else +# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle, source) + +gboolean +virEventGLibSourceUnrefIdle(gpointer data); + +#endif diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c index 193b3bfde240..88e3ec6d5dbb 100644 --- a/src/util/vireventglib.c +++ b/src/util/vireventglib.c @@ -185,36 +185,6 @@ virEventGLibHandleFind(int watch) return NULL; } -/* - * If the last reference to a GSource is released in a non-main - * thread we're exposed to a race condition that causes a - * crash: - * - * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358 - * - * Thus we're using an idle func to release our ref... - * - * ...but this imposes a significant performance penalty on - * I/O intensive workloads which are sensitive to the iterations - * of the event loop, so avoid the workaround if we know we have - * new enough glib. - */ -#if GLIB_CHECK_VERSION(2, 64, 0) -# define g_vir_source_unref_safe(source) g_source_unref(source) -#else -# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle, source) - -static gboolean -virEventGLibSourceUnrefIdle(gpointer data) -{ - GSource *src = data; - - g_source_unref(src); - - return FALSE; -} -#endif - static void virEventGLibHandleUpdate(int watch, -- 2.30.1
On Thu, Mar 04, 2021 at 10:48:10AM +0100, Martin Kletzander wrote:
This way it can be used from other places as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/glibcompat.c | 35 +++++++++++++++++++++++++++++++++++ src/util/glibcompat.h | 11 +++++++++++ src/util/vireventglib.c | 30 ------------------------------ 3 files changed, 46 insertions(+), 30 deletions(-)
9rev 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 :|
On Thu, Mar 04, 2021 at 10:20:14AM +0000, Daniel P. Berrangé wrote:
On Thu, Mar 04, 2021 at 10:48:10AM +0100, Martin Kletzander wrote:
This way it can be used from other places as well.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/glibcompat.c | 35 +++++++++++++++++++++++++++++++++++ src/util/glibcompat.h | 11 +++++++++++ src/util/vireventglib.c | 30 ------------------------------ 3 files changed, 46 insertions(+), 30 deletions(-)
9rev
which should be 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 :|
Similarly to the crash workaround: commit 0db4743645b7a0611a3c0687f834205c9956f7fc Author: Daniel P. Berrangé <berrange@redhat.com> Date: Tue Jul 28 16:52:47 2020 +0100 util: avoid crash due to race in glib event loop code we need to do this in the other event loop as crash in that one was also reported: https://bugzilla.redhat.com/show_bug.cgi?id=1931331 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_monitor.c | 2 +- src/util/vireventthread.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 9a74b802b89d..8b880d0d157c 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -533,7 +533,7 @@ qemuAgentUnregister(qemuAgentPtr agent) { if (agent->watch) { g_source_destroy(agent->watch); - g_source_unref(agent->watch); + g_vir_source_unref_safe(agent->watch); agent->watch = NULL; } } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 73f337a6be53..b4f2641504f8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -862,7 +862,7 @@ qemuMonitorUnregister(qemuMonitorPtr mon) { if (mon->watch) { g_source_destroy(mon->watch); - g_source_unref(mon->watch); + g_vir_source_unref_safe(mon->watch); mon->watch = NULL; } } diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c index 8342f420f666..e9d18d3acf2f 100644 --- a/src/util/vireventthread.c +++ b/src/util/vireventthread.c @@ -123,7 +123,7 @@ virEventThreadWorker(void *opaque) g_main_loop_run(data->loop); - g_source_unref(running); + g_vir_source_unref_safe(running); virEventThreadDataFree(data); return NULL; -- 2.30.1
On Thu, Mar 04, 2021 at 10:48:11AM +0100, Martin Kletzander wrote:
Similarly to the crash workaround:
commit 0db4743645b7a0611a3c0687f834205c9956f7fc Author: Daniel P. Berrangé <berrange@redhat.com> Date: Tue Jul 28 16:52:47 2020 +0100
util: avoid crash due to race in glib event loop code
we need to do this in the other event loop as crash in that one was also reported:
https://bugzilla.redhat.com/show_bug.cgi?id=1931331
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_monitor.c | 2 +- src/util/vireventthread.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 9a74b802b89d..8b880d0d157c 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -533,7 +533,7 @@ qemuAgentUnregister(qemuAgentPtr agent) { if (agent->watch) { g_source_destroy(agent->watch); - g_source_unref(agent->watch); + g_vir_source_unref_safe(agent->watch); agent->watch = NULL; } } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 73f337a6be53..b4f2641504f8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -862,7 +862,7 @@ qemuMonitorUnregister(qemuMonitorPtr mon) { if (mon->watch) { g_source_destroy(mon->watch); - g_source_unref(mon->watch); + g_vir_source_unref_safe(mon->watch); mon->watch = NULL; } }
Upto here: Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c index 8342f420f666..e9d18d3acf2f 100644 --- a/src/util/vireventthread.c +++ b/src/util/vireventthread.c @@ -123,7 +123,7 @@ virEventThreadWorker(void *opaque)
g_main_loop_run(data->loop);
- g_source_unref(running); + g_vir_source_unref_safe(running);
This one isn't desirable as it'll cause a memory leak. This method is the very thread that was running the event loop, so there's no possibility of a race condition. Also the loop has stopped running at this point, so the idle source will never run. Just drop this hunk 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 :|
participants (2)
-
Daniel P. Berrangé -
Martin Kletzander