[FFmpeg-devel] [PATCH] Fix non-rounding up to next 16-bit aligned bug in IFF decoder

[Sebastian Vater]

Ronald S. Bultje a ?crit :
> Hi,
>
> On Wed, Apr 28, 2010 at 7:30 AM, Sebastian Vater
> <cdgs.basty at googlemail.com> wrote:
>   
>> I have fixed the wrong IFF decoding issue in the IFF decoder.
>>
>> The reason is that the IFF docs say that each line in the BODY chunk has
>> it's width rounded up to next 16-bit boundary, such that each new line
>> begins on a word boundary (address divisible by 2).
>>     
> [..]
>   
>> +    s->planesize = ((avctx->width + 15) >> 4) * 2;
>>     
>
> This is not how people commonly round up to 2. Most people do
> (x+1)&~1, so you could do:
>
> ((width+15)>>4)<<1
>
> or (which is what most people do, so I'd prefer this):
>
> ((width+15)>>3)&~1
>   

Fixed by applying & ~1.

> But both are fine. And add a comment what this does, e.g. "/* round up
> width to word-boundary */" or so, for the unsuspecting reader.
>   

Fixed.

> The rest of the patch is unrelated to this change and should be
> reviewed separately.
>
>   
>> -    const unsigned b = (buf_size * 8) + bps - 1;
>> +    const uint8_t *end = dst + (buf_size * 8);
>>     
>
> I'm not convinced that is correct. "It doesn't change the output of my
> files" isn't a good enough reason. You're changing the math so it will
> either get worse or better, and for the "better" part it'd need to fix
> a bug or you'd need to show the documentation pointing out that this
> is better.
>   

I can convince you this is correct.

If you look carefully at this code, you will notice that + bps - 1 is
not just unnecessary but also will cause buffer overflow with new width
calculation (remember it rounds up and thus buf_size will get larger)!

Proof with this patch applied:
Let width be 1 and bps be 8.

buf_size will be (1 + 15) >> 4 = 1;

thus end will become = dst + 8;

Therefore dp8 loop will run exactly twice (reading 4 bits each).

Proff w/o this patch applied:
buf_size will be (1 + 15) >> 4;

thus end will become = dst + 8 + 7 (bps - 1) = dst + 15.

Therefore dp8 loop will run 4 times (reading 4 bits each) and thus write
a width of 2 effectively (causing a buffer overflow on last line).

So this will an trigger error which occurs with the new uprounding code!

q.e.d

New patch attached for & ~1 change and added comment.

-- 

Best regards,
                   :-) Basty/CDGS (-:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: iff-decoder-fix.patch
Type: text/x-patch
Size: 1571 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100428/8a527d61/attachment.bin>