[Qemu-devel] [PATCH] osdep: initialize glib threads in all QEMU tools

[Qemu-devel] [PATCH] osdep: initialize glib threads in all QEMU tools

[Stefan Hajnoczi]

glib versions prior to 2.31.0 require an explicit g_thread_init() call
to enable multi-threading.

Failure to initialize threading causes glib to take single-threaded code
paths without synchronization.  For example, the g_slice allocator will
crash due to race conditions.

Fix this for all QEMU tool programs (qemu-nbd, qemu-io, qemu-img) by
moving the g_thread_init() call from vl.c:main() into a new
osdep.c:thread_init() constructor function.

thread_init() has __attribute__((constructor)) and is automatically
invoked by the runtime during startup.

We can now drop the "simple" trace backend's g_thread_init() call since
thread_init() already called it.

Note that we must keep coroutine-gthread.c's g_thread_init() call which
is located in a constructor function.  There is no guarantee for
constructor function ordering so thread_init() may only be called later.

Reported-by: Mario de Chenno <mario.dechenno@unina2.it>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 trace/simple.c |  9 ---------
 util/osdep.c   | 18 ++++++++++++++++++
 vl.c           |  8 --------
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/trace/simple.c b/trace/simple.c
index 1e3f691..7833309 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -404,15 +404,6 @@ bool trace_backend_init(const char *events, const char *file)
 {
     GThread *thread;
 
-    if (!g_thread_supported()) {
-#if !GLIB_CHECK_VERSION(2, 31, 0)
-        g_thread_init(NULL);
-#else
-        fprintf(stderr, "glib threading failed to initialize.\n");
-        exit(1);
-#endif
-    }
-
 #if !GLIB_CHECK_VERSION(2, 31, 0)
     trace_available_cond = g_cond_new();
     trace_empty_cond = g_cond_new();
diff --git a/util/osdep.c b/util/osdep.c
index 62072b4..e29ba6f 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -437,6 +437,24 @@ int socket_init(void)
     return 0;
 }
 
+/* Ensure that glib is running in multi-threaded mode */
+static void __attribute__((constructor)) thread_init(void)
+{
+    if (!g_thread_supported()) {
+#if !GLIB_CHECK_VERSION(2, 31, 0)
+        /* Old versions of glib require explicit initialization.  Failure to do
+         * this results in the single-threaded code paths being taken inside
+         * glib.  For example, the g_slice allocator will not be thread-safe
+         * and cause crashes.
+         */
+        g_thread_init(NULL);
+#else
+        fprintf(stderr, "glib threading failed to initialize.\n");
+        exit(1);
+#endif
+    }
+}
+
 #ifndef CONFIG_IOVEC
 /* helper function for iov_send_recv() */
 static ssize_t
diff --git a/vl.c b/vl.c
index 983cdc6..e2dee8e 100644
--- a/vl.c
+++ b/vl.c
@@ -2857,14 +2857,6 @@ int main(int argc, char **argv, char **envp)
     error_set_progname(argv[0]);
 
     g_mem_set_vtable(&mem_trace);
-    if (!g_thread_supported()) {
-#if !GLIB_CHECK_VERSION(2, 31, 0)
-        g_thread_init(NULL);
-#else
-        fprintf(stderr, "glib threading failed to initialize.\n");
-        exit(1);
-#endif
-    }
 
     module_call_init(MODULE_INIT_QOM);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] osdep: initialize glib threads in all QEMU tools

[Kevin Wolf]

Am 08.10.2013 um 11:58 hat Stefan Hajnoczi geschrieben:
> glib versions prior to 2.31.0 require an explicit g_thread_init() call
> to enable multi-threading.
> 
> Failure to initialize threading causes glib to take single-threaded code
> paths without synchronization.  For example, the g_slice allocator will
> crash due to race conditions.
> 
> Fix this for all QEMU tool programs (qemu-nbd, qemu-io, qemu-img) by
> moving the g_thread_init() call from vl.c:main() into a new
> osdep.c:thread_init() constructor function.
> 
> thread_init() has __attribute__((constructor)) and is automatically
> invoked by the runtime during startup.
> 
> We can now drop the "simple" trace backend's g_thread_init() call since
> thread_init() already called it.
> 
> Note that we must keep coroutine-gthread.c's g_thread_init() call which
> is located in a constructor function.  There is no guarantee for
> constructor function ordering so thread_init() may only be called later.

The glib documentation says:

    Since version 2.24, calling g_thread_init() multiple times is
    allowed, but nothing happens except for the first call.

I take that this means previously it wasn't allowed. qemu's configure
checks for a minimum version of 2.12, so we seems to support glib
versions that don't allow g_thread_init() to be called multiple times.

Do we need to protect against this?

Kevin

Re: [Qemu-devel] [PATCH] osdep: initialize glib threads in all QEMU tools

[Paolo Bonzini]

Il 08/10/2013 14:25, Kevin Wolf ha scritto:
> The glib documentation says:
> 
>     Since version 2.24, calling g_thread_init() multiple times is
>     allowed, but nothing happens except for the first call.
> 
> I take that this means previously it wasn't allowed. qemu's configure
> checks for a minimum version of 2.12, so we seems to support glib
> versions that don't allow g_thread_init() to be called multiple times.
> 
> Do we need to protect against this?

I think that's the point of the "if (!g_thread_supported ())" tests.

Paolo

Re: [Qemu-devel] [PATCH] osdep: initialize glib threads in all QEMU tools

[Kevin Wolf]

Am 08.10.2013 um 15:08 hat Paolo Bonzini geschrieben:
> Il 08/10/2013 14:25, Kevin Wolf ha scritto:
> > The glib documentation says:
> > 
> >     Since version 2.24, calling g_thread_init() multiple times is
> >     allowed, but nothing happens except for the first call.
> > 
> > I take that this means previously it wasn't allowed. qemu's configure
> > checks for a minimum version of 2.12, so we seems to support glib
> > versions that don't allow g_thread_init() to be called multiple times.
> > 
> > Do we need to protect against this?
> 
> I think that's the point of the "if (!g_thread_supported ())" tests.

Ah yes, I think you're right. Not the best function name I've ever seen
that glib uses there, but okay.

Kevin

Re: [Qemu-devel] [PATCH] osdep: initialize glib threads in all QEMU tools

[Stefan Hajnoczi]

On Tue, Oct 08, 2013 at 03:58:24PM +0200, Kevin Wolf wrote:
> Am 08.10.2013 um 15:08 hat Paolo Bonzini geschrieben:
> > Il 08/10/2013 14:25, Kevin Wolf ha scritto:
> > > The glib documentation says:
> > > 
> > >     Since version 2.24, calling g_thread_init() multiple times is
> > >     allowed, but nothing happens except for the first call.
> > > 
> > > I take that this means previously it wasn't allowed. qemu's configure
> > > checks for a minimum version of 2.12, so we seems to support glib
> > > versions that don't allow g_thread_init() to be called multiple times.
> > > 
> > > Do we need to protect against this?
> > 
> > I think that's the point of the "if (!g_thread_supported ())" tests.
> 
> Ah yes, I think you're right. Not the best function name I've ever seen
> that glib uses there, but okay.

That's correct, g_thread_support() is there to initialize once only.

Stefan

Re: [Qemu-devel] [PATCH] osdep: initialize glib threads in all QEMU tools

[Stefan Hajnoczi]

On Tue, Oct 08, 2013 at 11:58:31AM +0200, Stefan Hajnoczi wrote:
> glib versions prior to 2.31.0 require an explicit g_thread_init() call
> to enable multi-threading.
> 
> Failure to initialize threading causes glib to take single-threaded code
> paths without synchronization.  For example, the g_slice allocator will
> crash due to race conditions.
> 
> Fix this for all QEMU tool programs (qemu-nbd, qemu-io, qemu-img) by
> moving the g_thread_init() call from vl.c:main() into a new
> osdep.c:thread_init() constructor function.
> 
> thread_init() has __attribute__((constructor)) and is automatically
> invoked by the runtime during startup.
> 
> We can now drop the "simple" trace backend's g_thread_init() call since
> thread_init() already called it.
> 
> Note that we must keep coroutine-gthread.c's g_thread_init() call which
> is located in a constructor function.  There is no guarantee for
> constructor function ordering so thread_init() may only be called later.
> 
> Reported-by: Mario de Chenno <mario.dechenno@unina2.it>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  trace/simple.c |  9 ---------
>  util/osdep.c   | 18 ++++++++++++++++++
>  vl.c           |  8 --------
>  3 files changed, 18 insertions(+), 17 deletions(-)

Applied to my block tree, we need this for QEMU 2.0:
https://github.com/stefanha/qemu/commits/block

Stefan