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

[Michal Prívozník]

On 9/27/22 17:38, Jiang Jiacheng wrote:
> From: jiangjiacheng <jiangjiacheng at 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 at 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);

and in the rest of the hunks too. But at this point, I wonder whether we
shouldn't have just two places where this free is done: one in
successful path and one under error label.

Michal