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

[Michal Prívozník]

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);
    }


Michal