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

[Peng Liang]

On 10/04/2022 19:43, Michal Prívozník wrote:
> On 10/4/22 12:24, Peng Liang wrote:
>>
>>
>> On 10/03/2022 15:38, Michal Prívozník wrote:
>>> 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.
>>
>> But there are some `goto error` before virIdentitySetCurrent(NULL) fails
>> originally, e.g.,
>> [1]
>> https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram.c#L403
>> [2]
>> https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram.c#L410
>> [3]
>> https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram.c#L413
>> [4]
>> https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram.c#L416
>> Since dispatcher is allocated already, xdr_free(dispatcher->ret_filter,
>> ret) will be called if we goto error from those scenarios after applying
>> Patch v3
>> (https://listman.redhat.com/archives/libvir-list/2022-September/234558.html). Will this cause invalid memory access?
> 
> Ah, xdr_free() does not accept NULL. So we need something like this:
> 
>      if (dispatcher) {
>          if (arg)
>              xdr_free(dispatcher->arg_filter, arg);
>          if (ret)
>              xdr_free(dispatcher->ret_filter, ret);
>      }
> 

If we reach [2],[3],[4], however, then ret is allocated (but is not 
populated). I think there is still a invalid memory access via 
xdr_free(dispatcher->ret_filter, ret). Maybe the following will work?
free_ret:
     xdr_free(dispatcher->arg_filter, ret);
free_arg:
     xdr_free(dispatcher->arg_filter, arg);

then change all gotos after (dispatcher->func)(server, client, msg, 
&rerr, arg, ret) to goto free_ret, and change [2], [3], [4] to goto 
free_arg.

> 
> Michal
>