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

Re: [libvirt] [PATCH] rpc: avoid libvirtd crash on unexpected client close

On 08/02/2011 04:41 AM, Daniel P. Berrange wrote:
On Mon, Aug 01, 2011 at 01:53:19PM -0600, Eric Blake wrote:
Steps to reproduce this problem (vm1 is not running):
for i in `seq 50`; do virsh managedsave vm1&  done; killall virsh

Pre-patch, virNetServerClientClose could end up setting client->sock
to NULL prior to other cleanup functions trying to use client->sock.
This fixes things by checking for NULL in more places, and by deferring
the cleanup until after all queued messages have been served.

@@ -570,10 +577,7 @@ void virNetServerClientClose(virNetServerClientPtr client)
          client->tls = NULL;
-    if (client->sock) {
-        virNetSocketFree(client->sock);
-        client->sock = NULL;
-    }
+    client->wantClose = true;

      while (client->rx) {
          virNetMessagePtr msg
@@ -586,6 +590,11 @@ void virNetServerClientClose(virNetServerClientPtr client)

+    if (client->sock) {
+        virNetSocketFree(client->sock);
+        client->sock = NULL;
+    }

I'm curious why you have these last 2 hunks of the patc ?  The entire
of the virNetServerClientClose() is executed under the mutex, so AFAICT
moving these 4 lines to later in the function can have no effect. The
assignment to wantClose ought to not be needed at this point either,
since this flag is used by the server to decide to invoke the
virNetServerClientClose method.

I did it because virNetMessageFree calls a callback, and I didn't want to audit whether that callback might temporarily drop the mutex or reference client->sock.

That all said, these 2 hunks are at worst harmless, so ACK to the

Fair enough, so I pushed as-is.

Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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