[libvirt] [PATCH] Fix bug libvirt daemon segfault when new force console vm session break down existed console session. When force console vm command arrived, libvirtd will break down existed console session, and in this procedure, it will call daemonStreamEvent() to release resources , so daemonStreamFilter() need to check if stream filter existed when get client object lock and client->privData's lock, if not existed, just return -1

[Lance Liu]

We first produce this bug in rhel7.4's libvir daemon。For easily produce the
bug, the step can be as follows:
1. add sleep(3)  in daemonStreamFilter() pre
virMutexLock(&stream->priv->lock), then build libvirtd bin executable, then
restart libvirtd
2. use virsh console open one vm's console, for this console（the vm's
kernel need console=ttyS0 boot parameter,then just  input from keyboard on
and on
3. use virsh console --force to break the previous console session,  than
you will get libvirt daemon deadlock。

And for the problem client->privData to be released problem, only
virNetServerClientClose() will free client->privData and client, I think
this can be fixed by remove virNetServerClientClose()
from daemonStreamEvent(),
and replace with virNetServerClientImmediateClose(),
so virNetServerProcessClients() will test the session would be closed。



Michal Privoznik <mprivozn at redhat.com> 于2019年11月25日周一 下午5:38写道：

> On 11/25/19 8:34 AM, LanceLiu wrote:
> > ---
> >   src/libvirt_remote.syms           |  1 +
> >   src/remote/remote_daemon_stream.c | 10 +++++++++-
> >   src/rpc/virnetserverclient.c      | 12 ++++++++++++
> >   src/rpc/virnetserverclient.h      |  2 ++
> >   4 files changed, 24 insertions(+), 1 deletion(-)
>
> Please format commit messages following title + message format (look at
> git log how other messages are formatted).
>
> >
> > diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> > index 0493467..c32e234 100644
> > --- a/src/libvirt_remote.syms
> > +++ b/src/libvirt_remote.syms
> > @@ -173,6 +173,7 @@ virNetServerClientPreExecRestart;
> >   virNetServerClientRemoteAddrStringSASL;
> >   virNetServerClientRemoteAddrStringURI;
> >   virNetServerClientRemoveFilter;
> > +virNetServerClientCheckFilterExist;
> >   virNetServerClientSendMessage;
> >   virNetServerClientSetAuthLocked;
> >   virNetServerClientSetAuthPendingLocked;
> > diff --git a/src/remote/remote_daemon_stream.c
> b/src/remote/remote_daemon_stream.c
> > index 82cadb6..de0dca3 100644
> > --- a/src/remote/remote_daemon_stream.c
> > +++ b/src/remote/remote_daemon_stream.c
> > @@ -292,10 +292,18 @@ daemonStreamFilter(virNetServerClientPtr client,
> >   {
> >       daemonClientStream *stream = opaque;
> >       int ret = 0;
> > +    daemonClientPrivatePtr priv = NULL;
> > +    int filter_id = stream->filterID;
> >
> >       virObjectUnlock(client);
> > +    priv = virNetServerClientGetPrivateData(client);
>
> This is not needed.
>
> >       virMutexLock(&stream->priv->lock);
> >       virObjectLock(client);
> > +    if (!virNetServerClientCheckFilterExist(client, filter_id)) {
> > +        VIR_WARN("this daemon stream filter: %d have been deleted!",
> filter_id);
> > +        ret = -1;
> > +        goto cleanup;
> > +    }
> >
> >       if (msg->header.type != VIR_NET_STREAM &&
> >           msg->header.type != VIR_NET_STREAM_HOLE)
> > @@ -317,7 +325,7 @@ daemonStreamFilter(virNetServerClientPtr client,
> >       ret = 1;
> >
> >    cleanup:
> > -    virMutexUnlock(&stream->priv->lock);
> > +    virMutexUnlock(&priv->lock);
>
> This is not needed: stream->priv and priv are the same structure.
>
> >       return ret;
> >   }
> >
>
> Anyway, this still doesn't work. Problem is, that if a stream is
> removed, the private data might be removed too and hence
> virMutexLock(&stream->priv->lock) will do something undefined (besides
> accessing freed memory). In my testing the daemon deadlocks because it's
> trying to lock stream-priv->lock which is locked.
>
> As I said in the other thread - we need to re-evaluate the first commit.
> Do you have a reproducer for the original problem please?
>
> Michal
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191125/b5c3689c/attachment-0001.htm>