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

Peng Liang tcx4c70 at gmail.com
Tue Oct 4 10:24:21 UTC 2022



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?

Thanks,
Peng

> 
> Michal
> 



More information about the libvir-list mailing list