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... [2] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [3] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... [4] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprogram... 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