2011/3/26 Eric Blake <eblake@xxxxxxxxxx>: > Detected in this valgrind run: > https://bugzilla.redhat.com/show_bug.cgi?id=690734 > ==13864== 10 bytes in 1 blocks are definitely lost in loss record 10 of 34 > ==13864== Â Âat 0x4A05FDE: malloc (vg_replace_malloc.c:236) > ==13864== Â Âby 0x308587FD91: strdup (in /lib64/libc-2.12.so) > ==13864== Â Âby 0x3BB68BA0A3: doRemoteOpen (remote_driver.c:451) > ==13864== Â Âby 0x3BB68BCFCF: remoteOpen (remote_driver.c:1111) > ==13864== Â Âby 0x3BB689A0FF: do_open (libvirt.c:1290) > ==13864== Â Âby 0x3BB689ADD5: virConnectOpenAuth (libvirt.c:1545) > ==13864== Â Âby 0x41F659: main (virsh.c:11613) > > * src/remote/remote_driver.c (doRemoteClose): Move up. > (remoteOpenSecondaryDriver, remoteOpen): Avoid leak on failure. > --- > > Original post: > https://www.redhat.com/archives/libvir-list/2011-March/msg01196.html > > Âsrc/remote/remote_driver.c | Â120 +++++++++++++++++++++++--------------------- > Â1 files changed, 62 insertions(+), 58 deletions(-) > > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index b05bbcb..4c7df17 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c > @@ -998,6 +998,64 @@ retry: > Â Â goto cleanup; > Â} > > +static int > +doRemoteClose (virConnectPtr conn, struct private_data *priv) > +{ > + Â Âif (priv->eventFlushTimer >= 0) { > + Â Â Â Â/* Remove timeout */ > + Â Â Â ÂvirEventRemoveTimeout(priv->eventFlushTimer); > + Â Â Â Â/* Remove handle for remote events */ > + Â Â Â ÂvirEventRemoveHandle(priv->watch); > + Â Â Â Âpriv->watch = -1; > + Â Â} > + > + Â Âif (call (conn, priv, 0, REMOTE_PROC_CLOSE, > + Â Â Â Â Â Â Â(xdrproc_t) xdr_void, (char *) NULL, > + Â Â Â Â Â Â Â(xdrproc_t) xdr_void, (char *) NULL) == -1) > + Â Â Â Âreturn -1; Preexisting problem, but when this remote call fails for some reason we leak the gnutls and sasl sessions, leave FDs open and leak other memory that would have been freed by the rest of the function. > + Â Â/* Close socket. */ > + Â Âif (priv->uses_tls && priv->session) { > + Â Â Â Âgnutls_bye (priv->session, GNUTLS_SHUT_RDWR); > + Â Â Â Âgnutls_deinit (priv->session); > + Â Â} > +#if HAVE_SASL > + Â Âif (priv->saslconn) > + Â Â Â Âsasl_dispose (&priv->saslconn); > +#endif > + Â ÂVIR_FORCE_CLOSE(priv->sock); > + Â ÂVIR_FORCE_CLOSE(priv->errfd); > + > +#ifndef WIN32 > + Â Âif (priv->pid > 0) { > + Â Â Â Âpid_t reap; > + Â Â Â Âdo { > +retry: > + Â Â Â Â Â Âreap = waitpid(priv->pid, NULL, 0); > + Â Â Â Â Â Âif (reap == -1 && errno == EINTR) > + Â Â Â Â Â Â Â Âgoto retry; > + Â Â Â Â} while (reap != -1 && reap != priv->pid); > + Â Â} > +#endif > + Â ÂVIR_FORCE_CLOSE(priv->wakeupReadFD); > + Â ÂVIR_FORCE_CLOSE(priv->wakeupSendFD); > + > + > + Â Â/* Free hostname copy */ > + Â ÂVIR_FREE(priv->hostname); > + > + Â Â/* See comment for remoteType. */ > + Â ÂVIR_FREE(priv->type); > + > + Â Â/* Free callback list */ > + Â ÂvirDomainEventCallbackListFree(priv->callbackList); > + > + Â Â/* Free queued events */ > + Â ÂvirDomainEventQueueFree(priv->domainEvents); > + > + Â Âreturn 0; > +} > + > Âstatic struct private_data * > ÂremoteAllocPrivateData(void) > Â{ > @@ -1039,7 +1097,9 @@ remoteOpenSecondaryDriver(virConnectPtr conn, > > Â Â ret = doRemoteOpen(conn, *priv, auth, rflags); > Â Â if (ret != VIR_DRV_OPEN_SUCCESS) { > + Â Â Â ÂdoRemoteClose(conn, *priv); I'm not sure that this is the right thing to do here. I think we need to make doRemoteOpen cleanup properly on a goto failure, as this is the actual problem here. doRemoteOpen can fail early before setting up the remote connection. If we call doRemoteClose in such a situation the call of the call function with REMOTE_PROC_CLOSE fail and we will leak more stuff. Actually the valgrind log in the bug reports says "error: server closed connection". In that case the call to the call function with REMOTE_PROC_CLOSE will fail and the free functions for priv->hostname, priv->callbackList and priv->domainEvents aren't called later in doRemoteClose. Therefore you patch doesn't help here. On the other hand I currently fail at reproducing the reported leaks. I also can't understand how priv->domainEvents can leak when doRemoteOpen returns != VIR_DRV_OPEN_SUCCESS, because when priv->domainEvents got allocated then doRemoteOpen is guaranteed to return VIR_DRV_OPEN_SUCCESS. That's another reason why I think adding a call to doRemoteClose here is not correct, because the problem seems to be somewhere else. Were you actually able to reproduce the leaks and verify that this patch fixes it? > Â Â Â Â remoteDriverUnlock(*priv); > + Â Â Â ÂvirMutexDestroy(&(*priv)->lock); ACK, to this single addition. > Â Â Â Â VIR_FREE(*priv); > Â Â } else { > Â Â Â Â (*priv)->localUses = 1; > @@ -1111,7 +1171,9 @@ remoteOpen (virConnectPtr conn, > Â Â ret = doRemoteOpen(conn, priv, auth, rflags); > Â Â if (ret != VIR_DRV_OPEN_SUCCESS) { > Â Â Â Â conn->privateData = NULL; > + Â Â Â ÂdoRemoteClose(conn, priv); Here the same remarks apply as in remoteOpenSecondaryDriver. > Â Â Â Â remoteDriverUnlock(priv); > + Â Â Â ÂvirMutexDestroy(&priv->lock); ACK, to this single addition too. > Â Â Â Â VIR_FREE(priv); > Â Â } else { > Â Â Â Â conn->privateData = priv; Matthias -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list