From owner-svn-src-all@FreeBSD.ORG Sat Feb 11 09:35:50 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C20B01065670; Sat, 11 Feb 2012 09:35:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id 4C96C8FC08; Sat, 11 Feb 2012 09:35:49 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q1B9ZkX9010997 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 11 Feb 2012 20:35:47 +1100 Date: Sat, 11 Feb 2012 20:35:46 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Martin Cracauer In-Reply-To: <201202102216.q1AMGI0m098192@svn.freebsd.org> Message-ID: <20120211194854.J2214@besplex.bde.org> References: <201202102216.q1AMGI0m098192@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r231449 - head/usr.bin/tee X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Feb 2012 09:35:51 -0000 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 . 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