Hey,
These 2 patches are from "Wartens, Herb" <[email protected]> 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
Signed-off-by: Herb Wartens <[email protected]>
---
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
Signed-off-by: Herb Wartens <[email protected]>
---
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
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" <[email protected] <mailto:[email protected]>> wrote:
Signed-off-by: Herb Wartens <[email protected] <mailto:[email protected]>>
---
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