Introduced by commit 239322cb, reported by Ruben Kerkhof. --- src/network/bridge_driver.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 518a5ba..4913126 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -55,7 +55,7 @@ int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, # define networkAllocateActualDevice(iface) 0 # define networkNotifyActualDevice(iface) 0 # define networkReleaseActualDevice(iface) 0 -# defing networkGetNetworkAddress(netname, netaddr) (-2) +# define networkGetNetworkAddress(netname, netaddr) (-2) # define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx) 0 # endif -- 1.7.6

于 2011年07月26日 19:51, Daniel P. Berrange 写道: Thanks, pushed. Osier

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

On Mon, Aug 01, 2011 at 01:53:19PM -0600, Eric Blake wrote: All thes changes look correct & desirable. 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. That all said, these 2 hunks are at worst harmless, so ACK to the patch. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|