2:53 p.m.
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.
* src/rpc/virnetserverclient.c (virNetServerClientRegisterEvent)
(virNetServerClientGetFD, virNetServerClientIsSecure)
(virNetServerClientLocalAddrString)
(virNetServerClientRemoteAddrString): Check for closed socket.
(virNetServerClientClose): Rearrange close sequence.
Analysis from Wen Congyang.
---
This fixes the problem using the reproduction formula (that is,
pre-patch I reproduced the failure; valgrind showed it at:
==29390== Thread 4:
==29390== Invalid read of size 4
==29390== at 0x3D16608FA0: pthread_mutex_lock (pthread_mutex_lock.c:50)
==29390== by 0x4E9FED2: virMutexLock (threads-pthread.c:85)
==29390== by 0x45DA5E: virNetSocketGetLocalIdentity (virnetsocket.c:741)
==29390== by 0x45554A: virNetServerClientGetLocalIdentity (virnetserverclient.c:401)
==29390== by 0x4420E3: remoteDispatchAuthList (remote.c:1682)
==29390== by 0x4224E4: remoteDispatchAuthListHelper (remote_dispatch.h:19)
==29390== by 0x453EFD: virNetServerProgramDispatchCall (virnetserverprogram.c:375)
==29390== by 0x4539FF: virNetServerProgramDispatch (virnetserverprogram.c:252)
==29390== by 0x456B20: virNetServerHandleJob (virnetserver.c:155)
==29390== by 0x4EA06A3: virThreadPoolWorker (threadpool.c:98)
==29390== by 0x4EA01D6: virThreadHelper (threads-pthread.c:157)
==29390== by 0x3D16606CCA: start_thread (pthread_create.c:301)
==29390== Address 0x10 is not stack'd, malloc'd or (recently) free'd
post-patch, libvirtd stays alive and valgrind no longer reports
an invalid read). But I'd really like danpb's opinion, if there
is time to get that before 0.9.4.
/me note to self 'git send-email --cc=...' uses cc as well as
honoring .git/config --to to the list; but the list manager
sometimes strips cc: lines. Converserly, 'git send-email --to=...'
overrides .git/config settings, leaving out the list. I can't
win; sorry for the private mails on my previous send attempt.
src/rpc/virnetserverclient.c | 29 +++++++++++++++++++----------
1 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 3c0dba8..2f6c040 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -177,7 +177,8 @@ static int virNetServerClientRegisterEvent(virNetServerClientPtr
client)
client->refs++;
VIR_DEBUG("Registering client event callback %d", mode);
- if (virNetSocketAddIOCallback(client->sock,
+ if (!client->sock ||
+ virNetSocketAddIOCallback(client->sock,
mode,
virNetServerClientDispatchEvent,
client,
@@ -386,9 +387,10 @@ int virNetServerClientGetTLSKeySize(virNetServerClientPtr client)
int virNetServerClientGetFD(virNetServerClientPtr client)
{
- int fd = 0;
+ int fd = -1;
virNetServerClientLock(client);
- fd = virNetSocketGetFD(client->sock);
+ if (client->sock)
+ fd = virNetSocketGetFD(client->sock);
virNetServerClientUnlock(client);
return fd;
}
@@ -396,9 +398,10 @@ int virNetServerClientGetFD(virNetServerClientPtr client)
int virNetServerClientGetLocalIdentity(virNetServerClientPtr client,
uid_t *uid, pid_t *pid)
{
- int ret;
+ int ret = -1;
virNetServerClientLock(client);
- ret = virNetSocketGetLocalIdentity(client->sock, uid, pid);
+ if (client->sock)
+ ret = virNetSocketGetLocalIdentity(client->sock, uid, pid);
virNetServerClientUnlock(client);
return ret;
}
@@ -413,7 +416,7 @@ bool virNetServerClientIsSecure(virNetServerClientPtr client)
if (client->sasl)
secure = true;
#endif
- if (virNetSocketIsLocal(client->sock))
+ if (client->sock && virNetSocketIsLocal(client->sock))
secure = true;
virNetServerClientUnlock(client);
return secure;
@@ -502,12 +505,16 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
const char *virNetServerClientLocalAddrString(virNetServerClientPtr client)
{
+ if (!client->sock)
+ return NULL;
return virNetSocketLocalAddrString(client->sock);
}
const char *virNetServerClientRemoteAddrString(virNetServerClientPtr client)
{
+ if (!client->sock)
+ return NULL;
return virNetSocketRemoteAddrString(client->sock);
}
@@ -570,10 +577,7 @@ void virNetServerClientClose(virNetServerClientPtr client)
virNetTLSSessionFree(client->tls);
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)
virNetMessageFree(msg);
}
+ if (client->sock) {
+ virNetSocketFree(client->sock);
+ client->sock = NULL;
+ }
+
virNetServerClientUnlock(client);
}
--
1.7.4.4