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