From owner-cvs-all  Fri Aug  3 12:38:51 2001
Delivered-To: cvs-all@freebsd.org
Received: from bazooka.unixfreak.org (bazooka.unixfreak.org [63.198.170.138])
	by hub.freebsd.org (Postfix) with ESMTP
	id F210C37B406; Fri,  3 Aug 2001 12:38:42 -0700 (PDT)
	(envelope-from dima@unixfreak.org)
Received: by bazooka.unixfreak.org (Postfix, from userid 1000)
	id 202443E28; Fri,  3 Aug 2001 12:38:27 -0700 (PDT)
Received: from bazooka.unixfreak.org (localhost [127.0.0.1])
	by bazooka.unixfreak.org (Postfix) with ESMTP
	id 0E3C83C12B; Fri,  3 Aug 2001 12:38:27 -0700 (PDT)
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 
In-Reply-To: <200107141818.f6EIINb35715@freefall.freebsd.org>; from iedowse@FreeBSD.org on "Sat, 14 Jul 2001 11:18:23 -0700 (PDT)"
Date: Fri, 03 Aug 2001 12:38:22 -0700
From: Dima Dorfman <dima@unixfreak.org>
Message-Id: <20010803193827.202443E28@bazooka.unixfreak.org>
Sender: owner-cvs-all@FreeBSD.ORG
Precedence: bulk
List-ID: <cvs-all.FreeBSD.ORG>
List-Archive: <http://docs.freebsd.org/mail/> (Web Archive)
List-Help: <mailto:majordomo@FreeBSD.ORG?subject=help> (List Instructions)
List-Subscribe: <mailto:majordomo@FreeBSD.ORG?subject=subscribe%20cvs-all>
List-Unsubscribe: <mailto:majordomo@FreeBSD.ORG?subject=unsubscribe%20cvs-all>
X-Loop: FreeBSD.ORG

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