[PATCH 0/2] Double-Free and Memory Leak Found In libtirpc

[PATCH 0/2] Double-Free and Memory Leak Found In libtirpc

[Yongcheng Yang]

Hey,

These 2 patches are from "Wartens, Herb" <wartens2@llnl.gov> and I
have just tried to make it appliable to the libtirpc upstream version.
Please help double check and apply them if there's no problem.

Herb Wartens (2):
  rpcb_clnt.c: fix a possible double free
  rpcb_clnt.c: fix memory leak in destroy_addr

 src/rpcb_clnt.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH 1/2] rpcb_clnt.c: fix a possible double free

[Yongcheng Yang]

Signed-off-by: Herb Wartens <wartens2@llnl.gov>
---
 src/rpcb_clnt.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
index d178d86..630f9ad 100644
--- a/src/rpcb_clnt.c
+++ b/src/rpcb_clnt.c
@@ -252,12 +252,16 @@ delete_cache(addr)
 	for (cptr = front; cptr != NULL; cptr = cptr->ac_next) {
 		if (!memcmp(cptr->ac_taddr->buf, addr->buf, addr->len)) {
 			/* Unlink from cache. We'll destroy it after releasing the mutex. */
-			if (cptr->ac_uaddr)
+			if (cptr->ac_uaddr) {
 				free(cptr->ac_uaddr);
-			if (prevptr)
+				cptr->ac_uaddr = NULL;
+			}
+			if (prevptr) {
 				prevptr->ac_next = cptr->ac_next;
-			else
+			}
+			else {
 				front = cptr->ac_next;
+			}
 			cachesize--;
 			break;
 		}
-- 
2.31.1

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

[Yongcheng Yang]

Signed-off-by: Herb Wartens <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;
 }
 
 /*
-- 
2.31.1

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

[Wartens, Herb]

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


/*
-- 
2.31.1