Subject: Re: kern/32842: SCM_RIGHTS can leak file descriptor resources
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Gary Thorpe <gathorpe79@yahoo.com>
List: netbsd-bugs
Date: 07/31/2007 03:45:02
The following reply was made to PR kern/32842; it has been noted by GNATS.

From: Gary Thorpe <gathorpe79@yahoo.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/32842: SCM_RIGHTS can leak file descriptor resources
Date: Mon, 30 Jul 2007 23:41:43 -0400 (EDT)

 After further testing, it seems my assertion was false: descriptors are
 only leaked when the send of the datagram fails (as originally
 reported). This is because unp_internalize() increments the reference
 counter of the file in the system table when each integer descriptors
 is converted into a struct file but the reference count is not updated
 if the packet could not be buffered for some reason. The original patch
 deals with most errors except at least some cases of ENOBUFS (maybe
 all) with SOCK_DGRAM.
 
 The patch earlier in the bug report with unp_dispose() doesn't cover
 all the paths, but I have included a patch that seems more complete: it
 also fixes a potential problem with SOCK_STREAM also for the same issue
 (if the control mbuf is discarded then the reference count should also
 be updated), but this is untested. Also, the original test program
 doesn't work for me so I made a smaller one that reveals the error in a
 more straightforward way and shows that the system file table never
 fills up after the patch (against CURRENT but seems like patch could be
 applied to 3 and possibly the 4 branch). Note that the leak is genuine:
 file table fills up even when descriptor is closed in sender without
 patch. Testing done on i386 with 3.0 userland; 3.0, CURRENT, and
 CURRENT+patch kernels tested.
 
 Patch to fix resource leak:
 
 Index: uipc_usrreq.c
 ===================================================================
 RCS file: /cvsroot/src/sys/kern/uipc_usrreq.c,v
 retrieving revision 1.97
 diff -u -r1.97 uipc_usrreq.c
 --- uipc_usrreq.c       22 Apr 2007 08:30:00 -0000      1.97
 +++ uipc_usrreq.c       31 Jul 2007 03:31:00 -0000
 @@ -154,6 +154,7 @@
                 control = unp_addsockcred(l, control);
         if (sbappendaddr(&so2->so_rcv, (const struct sockaddr *)sun, m,
             control) == 0) {
 +               unp_dispose(control);
                 m_freem(control);
                 m_freem(m);
                 so2->so_rcv.sb_overflowed++;
 @@ -329,6 +330,7 @@
                                 error = unp_connect(so, nam, l);
                                 if (error) {
                                 die:
 +                                       unp_dispose(control);
                                         m_freem(control);
                                         m_freem(m);
                                         break;
 @@ -368,8 +370,10 @@
                          * Wake up readers.
                          */
                         if (control) {
 -                               if (sbappendcontrol(rcv, m, control) ==
 0)
 +                               if (sbappendcontrol(rcv, m, control) ==
 0) {
 +                                       unp_dispose(control);
                                         m_freem(control);
 +                               }
                         } else
                                 sbappend(rcv, m);
                         snd->sb_mbmax -=
 
 Test program which demonstrates leak with ENOBUFS (affects CURRENT and
 3.0):
 
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 
 #include <fcntl.h>
 #include <unistd.h>
 #include <sys/socket.h>
 
 /* Test sending of UNIX datagrams with file descriptors via SCM_RIGHTS
 */
 
 /*
  * ITERS > kern.maxfiles: tune per system. First run will fill table,
 second
  * will always return ENFILE on opens.
  */
 #define ITERS 2000
 
 int
 main(void)
 {
         int i;
         int fd;
         int sfd[2];
         struct cmsghdr * cmptr;
         struct msghdr m_hdr;
         struct iovec m_iov;
         char msg_buf[1] = { 0 };
 
         /* socket never read */
         if (-1 == socketpair(AF_LOCAL, SOCK_DGRAM, 0, sfd)) {
                 perror("socketpair");
                 exit(1);
         }
 
         /* set up message headers */
         if (NULL == (cmptr = malloc(CMSG_LEN(sizeof(int))))) {
                 perror("malloc");
                 exit(1);
         }
         cmptr->cmsg_len = CMSG_LEN(sizeof(int));
         cmptr->cmsg_level = SOL_SOCKET;
         cmptr->cmsg_type = SCM_RIGHTS;
         m_iov.iov_base = msg_buf;
         m_iov.iov_len = sizeof(msg_buf);
         m_hdr.msg_name = NULL;
         m_hdr.msg_iov = &m_iov;
         m_hdr.msg_iovlen = 1;
         m_hdr.msg_control = cmptr;
         m_hdr.msg_controllen = cmptr->cmsg_len;
         m_hdr.msg_flags = 0;
 
         /* send fd's */
         for (i = 0; i < ITERS; i++) {
                 if (-1 == (fd = open("/dev/null", O_RDONLY, 0))) {
                         perror("open");
                         continue;
                 }
 
                 *(int *)CMSG_DATA(cmptr) = fd;
 
                 if (-1 == sendmsg(sfd[0], &m_hdr, 0)) {
                         /* buffer is expected to fill up */
                         if (ENOBUFS != errno) {
                                 perror("sendmsg");
                                 i = ITERS;
                         }
                 }
 
                 if (-1 == close(fd))
                         perror("close");
         }
 
         /* clean up */
         if (-1 == close(sfd[1]))
                 perror("close");
         if (-1 == close(sfd[0]))
                 perror("close");
         free(cmptr);
 
         exit(0);
 }
 
 
       Ask a question on any topic and get answers from real people. Go to Yahoo! Answers and share what you know at http://ca.answers.yahoo.com