If you look at the code: size_t fread(void *restrict destv, size_t size, size_t nmemb, FILE *restrict f) { unsigned char *dest = destv; size_t len = size*nmemb, l = len, k; if (!size) nmemb = 0; FLOCK(f); f->mode |= f->mode-1; if (f->rpos != f->rend) { /* First exhaust the buffer. */ k = MIN(f->rend - f->rpos, l); memcpy(dest, f->rpos, k); f->rpos += k; dest += k; l -= k; } /* Read the remainder directly */ for (; l; l-=k, dest+=k) { k = __toread(f) ? 0 : f->read(f, dest, l); if (!k) { FUNLOCK(f); return (len-l)/size; } } FUNLOCK(f); return nmemb; } Consider what happens when f->rpos == f->rend: k is used uninitialized. My suggested fix is: - if (f->rpos != f->rend) { + k = f->rend - f->rpos; + if (!k) { /* First exhaust the buffer. */ - k = MIN(f->rend - f->rpos, l); + k = MIN(k, l);
On Sat, Jul 10, 2021 at 03:10:26PM +0200, jason wrote:
> If you look at the code:
>
> size_t fread(void *restrict destv, size_t size, size_t nmemb, FILE *restrict f)
> {
> unsigned char *dest = destv;
> size_t len = size*nmemb, l = len, k;
> if (!size) nmemb = 0;
>
> FLOCK(f);
>
> f->mode |= f->mode-1;
>
> if (f->rpos != f->rend) {
> /* First exhaust the buffer. */
> k = MIN(f->rend - f->rpos, l);
> memcpy(dest, f->rpos, k);
> f->rpos += k;
> dest += k;
> l -= k;
> }
>
> /* Read the remainder directly */
> for (; l; l-=k, dest+=k) {
> k = __toread(f) ? 0 : f->read(f, dest, l);
> if (!k) {
> FUNLOCK(f);
> return (len-l)/size;
> }
> }
>
> FUNLOCK(f);
> return nmemb;
> }
>
> Consider what happens when f->rpos == f->rend: k is used uninitialized.
At which line?
Rich
On Sat, Jul 10, 2021 at 12:51 PM Rich Felker <dalias@libc.org> wrote: > > On Sat, Jul 10, 2021 at 03:10:26PM +0200, jason wrote: > > If you look at the code: > > > > size_t fread(void *restrict destv, size_t size, size_t nmemb, FILE *restrict f) > > { > > unsigned char *dest = destv; k declared but not initialized. > > size_t len = size*nmemb, l = len, k; > > if (!size) nmemb = 0; > > > > FLOCK(f); > > > > f->mode |= f->mode-1; > > > > if (f->rpos != f->rend) { > > /* First exhaust the buffer. */ k set to value. > > k = MIN(f->rend - f->rpos, l); > > memcpy(dest, f->rpos, k); > > f->rpos += k; > > dest += k; > > l -= k; > > } > > > > /* Read the remainder directly */ USE of k. If f->rpos == f->rend, k was never set before use for the first iteration of the loop. > > for (; l; l-=k, dest+=k) { > > k = __toread(f) ? 0 : f->read(f, dest, l); > > if (!k) { > > FUNLOCK(f); > > return (len-l)/size; > > } > > } > > > > FUNLOCK(f); > > return nmemb; > > } > > > > Consider what happens when f->rpos == f->rend: k is used uninitialized. > > At which line? > > Rich
On Sat, Jul 10, 2021 at 1:13 PM David Edelsohn <dje.gcc@gmail.com> wrote:
>
> On Sat, Jul 10, 2021 at 12:51 PM Rich Felker <dalias@libc.org> wrote:
> >
> > On Sat, Jul 10, 2021 at 03:10:26PM +0200, jason wrote:
> > > If you look at the code:
> > >
> > > size_t fread(void *restrict destv, size_t size, size_t nmemb, FILE *restrict f)
> > > {
> > > unsigned char *dest = destv;
>
> k declared but not initialized.
>
> > > size_t len = size*nmemb, l = len, k;
> > > if (!size) nmemb = 0;
> > >
> > > FLOCK(f);
> > >
> > > f->mode |= f->mode-1;
> > >
> > > if (f->rpos != f->rend) {
> > > /* First exhaust the buffer. */
>
> k set to value.
>
> > > k = MIN(f->rend - f->rpos, l);
> > > memcpy(dest, f->rpos, k);
> > > f->rpos += k;
> > > dest += k;
> > > l -= k;
> > > }
> > >
> > > /* Read the remainder directly */
>
> USE of k. If f->rpos == f->rend, k was never set before use for the
> first iteration of the loop.
>
> > > for (; l; l-=k, dest+=k) {
> > > k = __toread(f) ? 0 : f->read(f, dest, l);
> > > if (!k) {
> > > FUNLOCK(f);
> > > return (len-l)/size;
> > > }
> > > }
> > >
> > > FUNLOCK(f);
> > > return nmemb;
> > > }
> > >
> > > Consider what happens when f->rpos == f->rend: k is used uninitialized.
> >
> > At which line?
Sorry, my mistake, k will be set in the loop before the iteration
expression is evaluated.
- David