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