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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> > > ---
> > >  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.
> 
> 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.
> 

Actually that would require to handle return value of virNetServerAddSignalHandler
every time we need to re-register the handler for SIGHUP. So I still think that
the first approach is still better.

Pavel

> Thanks for review.
> 
> Pavel
> 
> > 
> > 
> > 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]