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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux