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

[Michal Prívozník]

On 10/4/22 13:50, Peng Liang wrote:
> 
> 
> 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.


When I try to simulate this problem with the following diff I don't see any invalid reads. Do you?

diff --git i/src/rpc/virnetserverprogram.c w/src/rpc/virnetserverprogram.c
index 212e0d5ab5..3fa88fafda 100644
--- i/src/rpc/virnetserverprogram.c
+++ w/src/rpc/virnetserverprogram.c
@@ -409,6 +409,8 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
     if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0)
         goto error;
 
+    if (rand()%10 == 0) goto error;
+
     if (!(identity = virNetServerClientGetIdentity(client)))
         goto error;
 
@@ -476,8 +478,10 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
 
  error:
     if (dispatcher) {
-        xdr_free(dispatcher->arg_filter, arg);
-        xdr_free(dispatcher->ret_filter, ret);
+        if (arg)
+            xdr_free(dispatcher->arg_filter, arg);
+        if (ret)
+            xdr_free(dispatcher->ret_filter, ret);
     }
 
     /* Bad stuff (de-)serializing message, but we have an


Michal