Re: [libvirt] [PATCH 1/8] remote: fix memory leak

2011/3/26 Eric Blake <eblake(a)redhat.com&gt;: > 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.

> + 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.

Were you actually able to reproduce the leaks and verify that this patch fixes it?

> @@ -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.