Re: [PATCH] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running

On Mon, Apr 25, 2022 at 11:06:06AM +0200, Michal Privoznik wrote: > When a SIGHUP is received a thread is spawned that runs > virStateReload(). However, if SIGINT is received while the former > thread is still running then we may get into problematic > situation: the cleanup code in main() sees drivers initialized > and thus calls virStateCleanup(). So now we have two threads, one > running virStateReload() the other virStateCleanup(). In this > situation it's very likely that a race condition occurs and > either of threads causes SIGSEGV. > > To fix this, unmark drivers as initialized in the > virStateReload() thread for the time the function runs. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075837 > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/remote/remote_daemon.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c > index 26469e0d9f..37d27f93f4 100644 > --- a/src/remote/remote_daemon.c > +++ b/src/remote/remote_daemon.c > @@ -77,7 +77,7 @@ virNetSASLContext *saslCtxt = NULL; > virNetServerProgram *remoteProgram = NULL; > virNetServerProgram *qemuProgram = NULL; > > -volatile bool driversInitialized = false; > +volatile gint driversInitialized = 0; > > static void daemonErrorHandler(void *opaque G_GNUC_UNUSED, >                                virErrorPtr err G_GNUC_UNUSED) > @@ -453,8 +453,13 @@ static void daemonReloadHandlerThread(void > *opaque G_GNUC_UNUSED) >     VIR_INFO("Reloading configuration on SIGHUP"); >     virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", >                 VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL); > -    if (virStateReload() < 0) > + > +    g_atomic_int_set(&driversInitialized, 0); [0] come back here after the comment below [1] Something like the following would be a "workaround": if (g_atomic_int_dec_and_test(&driversInitialized))     return; > +    if (virStateReload() < 0) { >         VIR_WARN("Error while reloading drivers"); But I wonder about this.  Since we could not reload the drivers, we will never be able to reload them again.  I guess we leave it to the user to restart the daemon whenever they find a non-functioning API, right?  I guess that's fine, it's just that before the reload could've been made again and maybe increasing it to 1 again would not be that big of a deal.

> +    } else { > +        g_atomic_int_inc(&driversInitialized); > +    } > } > > static void daemonReloadHandler(virNetDaemon *dmn G_GNUC_UNUSED, > @@ -463,7 +468,7 @@ static void daemonReloadHandler(virNetDaemon *dmn > G_GNUC_UNUSED, > { >     virThread thr; > > -    if (!driversInitialized) { > +    if (g_atomic_int_get(&driversInitialized) == 0) { [1] I would prefer something like g_atomic_int_inc_and_test (which does not exist) here so that there are no two reload handlers running at the same time, essentially eliminating the race window.  Since it does not exist we can do [0]