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

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(a)huawei.com&gt; >>>> >>>> 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(a)huawei.com&gt; >>>> --- >>>>    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/virnetserverprog... [2] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... [3] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... [4] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... 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?