From owner-freebsd-arch@FreeBSD.ORG Wed Feb 27 05:01:30 2008 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B9D3F1065672 for ; Wed, 27 Feb 2008 05:01:30 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from speedfactory.net (mail.speedfactory.net [66.23.216.219]) by mx1.freebsd.org (Postfix) with ESMTP id 4DB8013C447 for ; Wed, 27 Feb 2008 05:01:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (unverified [66.23.211.162]) by speedfactory.net (SurgeMail 3.8s) with ESMTP id 233533631-1834499 for multiple; Tue, 26 Feb 2008 23:59:40 -0500 Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.14.2/8.14.2) with ESMTP id m1R51Gq1026417; Wed, 27 Feb 2008 00:01:17 -0500 (EST) (envelope-from jhb@freebsd.org) From: John Baldwin To: Garrett Wollman Date: Tue, 26 Feb 2008 23:55:16 -0500 User-Agent: KMail/1.9.7 References: <200802262251.m1QMp7bV021709@hergotha.csail.mit.edu> In-Reply-To: <200802262251.m1QMp7bV021709@hergotha.csail.mit.edu> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200802262355.16519.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Wed, 27 Feb 2008 00:01:17 -0500 (EST) X-Virus-Scanned: ClamAV 0.91.2/6006/Tue Feb 26 20:03:40 2008 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: arch@freebsd.org Subject: Re: Cleaning up FILE in stdio.. X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Feb 2008 05:01:30 -0000 On Tuesday 26 February 2008 05:51:07 pm Garrett Wollman wrote: > In article <200802261524.30384.jhb@FreeBSD.org> you write: > > >This is assuming that the contents and layout of FILE are not a > >public ABI (i.e. we malloc the things internally and consumers should > >just treat the pointer value as a cookie and not grub around in the > >internals). > > Most interpreted languages grub around in the internals, as > (historically) do a number of macros. Historically Emacs > did so as well (I suppose you can call it an interpreted language). > > >Comments? > > I think you have the right idea but this will break the ABI in a way > that can't be fudged with symbol versioning. Yes, I discovered the macros today while working on my fd as short problem. In actual fact, I bet the software that does this is rare outside of our stdio.h since glibc doesn't expose its FILE struct and doesn't inline operations like fileno(). Axeing __sFILEX should be safe as it doesn't affect any of the members that we access via inline macros and would merely restore one member where 'extra' currently lives and add the locking stuff to the end. However, I can't fix the fact that our stdio can't handle fd's > SHRT_MAX (again, glibc handles this just fine) w/o making a royal mess. We could create a new versioned FILE struct (so long as we can recognize the existing FILE struct somehow) and have new fopen()/fdopen()/freopen() symbols that return the new struct but then all the stdio routines would have to check to see if the structure was an old structure explicitly and handle it appropriately if so. Rather gross. What I've gone with instead to fix the SHRT_MAX problem is to change fopen/fdopen/freopen to fail to use fd's > SHRT_MAX with an error. This fixes the problem we are seeing at work where once an app has 32k open file descriptors, calls to gethostbyname(3) leak a file descriptor of /etc/hosts because the 'files' impl of gethostbyname(3) fopen()s /etc/hosts but the subsequent fclose() (as well as the fread()s to parse it) fail because the fd is sign-extended when the short value is converted to an int to be passed to read(2) and close(2). With this change fopen() now fails instead of fread() and fclose() and it no longer leaks file descriptors. --- //depot/vendor/freebsd_6/src/lib/libc/stdio/fdopen.c 2004/10/22 16:37:59 +++ //depot/yahoo/ybsd_6/src/lib/libc/stdio/fdopen.c 2008/02/26 20:27:21 @@ -61,6 +61,18 @@ if (nofile == 0) nofile = getdtablesize(); + /* + * File descriptors are a full int, but _file is only a short. + * If we get a valid file descriptor that is greater than + * SHRT_MAX, then the fd will get sign-extended into an + * invalid file descriptor. Handle this case by failing the + * open. + */ + if (fd > SHRT_MAX) { + errno = EINVAL; + return (NULL); + } + if ((flags = __sflags(mode, &oflags)) == 0) return (NULL); --- //depot/vendor/freebsd_6/src/lib/libc/stdio/fopen.c 2004/10/22 16:37:59 +++ //depot/yahoo/ybsd_6/src/lib/libc/stdio/fopen.c 2008/02/26 20:33:26 @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include "un-namespace.h" @@ -67,6 +68,18 @@ fp->_flags = 0; /* release */ return (NULL); } + /* + * File descriptors are a full int, but _file is only a short. + * If we get a valid file descriptor that is greater than + * SHRT_MAX, then the fd will get sign-extended into an + * invalid file descriptor. Handle this case by failing the + * open. + */ + if (f > SHRT_MAX) { + _close(f); + errno = ENOMEM; + return (NULL); + } fp->_file = f; fp->_flags = flags; fp->_cookie = fp; --- //depot/vendor/freebsd_6/src/lib/libc/stdio/freopen.c 2006/11/18 14:28:31 +++ //depot/yahoo/ybsd_6/src/lib/libc/stdio/freopen.c 2008/02/26 20:27:21 @@ -207,6 +207,20 @@ } } + /* + * File descriptors are a full int, but _file is only a short. + * If we get a valid file descriptor that is greater than + * SHRT_MAX, then the fd will get sign-extended into an + * invalid file descriptor. Handle this case by failing the + * open. + */ + if (f > SHRT_MAX) { + fp->_flags = 0; /* set it free */ + FUNLOCKFILE(fp); + errno = ENOMEM; + return (NULL); + } + fp->_flags = flags; fp->_file = f; fp->_cookie = fp; -- John Baldwin