Date: Sat, 11 Feb 2012 20:35:46 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Martin Cracauer <cracauer@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r231449 - head/usr.bin/tee Message-ID: <20120211194854.J2214@besplex.bde.org> In-Reply-To: <201202102216.q1AMGI0m098192@svn.freebsd.org> References: <201202102216.q1AMGI0m098192@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 10 Feb 2012, Martin Cracauer wrote:
> Log:
> Fix bin/164947: tee looses data when writing to non-blocking file descriptors
>
> tee was not handling EAGAIN
>
> patch submitted by Diomidis Spinellis <dds@aueb.gr>. Thanks so much
>
> reproduced and re-tested locally
This seems to give a buffer overrun (apart from reducing to a write()
spinloop) when the number of output files is enormous and non-blocking
mode is broken.
> Modified: head/usr.bin/tee/tee.c
> ==============================================================================
> --- head/usr.bin/tee/tee.c Fri Feb 10 22:14:34 2012 (r231448)
> +++ head/usr.bin/tee/tee.c Fri Feb 10 22:16:17 2012 (r231449)
> ...
> @@ -106,9 +109,14 @@ main(int argc, char *argv[])
> bp = buf;
> do {
> if ((wval = write(p->fd, bp, n)) == -1) {
> - warn("%s", p->name);
> - exitval = 1;
> - break;
> + if (errno == EAGAIN) {
> + waitfor(p->fd);
p->fd is limited only by the number of args (which is limited by
{ARG_MAX}, which defaults to 256K and is hard to change) and {OPEN_MAX}
(which is about 12000 on all machines I can test quickly, including
2 very different ones; it is easy to reduce but not so easy to increase).
However, this code should be unreachable except for fd == 0, since we
opened all the other fd's for itself and we didn't ask for O_NONBLOCK
on them). However2, I think some device files still have broken
non-blocking mode, where the non-blocking flag is set in the device
state instead of in the open file state and is thus too global.
> + wval = 0;
> + } else {
> + warn("%s", p->name);
> + exitval = 1;
> + break;
> + }
> }
> bp += wval;
> } while (n -= wval);
> @@ -137,3 +145,15 @@ add(int fd, const char *name)
> p->next = head;
> head = p;
> }
> +
> +/* Wait for the specified fd to be ready for writing */
> +static void
> +waitfor(int fd)
> +{
> + fd_set writefds;
The number of bits is this is limited by FD_SETSIZE, which defaults
to 1024. This is much smaller than the other limits, so fd can
easily exceed FD_SETSIZE. FD_SETSIZE is easy to change, but it is
not changed by this program.
> +
> + FD_ZERO(&writefds);
> + FD_SET(fd, &writefds);
This gives a buffer overrun when fd >= FD_SETSIZE. FD_SET() does
no internal range checking, so callers must do it.
> + if (select(fd + 1, NULL, &writefds, NULL, NULL) == -1)
> + err(1, "select");
This might even work if the buffer overrun doesn't trash too much,
since (fd + 1) is not too large for the kernel.
> +}
>
The easiest fix is to return immediately if fd >= FD_SETSIZE or even
if fd > 0. This intentionally reduces to the write() spinloop if
someone makes fd >= FD_SETSIZE, or if someone uses tee on a buggy
device.
An enormous number of output files enlarges chances for other interesting
bugs. It only takes one in the middle to have an i/o error (EIO for
hangup is most likely) to break the i/o for all. Blocking on one while
the others could be displaying output is not best.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120211194854.J2214>