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

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

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

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(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. But there are some `goto error` before virIdentitySetCurrent(NULL) fails originally, e.g., [1] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... [2] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... [3] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... [4] https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... 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?

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(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. >> >> But there are some `goto error` before virIdentitySetCurrent(NULL) fails >> originally, e.g., >> [1] >> https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... >> [2] >> https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... >> [3] >> https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... >> [4] >> https://gitlab.com/libvirt/libvirt/-/blob/master/src/rpc/virnetserverprog... >> 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.