1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] add lock priv->lock protection in remoteClientCloseFunc()

LanceLiu posted 1 patch 4 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1575256581-21921-1-git-send-email-liu.lance.89@gmail.com
src/remote/remote_daemon_dispatch.c |  2 ++
src/remote/remote_daemon_stream.c   | 14 +++++++-------
2 files changed, 9 insertions(+), 7 deletions(-)
[libvirt] [PATCH] add lock priv->lock protection in remoteClientCloseFunc()
Posted by LanceLiu 4 years, 4 months ago 
sometimes, virsh console session may closed by virsh console existed session(when virsh console client
 exit) and new virsh console --force session simutaneously. So in one thread(Job worker), it calls
daemonStreamEvent() and referencing stream, and in another thread(main thread), virNetServerClientClose()
was called, and the callback remoteClientCloseFunc() was called, without protect by priv->lock in
remoteClientCloseFunc(), it may lead to daemonStreamEvent reference stream released by remoteClientCloseFunc(),
and also need change virNetServerClientClose() to be virNetServerClientImmediateClose() in daemonStreamEvent(),
or it lead to libvirt daemon deadlock by double lock priv->lock in daemonStreamEvent()
---
 src/remote/remote_daemon_dispatch.c |  2 ++
 src/remote/remote_daemon_stream.c   | 14 +++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index f369ffb..b0982b7 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1724,7 +1724,9 @@ static void remoteClientCloseFunc(virNetServerClientPtr client)
 {
     struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
 
+    virMutexLock(&priv->lock);
     daemonRemoveAllClientStreams(priv->streams);
+    virMutexUnlock(&priv->lock);
 
     remoteClientFreePrivateCallbacks(priv);
 }
diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c
index 73e4d7b..6a84fdf 100644
--- a/src/remote/remote_daemon_stream.c
+++ b/src/remote/remote_daemon_stream.c
@@ -141,7 +141,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
         (events & VIR_STREAM_EVENT_WRITABLE)) {
         if (daemonStreamHandleWrite(client, stream) < 0) {
             daemonRemoveClientStream(client, stream);
-            virNetServerClientClose(client);
+            virNetServerClientImmediateClose(client);
             goto cleanup;
         }
     }
@@ -151,7 +151,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
         events = events & ~(VIR_STREAM_EVENT_READABLE);
         if (daemonStreamHandleRead(client, stream) < 0) {
             daemonRemoveClientStream(client, stream);
-            virNetServerClientClose(client);
+            virNetServerClientImmediateClose(client);
             goto cleanup;
         }
         /* If we detected EOF during read processing,
@@ -176,7 +176,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
             if (daemonStreamHandleFinish(client, stream, msg) < 0) {
                 virNetMessageFree(msg);
                 daemonRemoveClientStream(client, stream);
-                virNetServerClientClose(client);
+                virNetServerClientImmediateClose(client);
                 goto cleanup;
             }
             break;
@@ -186,7 +186,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
             if (daemonStreamHandleAbort(client, stream, msg) < 0) {
                 virNetMessageFree(msg);
                 daemonRemoveClientStream(client, stream);
-                virNetServerClientClose(client);
+                virNetServerClientImmediateClose(client);
                 goto cleanup;
             }
             break;
@@ -205,7 +205,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
         stream->recvEOF = true;
         if (!(msg = virNetMessageNew(false))) {
             daemonRemoveClientStream(client, stream);
-            virNetServerClientClose(client);
+            virNetServerClientImmediateClose(client);
             goto cleanup;
         }
         msg->cb = daemonStreamMessageFinished;
@@ -219,7 +219,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
                                               "", 0) < 0) {
             virNetMessageFree(msg);
             daemonRemoveClientStream(client, stream);
-            virNetServerClientClose(client);
+            virNetServerClientImmediateClose(client);
             goto cleanup;
         }
     }
@@ -262,7 +262,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
         }
         daemonRemoveClientStream(client, stream);
         if (ret < 0)
-            virNetServerClientClose(client);
+        	virNetServerClientImmediateClose(client);
         goto cleanup;
     }
 
-- 
1.8.3.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] add lock priv->lock protection in remoteClientCloseFunc()
Posted by Lance Liu 4 years, 4 months ago 
Hi:
    Need any more informations about my patch?

LanceLiu <liu.lance.89@gmail.com> 于2019年12月2日周一 上午11:16写道：

> sometimes, virsh console session may closed by virsh console existed
> session(when virsh console client
>  exit) and new virsh console --force session simutaneously. So in one
> thread(Job worker), it calls
> daemonStreamEvent() and referencing stream, and in another thread(main
> thread), virNetServerClientClose()
> was called, and the callback remoteClientCloseFunc() was called, without
> protect by priv->lock in
> remoteClientCloseFunc(), it may lead to daemonStreamEvent reference stream
> released by remoteClientCloseFunc(),
> and also need change virNetServerClientClose() to be
> virNetServerClientImmediateClose() in daemonStreamEvent(),
> or it lead to libvirt daemon deadlock by double lock priv->lock in
> daemonStreamEvent()
> ---
>  src/remote/remote_daemon_dispatch.c |  2 ++
>  src/remote/remote_daemon_stream.c   | 14 +++++++-------
>  2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/remote/remote_daemon_dispatch.c
> b/src/remote/remote_daemon_dispatch.c
> index f369ffb..b0982b7 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1724,7 +1724,9 @@ static void
> remoteClientCloseFunc(virNetServerClientPtr client)
>  {
>      struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> +    virMutexLock(&priv->lock);
>      daemonRemoveAllClientStreams(priv->streams);
> +    virMutexUnlock(&priv->lock);
>
>      remoteClientFreePrivateCallbacks(priv);
>  }
> diff --git a/src/remote/remote_daemon_stream.c
> b/src/remote/remote_daemon_stream.c
> index 73e4d7b..6a84fdf 100644
> --- a/src/remote/remote_daemon_stream.c
> +++ b/src/remote/remote_daemon_stream.c
> @@ -141,7 +141,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>          (events & VIR_STREAM_EVENT_WRITABLE)) {
>          if (daemonStreamHandleWrite(client, stream) < 0) {
>              daemonRemoveClientStream(client, stream);
> -            virNetServerClientClose(client);
> +            virNetServerClientImmediateClose(client);
>              goto cleanup;
>          }
>      }
> @@ -151,7 +151,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>          events = events & ~(VIR_STREAM_EVENT_READABLE);
>          if (daemonStreamHandleRead(client, stream) < 0) {
>              daemonRemoveClientStream(client, stream);
> -            virNetServerClientClose(client);
> +            virNetServerClientImmediateClose(client);
>              goto cleanup;
>          }
>          /* If we detected EOF during read processing,
> @@ -176,7 +176,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>              if (daemonStreamHandleFinish(client, stream, msg) < 0) {
>                  virNetMessageFree(msg);
>                  daemonRemoveClientStream(client, stream);
> -                virNetServerClientClose(client);
> +                virNetServerClientImmediateClose(client);
>                  goto cleanup;
>              }
>              break;
> @@ -186,7 +186,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>              if (daemonStreamHandleAbort(client, stream, msg) < 0) {
>                  virNetMessageFree(msg);
>                  daemonRemoveClientStream(client, stream);
> -                virNetServerClientClose(client);
> +                virNetServerClientImmediateClose(client);
>                  goto cleanup;
>              }
>              break;
> @@ -205,7 +205,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>          stream->recvEOF = true;
>          if (!(msg = virNetMessageNew(false))) {
>              daemonRemoveClientStream(client, stream);
> -            virNetServerClientClose(client);
> +            virNetServerClientImmediateClose(client);
>              goto cleanup;
>          }
>          msg->cb = daemonStreamMessageFinished;
> @@ -219,7 +219,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>                                                "", 0) < 0) {
>              virNetMessageFree(msg);
>              daemonRemoveClientStream(client, stream);
> -            virNetServerClientClose(client);
> +            virNetServerClientImmediateClose(client);
>              goto cleanup;
>          }
>      }
> @@ -262,7 +262,7 @@ daemonStreamEvent(virStreamPtr st, int events, void
> *opaque)
>          }
>          daemonRemoveClientStream(client, stream);
>          if (ret < 0)
> -            virNetServerClientClose(client);
> +               virNetServerClientImmediateClose(client);
>          goto cleanup;
>      }
>
> --
> 1.8.3.1
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.