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

[Eric Blake]

On 03/26/2011 07:31 AM, Matthias Bolte wrote:
> 2011/3/26 Eric Blake <eblake at redhat.com>:
>> 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?

Good catch.  I agree that this needs a v2.  I posted v1 by inspection
only, going off of someone else's valgrind report; I'll try harder to
actually reproduce the failure locally before v2.

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

That might work, too - but the point is that doRemoteOpen should
probably be using doRemoteClose to do that cleanup on failure, so
doRemoteClose still needs to be fixed to get all the contents cleaned up
in all cases.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110326/30e2b28e/attachment-0001.sig>