Re: [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

25 Nov 2019

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

    

Lance Liu