bin/44531: Possible memory leak in rpcinfo.c

[markus.mayer%ericsson.com@localhost]

>Number:         44531
>Category:       bin
>Synopsis:       Possible memory leak in rpcinfo.c
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Feb 07 23:45:00 +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@localhost:/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.