Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 03 Aug 2001 12:38:22 -0700
From:      Dima Dorfman <dima@unixfreak.org>
To:        Ian Dowse <iedowse@FreeBSD.org>
Cc:        cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org, mb@imp.ch
Subject:   Re: cvs commit: src/lib/libc/rpc rpcb_clnt.c 
Message-ID:  <20010803193827.202443E28@bazooka.unixfreak.org>
In-Reply-To: <200107141818.f6EIINb35715@freefall.freebsd.org>; from iedowse@FreeBSD.org on "Sat, 14 Jul 2001 11:18:23 -0700 (PDT)"
next in thread | previous in thread | raw e-mail | index | archive | help 
Ian Dowse <iedowse@FreeBSD.org> writes:
> iedowse     2001/07/14 11:18:23 PDT
> 
>   Modified files:
>     lib/libc/rpc         rpcb_clnt.c 
>   Log:
>   Fix a memory leak in __rpcb_findaddr(), avoid compiler warnings.

I think the first part is wrong.  Here's the relevant chunk:

	@@ -938,6 +939,8 @@
	        } else if (client) {
	                CLNT_DESTROY(client);
	        }
	+       if (parms.r_addr != NULL && parms.r_addr != nullstring)
	+               free(parms.r_addr);
	        return (address);
	 }
 

params.r_addr is invariably set by getclnthandle() in a call such as:

                client = getclnthandle(host, nconf, &parms.r_addr);

In getclnthandle(), on line 388, we have:

                if (client) {
                        tmpaddr = targaddr ? taddr2uaddr(nconf, &taddr) : NULL;
   --->                 add_cache(host, nconf->nc_netid, &taddr, tmpaddr);
                        if (targaddr)
                                *targaddr = tmpaddr;
                        break;
                }

`targaddr' is the last parameter passed (in this case,
`&params.r_addr').  Notice on the line with the arrow that the value
set is added to the cache.  When we free it in __rpcb_findaddr, we
could be freeing something that's still stored somewhere else, and may
be returned to us later (see line 309).  The simplest way I've found
to tickle this bug is:

	dd@ref5% /usr/libexec/ypxfr -d freebsd -h kerberos ypservers
	ypxfr in free(): error: chunk is already free.
	Abort (core dumped)

(The actual values are irrelevant as long as they're valid.)

The fix, I think, is to remove the part of the patch I quoted above.
Since the value is added to the cache, it's up to whoever manages the
cache to make sure memory isn't leaking.  I'm not familiar enough with
this code to know when and where the cache is purged, but that's
likely where the value should be freed.

Below is the patch (not like it was necessary).  If you have a better
idea, by all means implement it, but please fix the bug somehow.  :-)

Thanks.

Index: rpcb_clnt.c
===================================================================
RCS file: /ref/cvsf/src/lib/libc/rpc/rpcb_clnt.c,v
retrieving revision 1.3
diff -u -r1.3 rpcb_clnt.c
--- rpcb_clnt.c	2001/07/14 18:18:23	1.3
+++ rpcb_clnt.c	2001/08/03 19:15:14
@@ -939,8 +939,6 @@
 	} else if (client) {
 		CLNT_DESTROY(client);
 	}
-	if (parms.r_addr != NULL && parms.r_addr != nullstring)
-		free(parms.r_addr);
 	return (address);
 }
 

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010803193827.202443E28>