From owner-svn-src-head@FreeBSD.ORG Sun Jan 10 22:19:17 2010 Return-Path: Delivered-To: svn-src-head@FreeBSD.ORG Received: from mx2.freebsd.org (mx2.freebsd.org [IPv6:2001:4f8:fff6::35]) by hub.freebsd.org (Postfix) with ESMTP id C37081065695 for ; Sun, 10 Jan 2010 22:19:17 +0000 (UTC) (envelope-from cperciva@freebsd.org) Received: from xps.daemonology.net (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx2.freebsd.org (Postfix) with SMTP id 4567214E3A9 for ; Sun, 10 Jan 2010 22:18:59 +0000 (UTC) Received: (qmail 19135 invoked from network); 10 Jan 2010 22:18:58 -0000 Received: from unknown (HELO xps.daemonology.net) (127.0.0.1) by localhost with SMTP; 10 Jan 2010 22:18:58 -0000 Message-ID: <4B4A5252.9070205@freebsd.org> Date: Sun, 10 Jan 2010 14:18:58 -0800 From: Colin Percival User-Agent: Thunderbird 2.0.0.23 (X11/20091215) MIME-Version: 1.0 To: Andrey Chernov , Colin Percival , src-committers@FreeBSD.ORG, svn-src-all@FreeBSD.ORG, svn-src-head@FreeBSD.ORG References: <201001101430.o0AEUURS051917@svn.freebsd.org> <20100110212548.GA47331@nagual.pp.ru> In-Reply-To: <20100110212548.GA47331@nagual.pp.ru> X-Enigmail-Version: 0.96.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Subject: Re: svn commit: r201999 - head/lib/libc/stdio X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 10 Jan 2010 22:19:17 -0000 Andrey Chernov wrote: > On Sun, Jan 10, 2010 at 02:30:30PM +0000, Colin Percival wrote: >> + if (((count | size) > 0xFFFF) && >> + (count > SIZE_MAX / size)) { >> + errno = EINVAL; >> + fp->_flags |= __SERR; >> + return (0); >> + } > > 1) I don't think that this is good place of exact constants like 0xFFFF, > usually we don't use such things in overflow checks (see all other ones). > fread/fwrite are already slow as designed, so optimizing one time argument > check looks strange. On my laptop, this optimization speeds up 1-byte fwrite calls by 27% compared to always doing the division. You can argue that people who care about code performance shouldn't be making millions of 1-byte fwrite calls, but I think it's a simple enough optimization and has enough of an impact to be worthwhile. > 2) fp->_flags |= __SERR; > This flag is for errors in the file stream, not for errors in > the arguments. Please back that line out. Quoting fread(3): The function fread() does not distinguish between end-of-file and error, and callers must use feof(3) and ferror(3) to determine which occurred. This would seem to imply that for any failed request, either feof or ferror should return a non-zero value. Removing this line would break the common while (fread(ptr, reclen, 1, f) == 1) { /* Do stuff here. */ } if (ferror(f)) { /* Die due to read error */ } /* No error? Ok, we must have hit EOF */ idiom. > 3) errno should be EOVERFLOW, see other owerflow checks in the stdio. I picked EINVAL because this is the code used by read(2) and write(2) if they are passed nbytes > INT_MAX. It would seem odd to use one error code for a number of bytes between INT_MAX and SIZE_MAX and then switch to a different error code for > SIZE_MAX bytes. -- Colin Percival Security Officer, FreeBSD | freebsd.org | The power to serve Founder / author, Tarsnap | tarsnap.com | Online backups for the truly paranoid