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

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); }