NetBSD Problem Report #44531

From www@NetBSD.org  Mon Feb  7 23:41:01 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id F143063B873
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  7 Feb 2011 23:41:00 +0000 (UTC)
Message-Id: <20110207234059.EC61663B842@www.NetBSD.org>
Date: Mon,  7 Feb 2011 23:40:59 +0000 (UTC)
From: markus.mayer@ericsson.com
Reply-To: markus.mayer@ericsson.com
To: gnats-bugs@NetBSD.org
Subject: Possible memory leak in rpcinfo.c
X-Send-Pr-Version: www-1.0

>Number:         44531
>Category:       bin
>Synopsis:       Possible memory leak in rpcinfo.c
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Feb 07 23:45:00 +0000 2011
>Closed-Date:    Wed Feb 09 06:11:32 +0000 2011
>Last-Modified:  Wed Feb 09 21:10:03 +0000 2011
>Originator:     Markus Mayer
>Release:        NetBSD 5.0.2
>Organization:
Ericsson
>Environment:
NetBSD netbsd.localdomain 5.0.2 NetBSD 5.0.2 (VMWARE) #4: Mon Apr  5 16:04:25 PDT 2010  mmayer@netbsd.localdomain:/usr/obj/sys/arch/i386/compile/VMWARE i386

>Description:
This report is about src/usr.bin/rpcinfo/rpcinfo.c.

It would seem that there exists a code path in clnt_rpcbind_create(), where "handle" is allocated, but never freed. This might be intentional, but I couldn't find the reason as to why it would be. It doesn't look like any references to "handle" are retained, so the memory allocated could never be re-used or freed once "handle" goes out of scope at the end of clnt_rpcbind_create().

See comments inserted below around lines 1622 and 1623.

  1596  static CLIENT *
  1597  clnt_rpcbind_create(host, rpcbversnum, targaddr)
  1598          char *host;
  1599          int rpcbversnum;
  1600          struct netbuf **targaddr;
  1601  {
  1602          static char *tlist[3] = {
  1603                  "circuit_n", "circuit_v", "datagram_v"
  1604          };
  1605          int i;
  1606          struct netconfig *nconf;
  1607          CLIENT *clnt = NULL;
  1608          void *handle;
  1609
  1610          rpc_createerr.cf_stat = RPC_SUCCESS;
  1611          for (i = 0; i < 3; i++) {
  1612                  if ((handle = __rpc_setconf(tlist[i])) == NULL)
  1613                          continue;
  1614                  while (clnt == NULL) {
  1615                          if ((nconf = __rpc_getconf(handle)) == NULL) {
  1616                                  if (rpc_createerr.cf_stat == RPC_SUCCESS)
  1617                                      rpc_createerr.cf_stat = RPC_UNKNOWNPROTO;
  1618                                  break;
  1619                          }
  1620                          clnt = getclnthandle(host, nconf, rpcbversnum,
  1621                                          targaddr);
  1622                  }
/* Looks like handle won't be freed if clnt != NULL at this point. */
  1623                  if (clnt)
  1624                          break;
/* If clnt != NULL, we'll never call __rpc_endconf(), which would free handle. */
  1625                  __rpc_endconf(handle);
  1626          }
  1627          return (clnt);
  1628  }

>How-To-Repeat:
The problem would occur at all times when rpcipc is being used, as "if (clnt) break;" seems to be the normal path through this function.
>Fix:
If the reason for the "break" instruction is to terminate the for loop early, the best thing to do would be to reorder a few lines of code.

  1623                  if (clnt)
  1624                          break;
  1625                  __rpc_endconf(handle);

would become

  1623                  __rpc_endconf(handle);
  1624                  if (clnt)
  1625                          break;

This would ensure that all memory allocated for "handle" is freed before executing the "break" instruction to terminate the for loop.

Somebody with more NFS background than myself should have a look to see if this makes sense.

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Wed, 09 Feb 2011 06:11:32 +0000
State-Changed-Why:
good catch, thanks.
Christos just fixed it in HEAD along with some other semi-related problems;
it probably isn't worthwhile getting this pulled up to the release branches,
unless for some reason you need it.


From: bch@methodlogic.net
To: gnats-bugs@NetBSD.org
Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, dholland@NetBSD.org,
	markus.mayer@ericsson.com
Subject: Re: bin/44531 (Possible memory leak in rpcinfo.c)
Date: Wed, 9 Feb 2011 06:20:05 +0000

 On Wed, Feb 09, 2011 at 06:11:33AM +0000, dholland@NetBSD.org wrote:
 > Synopsis: Possible memory leak in rpcinfo.c
 > 
 > State-Changed-From-To: open->closed
 > State-Changed-By: dholland@NetBSD.org
 > State-Changed-When: Wed, 09 Feb 2011 06:11:32 +0000
 > State-Changed-Why:
 > good catch, thanks.
 > Christos just fixed it in HEAD along with some other semi-related problems;
 > it probably isn't worthwhile getting this pulled up to the release branches,
 > unless for some reason you need it.

 rpcinfo.c is failing build at line 646 (comparison between
 signed/unsigned) and 1537 (comparison is always false due to limited
 range of data type).

 > 

 -- 
 Brad Harder
 Method Logic Digital Consulting
 http://methodlogic.net
 http://twitter.com/bcharder

From: David Holland <dholland@netbsd.org>
To: bch@methodlogic.net, gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/44531 (Possible memory leak in rpcinfo.c)
Date: Wed, 9 Feb 2011 06:27:16 +0000

 On Wed, Feb 09, 2011 at 06:20:05AM +0000, bch@methodlogic.net wrote:
  > rpcinfo.c is failing build at line 646 (comparison between
  > signed/unsigned) and 1537 (comparison is always false due to limited
  > range of data type).

 fixed

 -- 
 David A. Holland
 dholland@netbsd.org

From: bch@methodlogic.net
To: gnats-bugs@NetBSD.org
Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, dholland@NetBSD.org,
	markus.mayer@ericsson.com
Subject: Re: bin/44531 (Possible memory leak in rpcinfo.c)
Date: Wed, 9 Feb 2011 06:30:56 +0000

 On Wed, Feb 09, 2011 at 06:20:05AM +0000, bch@methodlogic.net wrote:
 > On Wed, Feb 09, 2011 at 06:11:33AM +0000, dholland@NetBSD.org wrote:
 > > Synopsis: Possible memory leak in rpcinfo.c
 > > 
 > > State-Changed-From-To: open->closed
 > > State-Changed-By: dholland@NetBSD.org
 > > State-Changed-When: Wed, 09 Feb 2011 06:11:32 +0000
 > > State-Changed-Why:
 > > good catch, thanks.
 > > Christos just fixed it in HEAD along with some other semi-related problems;
 > > it probably isn't worthwhile getting this pulled up to the release branches,
 > > unless for some reason you need it.
 > 
 > rpcinfo.c is failing build at line 646 (comparison between
 > signed/unsigned) and 1537 (comparison is always false due to limited
 > range of data type).

 Appears fixed w/ 1.31 rev just checked-in.

 Cheers,

 -bch

 -- 
 Brad Harder
 Method Logic Digital Consulting
 http://methodlogic.net
 http://twitter.com/bcharder

From: Markus Mayer <markus.mayer@ericsson.com>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>
Cc: "dholland@NetBSD.org" <dholland@netbsd.org>,
        "gnats-admin@netbsd.org"
	<gnats-admin@netbsd.org>,
        "netbsd-bugs@netbsd.org" <netbsd-bugs@netbsd.org>
Subject: Re: bin/44531 (Possible memory leak in rpcinfo.c)
Date: Wed, 9 Feb 2011 11:29:29 -0800

 On 02/08/2011 22:11, dholland@NetBSD.org wrote:
 > Synopsis: Possible memory leak in rpcinfo.c
 >
 > State-Changed-From-To: open->closed
 > State-Changed-By: dholland@NetBSD.org
 > State-Changed-When: Wed, 09 Feb 2011 06:11:32 +0000
 > State-Changed-Why:
 > good catch, thanks.
 > Christos just fixed it in HEAD along with some other semi-related problems;
 > it probably isn't worthwhile getting this pulled up to the release branches,
 > unless for some reason you need it.
 Thanks, guys, for the fast response. We are good. No need to push the 
 fix into a release branch on our behalf.

 Regards,
 -Markus

>Unformatted:

Home	PR Database Search