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

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).