Re: [PATCH] Rewrite and fix grub_bufio_read()

[Andrei Borzenkov <arvidjaar@gmail.com>]

28.04.2016 22:52, 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.
> 


Ah, OK. Not sure why we need this check in the first place. Buffer size
is unrelated to file size, so this looks more like micro-optimization of
buffer size.

Although this means that we always have the whole file in buffer anyway
and so never hit power of 2 issue.