Date: Sun, 10 Jan 2010 14:18:58 -0800 From: Colin Percival <cperciva@freebsd.org> To: Andrey Chernov <ache@nagual.pp.ru>, Colin Percival <cperciva@FreeBSD.ORG>, src-committers@FreeBSD.ORG, svn-src-all@FreeBSD.ORG, svn-src-head@FreeBSD.ORG Subject: Re: svn commit: r201999 - head/lib/libc/stdio Message-ID: <4B4A5252.9070205@freebsd.org> In-Reply-To: <20100110212548.GA47331@nagual.pp.ru> References: <201001101430.o0AEUURS051917@svn.freebsd.org> <20100110212548.GA47331@nagual.pp.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4B4A5252.9070205>