[libvirt] [PATCH 1/2] daemon: Fix segfault by reloading daemon right after start

[Pavel Hrdina]

On Wed, Feb 18, 2015 at 04:13:15PM +0000, Daniel P. Berrange wrote:
> On Wed, Feb 18, 2015 at 05:08:28PM +0100, Pavel Hrdina wrote:
> > On Wed, Feb 18, 2015 at 03:29:35PM +0000, Daniel P. Berrange wrote:
> > > On Wed, Feb 18, 2015 at 04:24:31PM +0100, Pavel Hrdina wrote:
> > > > Libvirt could crash with segfault if user issue "service reload" right
> > > > after "service start". One possible way to crash libvirt is to run reload
> > > > during initialization of QEMU driver.
> > > > 
> > > > It could happen when qemu driver will initialize qemu_driver_lock but
> > > > don't have a time to set it's "config" and the SIGHUP arrives. The
> > > > reload handler tries to get qemu_drv->config during "virStorageAutostart"
> > > > and dereference it which ends with segfault.
> > > > 
> > > > Let's ignore all reload requests until all drivers are initialized.
> > > > 
> > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1179981
> > > > 
> > > > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > > > ---
> > > >  daemon/libvirtd.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> > > > index 86accaa..835e7dc 100644
> > > > --- a/daemon/libvirtd.c
> > > > +++ b/daemon/libvirtd.c
> > > > @@ -785,6 +785,11 @@ static void daemonReloadHandler(virNetServerPtr srv ATTRIBUTE_UNUSED,
> > > >                                  siginfo_t *sig ATTRIBUTE_UNUSED,
> > > >                                  void *opaque ATTRIBUTE_UNUSED)
> > > >  {
> > > > +    if (!driversInitialized) {
> > > > +        VIR_WARN("Drivers are not initialized, reload ignored");
> > > > +        return;
> > > > +    }
> > > > +
> > > >      VIR_INFO("Reloading configuration on SIGHUP");
> > > >      virHookCall(VIR_HOOK_DRIVER_DAEMON, "-",
> > > >                  VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);
> > > 
> > > Why don't we just not register the reload handler until we get to the
> > > point where it is safe. eg register it only after virStateInitialize
> > > is complete, and unregister it before we call virStateCleanup.
> > 
> > In that case we should register empty handler for SIGHUP otherwise reload could
> > exit libvirtd during driver initialization. But you have a point that my patch
> > still doesn't fix the issue that reload could be triggered during
> > virStateCleanup.
> 
> Oh that's a good point. I forgot about default SIGHUP behaviour.
> 
> > I'll send a v2 where at first we will set empty handler. After
> > virStateInitialize is complete we set the proper reload handler and before
> > virStateCleanup we set again empty handler.
> 
> Maybe your current approach is actually simpler than having to create a
> dummy sighup handler function. We'd just need to reset driversInitialized
> to false again before virSTateCleanup
> 

Agree, that's the missing peace. I'll sent v2.

> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list