[libvirt] [PATCH 02/14] rpc: Fix memory leaks @virnetserverclient

[John Ferlan]

On 12/21/2017 06:32 AM, Marc Hartmayer wrote:
> On Fri, Dec 15, 2017 at 01:14 PM +0100, John Ferlan <jferlan at redhat.com> wrote:
>> On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
>>> Direct leak of 104 byte(s) in 1 object(s) allocated from:
>>>     #0 0x7f904bfbe12b  (/lib64/liblsan.so.0+0xe12b)
>>>     #1 0x7f904ba0ad67 in virAlloc ../../src/util/viralloc.c:144
>>>     #2 0x7f904bbc11a4 in virNetMessageNew ../../src/rpc/virnetmessage.c:42
>>>     #3 0x7f904bbb8e77 in virNetServerClientNewInternal ../../src/rpc/virnetserverclient.c:392
>>>     #4 0x7f904bbb9921 in virNetServerClientNew ../../src/rpc/virnetserverclient.c:440
>>>     #5 0x402ce5 in testIdentity ../../tests/virnetserverclienttest.c:55
>>>     #6 0x403bed in virTestRun ../../tests/testutils.c:180
>>>     #7 0x402c1e in mymain ../../tests/virnetserverclienttest.c:146
>>>     #8 0x404c80 in virTestMain ../../tests/testutils.c:1119
>>>     #9 0x4030d5 in main ../../tests/virnetserverclienttest.c:152
>>>     #10 0x7f9047f7f889 in __libc_start_main (/lib64/libc.so.6+0x20889)
>>>
>>> Indirect leak of 4 byte(s) in 1 object(s) allocated from:
>>>     #0 0x7f904bfbe12b  (/lib64/liblsan.so.0+0xe12b)
>>>     #1 0x7f904ba0adc7 in virAllocN ../../src/util/viralloc.c:191
>>>     #2 0x7f904bbb8ec7 in virNetServerClientNewInternal ../../src/rpc/virnetserverclient.c:395
>>>     #3 0x7f904bbb9921 in virNetServerClientNew ../../src/rpc/virnetserverclient.c:440
>>>     #4 0x402ce5 in testIdentity ../../tests/virnetserverclienttest.c:55
>>>     #5 0x403bed in virTestRun ../../tests/testutils.c:180
>>>     #6 0x402c1e in mymain ../../tests/virnetserverclienttest.c:146
>>>     #7 0x404c80 in virTestMain ../../tests/testutils.c:1119
>>>     #8 0x4030d5 in main ../../tests/virnetserverclienttest.c:152
>>>     #9 0x7f9047f7f889 in __libc_start_main (/lib64/libc.so.6+0x20889)
>>>
>>> SUMMARY: LeakSanitizer: 108 byte(s) leaked in 2 allocation(s).
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>> ---
>>>  src/rpc/virnetserverclient.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
>>> index 6e086b7b4e2b..b454a3ff6992 100644
>>> --- a/src/rpc/virnetserverclient.c
>>> +++ b/src/rpc/virnetserverclient.c
>>> @@ -948,6 +948,9 @@ void virNetServerClientDispose(void *obj)
>>>      virObjectUnref(client->tlsCtxt);
>>>  #endif
>>>      virObjectUnref(client->sock);
>>> +
>>> +    virNetMessageFree(client->rx);
>>> +    virNetMessageFree(client->tx);
>>
>> Makes me curious why virNetServerClientClose isn't doing the Free for
>> the rx/tx.  Oh, I see, the test program isn't calling it.
>>
>> I think this is the right idea, but the issue is in the test program
>> doing the Unref before calling virNetServerClientClose.
> 
> Yes. Will add an patch for this. Should I remove this patch then?
> 

Yes probably should have been explicit - the R-B below was conditional
on altering this patch to call virNetServerClientClose in the test
program instead of in virNetServerClientDispose.

John
>>
>> If you see how other tests/code handle this, the client usually gets
>> added to the net server [n]clients list (virNetServerAddClient) and
>> later when netserver is disposed the nclients is perused and calls
>> virNetServerClientClose prior to the Unref as well for each client.
>>
>> So if you make the right call in tests/virnetserverclienttest.c, then
>> you have my
>>
>> Reviewed-by: John Ferlan <jferlan at redhat.com>
>>
>> John
>>
>>>  }
>>>
>>>
>>>
>>
> --
> Beste Grüße / Kind regards
>    Marc Hartmayer
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
>