At 08/02/2011 03:53 AM, Eric Blake Write:
> 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.
With this patch, libvirtd does not crash.
>
> * 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.
I agree with this patch. But give the chance to danpb to give the final
ack 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);
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list