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.

diff to v2:
- virNetServerProgramDispatchCall, only free the memory in successful path and error
label.

Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com>
---
 src/rpc/virnetserverclient.c  |  2 ++
 src/rpc/virnetserverprogram.c | 22 ++++++++++------------
 2 files changed, 12 insertions(+), 12 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..94660f867a 100644
--- a/src/rpc/virnetserverprogram.c
+++ b/src/rpc/virnetserverprogram.c
@@ -368,7 +368,7 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
     g_autofree char *arg = NULL;
     g_autofree char *ret = NULL;
     int rv = -1;
-    virNetServerProgramProc *dispatcher;
+    virNetServerProgramProc *dispatcher = NULL;
     virNetMessageError rerr;
     size_t i;
     g_autoptr(virIdentity) identity = NULL;
@@ -446,8 +446,6 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
         msg->nfds = 0;
     }
 
-    xdr_free(dispatcher->arg_filter, arg);
-
     if (rv < 0)
         goto error;
 
@@ -460,28 +458,28 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
     /*msg->header.serial = msg->header.serial;*/
     msg->header.status = VIR_NET_OK;
 
-    if (virNetMessageEncodeHeader(msg) < 0) {
-        xdr_free(dispatcher->ret_filter, ret);
+    if (virNetMessageEncodeHeader(msg) < 0)
         goto error;
-    }
 
     if (msg->nfds &&
-        virNetMessageEncodeNumFDs(msg) < 0) {
-        xdr_free(dispatcher->ret_filter, ret);
+        virNetMessageEncodeNumFDs(msg) < 0)
         goto error;
-    }
 
-    if (virNetMessageEncodePayload(msg, dispatcher->ret_filter, ret) < 0) {
-        xdr_free(dispatcher->ret_filter, ret);
+    if (virNetMessageEncodePayload(msg, dispatcher->ret_filter, ret) < 0)
         goto error;
-    }
 
+    xdr_free(dispatcher->arg_filter, arg);
     xdr_free(dispatcher->ret_filter, ret);
 
     /* Put reply on end of tx queue to send out  */
     return virNetServerClientSendMessage(client, msg);
 
  error:
+    if (dispatcher) {
+        xdr_free(dispatcher->arg_filter, arg);
+        xdr_free(dispatcher->ret_filter, ret);
+    }
+    
     /* Bad stuff (de-)serializing message, but we have an
      * RPC error message we can send back to the client */
     rv = virNetServerProgramSendReplyError(prog, client, msg, &rerr, &msg->header);
-- 
2.33.0

On 9/30/22 09:02, 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.
> 
> diff to v2:
> - virNetServerProgramDispatchCall, only free the memory in successful path and error
> label.

This ^^^^

> 
> Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com>
> ---

usually goes here.

>  src/rpc/virnetserverclient.c  |  2 ++
>  src/rpc/virnetserverprogram.c | 22 ++++++++++------------
>  2 files changed, 12 insertions(+), 12 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..94660f867a 100644
> --- a/src/rpc/virnetserverprogram.c
> +++ b/src/rpc/virnetserverprogram.c
> @@ -368,7 +368,7 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
>      g_autofree char *arg = NULL;
>      g_autofree char *ret = NULL;
>      int rv = -1;
> -    virNetServerProgramProc *dispatcher;
> +    virNetServerProgramProc *dispatcher = NULL;
>      virNetMessageError rerr;
>      size_t i;
>      g_autoptr(virIdentity) identity = NULL;
> @@ -446,8 +446,6 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
>          msg->nfds = 0;
>      }
>  
> -    xdr_free(dispatcher->arg_filter, arg);
> -
>      if (rv < 0)
>          goto error;
>  
> @@ -460,28 +458,28 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog,
>      /*msg->header.serial = msg->header.serial;*/
>      msg->header.status = VIR_NET_OK;
>  
> -    if (virNetMessageEncodeHeader(msg) < 0) {
> -        xdr_free(dispatcher->ret_filter, ret);
> +    if (virNetMessageEncodeHeader(msg) < 0)
>          goto error;
> -    }
>  
>      if (msg->nfds &&
> -        virNetMessageEncodeNumFDs(msg) < 0) {
> -        xdr_free(dispatcher->ret_filter, ret);
> +        virNetMessageEncodeNumFDs(msg) < 0)
>          goto error;
> -    }
>  
> -    if (virNetMessageEncodePayload(msg, dispatcher->ret_filter, ret) < 0) {
> -        xdr_free(dispatcher->ret_filter, ret);
> +    if (virNetMessageEncodePayload(msg, dispatcher->ret_filter, ret) < 0)
>          goto error;
> -    }
>  
> +    xdr_free(dispatcher->arg_filter, arg);
>      xdr_free(dispatcher->ret_filter, ret);
>  
>      /* Put reply on end of tx queue to send out  */
>      return virNetServerClientSendMessage(client, msg);
>  
>   error:
> +    if (dispatcher) {
> +        xdr_free(dispatcher->arg_filter, arg);
> +        xdr_free(dispatcher->ret_filter, ret);
> +    }
> +    

This adds needless whitespace.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

and pushed.

Michal