On Thu, Dec 13, 2018 at 11:01:10AM -0700, Todd C. Miller wrote:
> Here's a diff that fakes up a FILE like we do in other parts of
> stdio for unbuffered I/O. This lets us call __srefill() normally.
> If __srefill() returns an error, copy the flags back to the real
> FILE *.
>
> - todd
I like a lot the way it is done. Using a copy of FILE argument with
_bf._size changed to request read size is great.
and as juanfra@ said, it solves mercurial problem (I tested it too).
some comments inlined.
> Index: lib/libc/stdio/fread.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdio/fread.c,v
> retrieving revision 1.15
> diff -u -p -u -r1.15 fread.c
> --- lib/libc/stdio/fread.c 21 Sep 2016 04:38:56 -0000 1.15
> +++ lib/libc/stdio/fread.c 13 Dec 2018 17:24:34 -0000
> @@ -69,18 +69,39 @@ fread(void *buf, size_t size, size_t cou
> total = resid;
> p = buf;
>
> + /*
> + * If we're unbuffered we know that the buffer in fp is empty so
> + * we can read directly into buf. This is much faster than a
> + * series of one byte reads into fp->_nbuf.
> + */
> if ((fp->_flags & __SNBF) != 0) {
> - /*
> - * We know if we're unbuffered that our buffer is empty, so
> - * we can just read directly. This is much faster than the
> - * loop below which will perform a series of one byte reads.
> - */
> - while (resid > 0 && (r = (*fp->_read)(fp->_cookie, p, resid)) >
> 0) {
> - p += r;
> - resid -= r;
> + FILE fake;
> + struct __sfileext fakeext;
> +
> + _FILEEXT_SETUP(&fake, &fakeext);
> + /* copy the important variables */
> + fake._flags = fp->_flags;
> + fake._file = fp->_file;
> + fake._cookie = fp->_cookie;
> + fake._read = fp->_read;
in some cases (but it might be an ill argument passed to fread(): see
"if not already reading, have to be reading and writing"), __srefill()
could call __sflush(), and it will call _write handler (which is
uninitialized data). should we copying others handlers too ?
> +
> + /* set up the buffer */
> + fake._bf._base = buf;
does a test for ill arguments (like buf == NULL) should be done ?
else __srefill() will call __smakebuf(), and it will set:
fp->_bf._base = fp->_p = fp->_nbuf;
using _nbuf which is uninitialized data.
> + fake._bf._size = total;
> + fake._lbfsize = 0; /* not line buffered */
> +
Globally, I think `fake' should be properly initialized by doing a
memcpy() with `fp' data. And next, override values to copte with buffer
size. This way, it will take care of all ill cases, and no uninitialized
data will be used.
> + while (resid > 0) {
> + if (__srefill(&fake)) {
> + /* no more input: return partial result */
> + count = (total - resid) / size;
> + fp->_flags |= (fake._flags & (__SERR|__SEOF));
Initially, I wonder why you copied back only __SERR|__SEOF to _flags
(__srefill could change few others flags), but as _flags is sole
parameter to be copied back it makes sens.
> + break;
> + }
> + fake._p += fake._r;
> + resid -= fake._r;
> }
> FUNLOCKFILE(fp);
> - return ((total - resid) / size);
> + return (count);
> }
>
> while (resid > (r = fp->_r)) {
Thanks.
--
Sebastien Marie
