diff mbox series

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

Message ID 20230801093310.594942-3-yoyang@redhat.com (mailing list archive)
State New, archived
Headers show
Series Double-Free and Memory Leak Found In libtirpc | expand

Commit Message

Yongcheng Yang Aug. 1, 2023, 9:33 a.m. UTC 
Signed-off-by: Herb Wartens <wartens2@llnl.gov>
---
 src/rpcb_clnt.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Comments

Wartens, Herb March 15, 2024, 8:21 p.m. UTC | #1 
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@redhat.com <mailto:yoyang@redhat.com>> wrote:


Signed-off-by: Herb Wartens <wartens2@llnl.gov <mailto:wartens2@llnl.gov>>
---
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;
}


/*
diff mbox series

Patch

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;
 }
 
 /*