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

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.

Michal