[PATCH] bugfix: add lock in ClientClose to avoid data race

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

 



From: l00425170 <liujunjie23@xxxxxxxxxx>

When libvirt client registers callback function,
remoteDispatchConnectDomainEventCallbackRegisterAny is called, then virInsertElementsN
will be called to add new callback for struct daemonClientPrivate *priv.

But at the same time, if the corresponding socket is closed (one way is to kill the
client process), the close callback remoteClientCloseFunc is called, then
remoteFreePrivCallbacks(priv) is called to free priv. So there exists data race to
cause libvirtd coredump.

More details, in add callback, when memset for ptrptr (that is priv) is done in
virExpandN at the process of virInsertElementsN. At this time point, priv can be
free to be 0x0 when close client.

So when the procedure goes to memcpy in virInsertElementsN, the libvirtd will coredump.

And the stack are as follows:
Thread 1 (Thread 0x7feadf7fe700 (LWP 8978)):
    #0  0x00007feb7b77f314 in __memcpy_ssse3_back() from /usr/lib64/libc.so.6
    #1  0x00007feb7e769a82 in memcpy
        (__len=8, __src=0x7feadf7fdb58, __dest=<optimized out>)
        at /usr/include/bits/string3.h:51
    #2  virInsertElementsN
        (ptrptr=ptrptr@entry=0x558e9c36fae8, size=size@entry=8,
        at=<optimized out>, at@entry=18446744073709551615,
        countptr=countptr@entry=0x558e9c36faf0, add=add@entry=1,
        newelems=newelems@entry=0x7feadf7fdb58, clearOriginal=clearOriginal@entry=true,
        inPlace=inPlace@entry=false, report=report@entry=true,
        domcode=domcode@entry=7, filename=filename@entry=0x558e9af16107 "remote.c",
        funcname=funcname@entry=0x558e9af24600 <__FUNCTION__.48097>
        "remoteDispatchConnectDomainEventCallbackRegisterAny", linenr=linenr@entry=4424)
        at util/viralloc.c:454
    #3  0x0000558e9aed7001 in remoteDispatchConnectDomainEventCallbackRegisterAny
        (server=<optimized out>, msg=<optimized out>, ret=0x7feab805e6f0,
        args=0x7feab806aa40, rerr=0x7feadf7fdc90, client=<optimized out>)
        at remote.c:4422
    #4  remoteDispatchConnectDomainEventCallbackRegisterAnyHelper
        (server=<optimized out>, client=<optimized out>, msg=<optimized out>,
        rerr=0x7feadf7fdc90, args=0x7feab806aa40, ret=0x7feab805e6f0)
        at remote_dispatch.h:424
    #5  0x0000558e9af0ca48 in virNetServerProgramDispatchCall
        (msg=0x558e9c3925c0, client=0x558e9c36f970, server=0x558e9c351570,
        prog=0x558e9c367d50)
        at rpc/virnetserverprogram.c:474
    #6  virNetServerProgramDispatch
        (prog=0x558e9c367d50, server=server@entry=0x558e9c351570, client=0x558e9c36f970,
        msg=0x558e9c3925c0)
        at rpc/virnetserverprogram.c:311
    #7  0x0000558e9af085ed in virNetServerProcessMsg
        (msg=<optimized out>, prog=<optimized out>, client=<optimized out>,
        srv=0x558e9c351570)
        at rpc/virnetserver.c:148
    #8  virNetServerHandleJob
        (jobOpaque=<optimized out>, opaque=0x558e9c351570)
        at rpc/virnetserver.c:169
    #9  0x00007feb7e7f1861 in virThreadPoolWorker
        (opaque=opaque@entry=0x558e9c36b5f0)
        at util/virthreadpool.c:167
    #10 0x00007feb7e7f0b78 in virThreadHelper
        (data=<optimized out>)
        at util/virthread.c:219
    #11 0x00007feb7b9fcdc5 in start_thread () from /usr/lib64/libpthread.so.0
    #12 0x00007feb7b72b75d in clone () from /usr/lib64/libc.so.6

Signed-off-by: l00425170 <liujunjie23@xxxxxxxxxx>
---
 src/remote/remote_daemon.h          |  2 ++
 src/remote/remote_daemon_dispatch.c | 46 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/src/remote/remote_daemon.h b/src/remote/remote_daemon.h
index 4467f71..8accafc 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -76,6 +76,8 @@ struct daemonClientPrivate {
     virConnectPtr conn;
 
     daemonClientStreamPtr streams;
+
+    bool clientClosed;
 };
 
 
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index fdb0a36..59e69e6 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1752,8 +1752,12 @@ static void remoteClientCloseFunc(virNetServerClientPtr client)
     daemonRemoveAllClientStreams(priv->streams);
 
     /* Deregister event delivery callback */
-    if (priv->conn)
+    if (priv->conn) {
+        virMutexLock(&priv->lock);
         remoteClientFreePrivateCallbacks(priv);
+        priv->clientClosed = true;
+        virMutexUnlock(&priv->lock);
+    }
 }
 
 
@@ -3889,6 +3893,11 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED
 
     virMutexLock(&priv->lock);
 
+    if (priv->clientClosed) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection want close"));
+        goto cleanup;
+    }
+
     /* If we call register first, we could append a complete callback
      * to our array, but on OOM append failure, we'd have to then hope
      * deregister works to undo our register.  So instead we append an
@@ -4116,6 +4125,11 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
 
     virMutexLock(&priv->lock);
 
+    if (priv->clientClosed) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection want close"));
+        goto cleanup;
+    }
+
     /* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
      * new domain events added after this point should only use the
      * modern callback style of RPC.  */
@@ -4192,6 +4206,11 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI
 
     virMutexLock(&priv->lock);
 
+    if (priv->clientClosed) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection want close"));
+        goto cleanup;
+    }
+
     if (args->dom &&
         !(dom = get_nonnull_domain(priv->conn, *args->dom)))
         goto cleanup;
@@ -5702,6 +5721,11 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN
 
     virMutexLock(&priv->lock);
 
+    if (priv->clientClosed) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection want close"));
+        goto cleanup;
+    }
+
     if (args->net &&
         !(net = get_nonnull_network(priv->conn, *args->net)))
         goto cleanup;
@@ -5824,6 +5848,11 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server ATTRIBUT
 
     virMutexLock(&priv->lock);
 
+    if (priv->clientClosed) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection want close"));
+        goto cleanup;
+    }
+
     if (args->pool &&
         !(pool = get_nonnull_storage_pool(priv->conn, *args->pool)))
         goto cleanup;
@@ -5945,6 +5974,11 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE
 
     virMutexLock(&priv->lock);
 
+    if (priv->clientClosed) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection want close"));
+        goto cleanup;
+    }
+
     if (args->dev &&
         !(dev = get_nonnull_node_device(priv->conn, *args->dev)))
         goto cleanup;
@@ -6066,6 +6100,11 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
 
     virMutexLock(&priv->lock);
 
+    if (priv->clientClosed) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection want close"));
+        goto cleanup;
+    }
+
     if (args->secret &&
         !(secret = get_nonnull_secret(priv->conn, *args->secret)))
         goto cleanup;
@@ -6188,6 +6227,11 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_U
 
     virMutexLock(&priv->lock);
 
+    if (priv->clientClosed) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection want close"));
+        goto cleanup;
+    }
+
     if (args->dom &&
         !(dom = get_nonnull_domain(priv->conn, *args->dom)))
         goto cleanup;
-- 
2.13.3.windows.1


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

  Powered by Linux