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

[Michal Prívozník]

On 10/1/22 05:35, Peng Liang wrote:
> 
> 
> 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).

Oh, you're right. We need to call it only after dispatcher->func() was
called. That is when call to virIdentitySetCurrent(NULL) fails. Because
at that point dispatcher->func() already populated ret.

Michal