[PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall

[Peng Liang]

On 09/29/2022 21:31, Michal Prívozník wrote:
> On 9/27/22 17:38, Jiang Jiacheng wrote:
>> From: jiangjiacheng <jiangjiacheng at huawei.com>
>>
>> In virNetServerProgramDispatchCall, The arg is passed as a void* and used to point
>> to a certain struct depended on the dispatcher, so I think it's the memory of the
>> struct's member that leaks and this memory shuld be freed by xdr_free.
>>
>> In virNetServerClientNew, client->rx is assigned by invoking virNetServerClientNew,
>> but isn't freed if client->privateData's initialization failed, which leads to a
>> memory leak. Thanks to Liang Peng's suggestion, put virNetMessageFree(client->rx)
>> into virNetServerClientDispose() to release the memory.
>>
>> Signed-off-by: jiangjiacheng <jiangjiacheng at huawei.com>
>> ---
>>   src/rpc/virnetserverclient.c  |  2 ++
>>   src/rpc/virnetserverprogram.c | 12 +++++++++---
>>   2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
>> index a7d2dfa795..30f6af7be5 100644
>> --- a/src/rpc/virnetserverclient.c
>> +++ b/src/rpc/virnetserverclient.c
>> @@ -931,6 +931,8 @@ void virNetServerClientDispose(void *obj)
>>       PROBE(RPC_SERVER_CLIENT_DISPOSE,
>>             "client=%p", client);
>>   
>> +    if (client->rx)
>> +        virNetMessageFree(client->rx);
>>       if (client->privateData)
>>           client->privateDataFreeFunc(client->privateData);
> 
> Yeah, this one is a genuine memleak. IIUC it can be reproduced by:
> 
>    client = virNetServerClientNew(...);
>    virObjectUnref(client);
> 
>>   
>> diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c
>> index 3ddf9f0428..a813e821a3 100644
>> --- a/src/rpc/virnetserverprogram.c
>> +++ b/src/rpc/virnetserverprogram.c
>> @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
>>       if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0)
>>           goto error;
>>   
>> -    if (!(identity = virNetServerClientGetIdentity(client)))
>> +    if (!(identity = virNetServerClientGetIdentity(client))) {
>> +        xdr_free(dispatcher->arg_filter, arg);
> 
> But here I believe we also need to free dispatcher->ret_filter:
> 
>    xdr_free(dispatcher->ret_filter, ret);

Hi Michal,
I'm not sure why we need to free ret here. IIRC, xdr_free is to free 
memory pointed by *ret instead of ret. For example, if ret is pointing 
to a struct which contains a string, like:
struct Test {
     char *str;
}
then I think xdr_free(dispatcher->ret_filter, ret) will free str instead 
of struct Test itself. So I think we will only need to call 
xdr_free(dispatcher->ret_filter, ret) after we call 
(dispatcher->func)(server, client, msg, &rerr, arg, ret).

Thanks,
Peng

> 
> and in the rest of the hunks too. But at this point, I wonder whether we
> shouldn't have just two places where this free is done: one in
> successful path and one under error label.
> 
> Michal
>