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@xxxxxxxxxx>
>>>>>>>
>>>>>>> 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@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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
