Some monitor functions, most notably, monitor_cur() rely on global
data being initialized by 'monitor_init_globals()'. The latter is
called relatively late in startup. If code triggers error_report()
before monitor_init_globals() is called, QEMU will abort when
accessing the uninitialized monitor mutex.

The critical monitor global data must be initialized from a
constructor function, to improve the guarantee that it is done
before any possible calls to monitor_cur(). Not only that, but
the constructor must be marked to run before the default
constructor in case any of them trigger error reporting.

Note in particular that the RCU constructor will spawn a background
thread so we might even have non-constructor QEMU code running
concurrently with other constructors.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor/monitor.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index c5a5d30877..da54e1b1ce 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -704,18 +704,22 @@ void monitor_cleanup(void)
     }
 }
 
-static void monitor_qapi_event_init(void)
+/*
+ * Initialize static vars that have no deps on external
+ * module initialization, and are required for external
+ * functions to call things like monitor_cur()
+ */
+static void __attribute__((__constructor__(QEMU_CONSTRUCTOR_EARLY)))
+monitor_init_static(void)
 {
+    qemu_mutex_init(&monitor_lock);
+    coroutine_mon = g_hash_table_new(NULL, NULL);
     monitor_qapi_event_state = g_hash_table_new(qapi_event_throttle_hash,
                                                 qapi_event_throttle_equal);
 }
 
 void monitor_init_globals(void)
 {
-    monitor_qapi_event_init();
-    qemu_mutex_init(&monitor_lock);
-    coroutine_mon = g_hash_table_new(NULL, NULL);
-
     /*
      * The dispatcher BH must run in the main loop thread, since we
      * have commands assuming that context.  It would be nice to get
-- 
2.50.1

Daniel P. Berrangé <berrange@redhat.com> writes:

> Some monitor functions, most notably, monitor_cur() rely on global
> data being initialized by 'monitor_init_globals()'. The latter is
> called relatively late in startup. If code triggers error_report()
> before monitor_init_globals() is called, QEMU will abort when
> accessing the uninitialized monitor mutex.
>
> The critical monitor global data must be initialized from a
> constructor function, to improve the guarantee that it is done
> before any possible calls to monitor_cur(). Not only that, but
> the constructor must be marked to run before the default
> constructor in case any of them trigger error reporting.

Is error reporting from constructors a good idea?  I feel they're best
used for simple initializations only.

Do we actually do it?

> Note in particular that the RCU constructor will spawn a background
> thread so we might even have non-constructor QEMU code running
> concurrently with other constructors.

Ugh!

Arguably

  Fixes: e69ee454b5f9 (monitor: Make current monitor a per-coroutine property)

I never liked the @coroutine_mon hash table (which is what broke early
monitor_cur()), but accepted it for want of better ideas.

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

On Wed, Sep 17, 2025 at 04:07:06PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Some monitor functions, most notably, monitor_cur() rely on global
> > data being initialized by 'monitor_init_globals()'. The latter is
> > called relatively late in startup. If code triggers error_report()
> > before monitor_init_globals() is called, QEMU will abort when
> > accessing the uninitialized monitor mutex.
> >
> > The critical monitor global data must be initialized from a
> > constructor function, to improve the guarantee that it is done
> > before any possible calls to monitor_cur(). Not only that, but
> > the constructor must be marked to run before the default
> > constructor in case any of them trigger error reporting.
> 
> Is error reporting from constructors a good idea?  I feel they're best
> used for simple initializations only.

When you're down in the weeds on a given piece of code it might
not occurr that it could be used in a constructor.

The biggest usage is QOM type registration, which we've obviously
been careful (lucky) enough to keep safe.

The other common use if initializing global mutexes.

I rather wish our mutex APIs supported a static initializer
like you get with pthreads and/or glib mutexes. That would
have avoided this ordernig problem.

> 
> Do we actually do it?

Probably not, but I can't be that confident as I have not auditted
all constructors.

I accidentally created a problem myself by putting an error_report
call into the rcu constructor to debug something never realized
that would result in pain.

And then I put error_report into the RCU thread itself and thus
discovered that was running concurrently with other constructors.

> > Note in particular that the RCU constructor will spawn a background
> > thread so we might even have non-constructor QEMU code running
> > concurrently with other constructors.
> 
> Ugh!

Indeed, that was my thought when discovernig this :-(

> 
> Arguably
> 
>   Fixes: e69ee454b5f9 (monitor: Make current monitor a per-coroutine property)
> 
> I never liked the @coroutine_mon hash table (which is what broke early
> monitor_cur()), but accepted it for want of better ideas.

I spent a little time wondering if we could replace coroutine_mon with
a "__thread Monitor cur' and then update that in monitor_set_cur, but
I couldn't convince myself it would be entirely safe. So for sake of
getting the series done I took this approach and left the current
monitor stuff for another day.

> 
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 

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

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Sep 17, 2025 at 04:07:06PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > Some monitor functions, most notably, monitor_cur() rely on global
>> > data being initialized by 'monitor_init_globals()'. The latter is
>> > called relatively late in startup. If code triggers error_report()
>> > before monitor_init_globals() is called, QEMU will abort when
>> > accessing the uninitialized monitor mutex.
>> >
>> > The critical monitor global data must be initialized from a
>> > constructor function, to improve the guarantee that it is done
>> > before any possible calls to monitor_cur(). Not only that, but
>> > the constructor must be marked to run before the default
>> > constructor in case any of them trigger error reporting.
>> 
>> Is error reporting from constructors a good idea?  I feel they're best
>> used for simple initializations only.
>
> When you're down in the weeds on a given piece of code it might
> not occurr that it could be used in a constructor.

Fair.  The sane way to avoid that is keeping constructors super-simple.
Ideally, not call anything.  Next best, not call anything but simple
initialization functions from well-known system libraries, and our own
portability wrappers for them.

> The biggest usage is QOM type registration, which we've obviously
> been careful (lucky) enough to keep safe.
>
> The other common use if initializing global mutexes.
>
> I rather wish our mutex APIs supported a static initializer
> like you get with pthreads and/or glib mutexes. That would
> have avoided this ordernig problem.

Oh yes.  So much simpler, easier, and safer than constructors.

>> Do we actually do it?
>
> Probably not, but I can't be that confident as I have not auditted
> all constructors.

More evidence for us abusing constructors.

The constructor audit I'd like to see: dumb them down to super-simple, ...

> I accidentally created a problem myself by putting an error_report
> call into the rcu constructor to debug something never realized
> that would result in pain.

... so nobody will need to put debug prints there.

> And then I put error_report into the RCU thread itself and thus
> discovered that was running concurrently with other constructors.
>
>> > Note in particular that the RCU constructor will spawn a background
>> > thread so we might even have non-constructor QEMU code running
>> > concurrently with other constructors.
>> 
>> Ugh!
>
> Indeed, that was my thought when discovernig this :-(

The spiked pits we set up for ourselves...

>> Arguably
>> 
>>   Fixes: e69ee454b5f9 (monitor: Make current monitor a per-coroutine property)
>> 
>> I never liked the @coroutine_mon hash table (which is what broke early
>> monitor_cur()), but accepted it for want of better ideas.
>
> I spent a little time wondering if we could replace coroutine_mon with
> a "__thread Monitor cur' and then update that in monitor_set_cur, but
> I couldn't convince myself it would be entirely safe. So for sake of
> getting the series done I took this approach and left the current
> monitor stuff for another day.
>
>> 
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

I suggest to record our low opinion on constructor abuse in the commit
message.  As is, it almost sounds as if we considered it normal.
Starting threads there definitely isn't!

Reviewed-by: Markus Armbruster <armbru@redhat.com>