Re: [PATCH 2/2] rpcb_clnt.c: fix memory leak in destroy_addr

["Wartens, Herb" <wartens2@xxxxxxxx>]

Hi All,
I wish I had caught this sooner, but it appears that prior to committing the patch I submitted for this memory leak, it was modified and is missing an important part of the fix. Looking at the upstream source code in destroy_addr() we can see:

101 static void
 102 destroy_addr(addr)
 103         struct address_cache *addr;
 104 {
 105         if (addr == NULL)
 106                 return;
 107         if (addr->ac_host != NULL) {
 108                 free(addr->ac_host);
 109                 addr->ac_host = NULL;
 110         }
 111         if (addr->ac_netid != NULL) {
 112                 free(addr->ac_netid);
 113                 addr->ac_netid = NULL;
 114         }
 115         if (addr->ac_uaddr != NULL) {
 116                 free(addr->ac_uaddr);
 117                 addr->ac_uaddr = NULL;
 118         }
 119         if (addr->ac_taddr != NULL) {
 120                 if(addr->ac_taddr->buf != NULL) {
 121                         free(addr->ac_taddr->buf);
 122                         addr->ac_taddr->buf = NULL;
 123                 }
 124                 addr->ac_taddr = NULL;
 125         }
 126         free(addr);
 127         addr = NULL;
 128 }

It seems clear that this code is still not freeing addr->ac_taddr.

In the patch I originally submitted to this mailing list, I made sure to free that pointer (see below):
diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
index 630f9ad..539b521 100644
--- a/src/rpcb_clnt.c
+++ b/src/rpcb_clnt.c
@@ -102,19 +102,31 @@ static void
destroy_addr(addr)
struct address_cache *addr;
{
- if (addr == NULL)
+ if (addr == NULL) {
return;
- if(addr->ac_host != NULL)
+ }
+ if(addr->ac_host != NULL) {
free(addr->ac_host);
- if(addr->ac_netid != NULL)
+ addr->ac_host = NULL;
+ }
+ if(addr->ac_netid != NULL) {
free(addr->ac_netid);
- if(addr->ac_uaddr != NULL)
+ addr->ac_netid = NULL;
+ }
+ if(addr->ac_uaddr != NULL) {
free(addr->ac_uaddr);
+ addr->ac_uaddr = NULL;
+ }
if(addr->ac_taddr != NULL) {
- if(addr->ac_taddr->buf != NULL)
+ if(addr->ac_taddr->buf != NULL) {
free(addr->ac_taddr->buf);
+ addr->ac_taddr->buf = NULL;
+ }
+ free(addr->ac_taddr);
+ addr->ac_taddr = NULL;
}
free(addr);
+ addr = NULL;
}

I did not notice that the patch had been modified when it was committed upstream. I wish I had more carefully verified what had been committed back then. Sorry I neglected to do so. We finally got our hands on the updated rpm with the fixes that went in upstream and are still seeing this memory leak. That is how I noticed the problem and investigated the issue once again.

I believe the piece of the fix that was dropped from the patch should be:
diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
index 68fe69a..d909efc 100644
--- a/src/rpcb_clnt.c
+++ b/src/rpcb_clnt.c
@@ -121,6 +121,7 @@ destroy_addr(addr)
                        free(addr->ac_taddr->buf);
                        addr->ac_taddr->buf = NULL;
                }
+               free(addr->ac_taddr);
                addr->ac_taddr = NULL;
        }
        free(addr);

Would love to get this addressed and pulled into a RHEL version as soon as possible.

-Herb

On 8/1/23, 2:34 AM, "Yongcheng Yang" <yoyang@xxxxxxxxxx <mailto:yoyang@xxxxxxxxxx>> wrote:


Signed-off-by: Herb Wartens <wartens2@xxxxxxxx <mailto:wartens2@xxxxxxxx>>
---
src/rpcb_clnt.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)


diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
index 630f9ad..539b521 100644
--- a/src/rpcb_clnt.c
+++ b/src/rpcb_clnt.c
@@ -102,19 +102,31 @@ static void
destroy_addr(addr)
struct address_cache *addr;
{
- if (addr == NULL)
+ if (addr == NULL) {
return;
- if(addr->ac_host != NULL)
+ }
+ if(addr->ac_host != NULL) {
free(addr->ac_host);
- if(addr->ac_netid != NULL)
+ addr->ac_host = NULL;
+ }
+ if(addr->ac_netid != NULL) {
free(addr->ac_netid);
- if(addr->ac_uaddr != NULL)
+ addr->ac_netid = NULL;
+ }
+ if(addr->ac_uaddr != NULL) {
free(addr->ac_uaddr);
+ addr->ac_uaddr = NULL;
+ }
if(addr->ac_taddr != NULL) {
- if(addr->ac_taddr->buf != NULL)
+ if(addr->ac_taddr->buf != NULL) {
free(addr->ac_taddr->buf);
+ addr->ac_taddr->buf = NULL;
+ }
+ free(addr->ac_taddr);
+ addr->ac_taddr = NULL;
}
free(addr);
+ addr = NULL;
}


/*
-- 
2.31.1