1. Patchew
  2. Libvirt
  3. View series

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

Jiang Jiacheng posted 1 patch 1 year, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20220927153821.461930-1-jiangjiacheng@huawei.com
There is a newer version of this series
src/rpc/virnetserverclient.c  |  2 ++
src/rpc/virnetserverprogram.c | 12 +++++++++---
2 files changed, 11 insertions(+), 3 deletions(-)
[PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
Posted by Jiang Jiacheng 1 year, 7 months ago 
From: jiangjiacheng <jiangjiacheng@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@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);
 
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);
         goto error;
+    }
 
-    if (virIdentitySetCurrent(identity) < 0)
+    if (virIdentitySetCurrent(identity) < 0) {
+        xdr_free(dispatcher->arg_filter, arg);
         goto error;
+    }
 
     /*
      * When the RPC handler is called:
@@ -427,8 +431,10 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
      */
     rv = (dispatcher->func)(server, client, msg, &rerr, arg, ret);
 
-    if (virIdentitySetCurrent(NULL) < 0)
+    if (virIdentitySetCurrent(NULL) < 0) {
+        xdr_free(dispatcher->arg_filter, arg);
         goto error;
+    }
 
     /*
      * If rv == 1, this indicates the dispatch func has
-- 
2.27.0
Re: [PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
Posted by Michal Prívozník 1 year, 7 months ago 
On 9/27/22 17:38, Jiang Jiacheng wrote:
> From: jiangjiacheng <jiangjiacheng@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@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);

and in the rest of the hunks too. But at this point, I wonder whether we
shouldn't have just two places where this free is done: one in
successful path and one under error label.

Michal
Re: [PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
Posted by Peng Liang 1 year, 7 months ago 

On 09/29/2022 21:31, Michal Prívozník wrote:
> On 9/27/22 17:38, Jiang Jiacheng wrote:
>> From: jiangjiacheng <jiangjiacheng@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@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).

Thanks,
Peng

> 
> and in the rest of the hunks too. But at this point, I wonder whether we
> shouldn't have just two places where this free is done: one in
> successful path and one under error label.
> 
> Michal
>
Re: [PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
Posted by Michal Prívozník 1 year, 7 months ago 
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@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@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
Re: [PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
Posted by Peng Liang 1 year, 7 months ago 

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@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@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
>
Re: [PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
Posted by Michal Prívozník 1 year, 7 months ago 
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@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@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
Re: [PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
Posted by Peng Liang 1 year, 7 months ago 

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@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@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
>
Re: [PATCH v2] rpc: fix memory leak in virNetServerClientNew and virNetServerProgramDispatchCall
Posted by Michal Prívozník 1 year, 7 months ago 
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@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@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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.