Re: [PATCH] Rewrite and fix grub_bufio_read()

[Stefan Fritsch]

On Thu, 28 Apr 2016, Andrei Borzenkov wrote:

> 28.04.2016 14:11, Stefan Fritsch пишет:
> > We had problems downloading large (10-100 MB) gziped files via http
> > with grub. After some debugging, I think I have found the reason in
> > grub_bufio_read, which leads to wrong data being passed on. This patch
> > fixes the issues for me.  I did my tests with a somewhat older
> > snapshot, but the bufio.c file is the same.
> > 
> 
> Please send minimal patch that fixes bug in bufio so we can actually see
> where the bug is and apply bug fix for 2.02. It is impossible to deduce
> from your complete rewrite and this patch is too intrusive for 2.02
> irrespectively of considerations below.
> 
> > Cheers,
> > Stefan
> > 
> > The grub_bufio_read() had some issues:
> > 
> > * in the calculation of next_buf, it assumed that bufio->block_size is a
> >   power of 2, which is not always the case (see code in grub_bufio_open()).
> > 
> 
> All current callers set it to power of 2, although you are right that it
> should verify it.

No:

  if ((size < 0) || ((unsigned) size > io->size))
    size = ((io->size > GRUB_BUFIO_MAX_SIZE) ? GRUB_BUFIO_MAX_SIZE :
            io->size);

...

  bufio->block_size = size;

io->size is the file size. So at least for files < 32K (which is the size 
that bufio is called with by the net layer), block_size is not a power of 
2 but the size of the file. Then next_buf typically gets a value from 0 
to 8.

Though that should not cause wrong data and does not explain the problems 
we have seen with large files.

> 
> > * it assumed that the len passed to grub_bufio_read() is not larger
> >   than bufio->block_size. I have no idea if this is always true, but it
> >   seems rather fragile to depend on that.
> > 
> 
> Where do you got this impression from? It reads arbitrary size, there is
> no assumption about len. Otherwise netboot had not worked at all. The
> logic is - rest of previous buffer, direct read until farthest buffer
> boundary, partial buffer read at the end. Low level file is responsible
> for the second step, so there is no loop needed.

You are right, I was misreading the second part of the code with the 
direct read. Now I wonder why the patch helped with our problem. I will do 
some more debugging.