Date: Tue, 26 Feb 2008 23:55:16 -0500 From: John Baldwin <jhb@freebsd.org> To: Garrett Wollman <wollman@hergotha.csail.mit.edu> Cc: arch@freebsd.org Subject: Re: Cleaning up FILE in stdio.. Message-ID: <200802262355.16519.jhb@freebsd.org> In-Reply-To: <200802262251.m1QMp7bV021709@hergotha.csail.mit.edu> References: <200802262251.m1QMp7bV021709@hergotha.csail.mit.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <stdio.h> 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 <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
+#include <unistd.h>
#include <stdio.h>
#include <errno.h>
#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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200802262355.16519.jhb>