[FFmpeg-devel] [PATCH] Fix non-rounding up to next 16-bit aligned bug in IFF decoder
Wed Apr 28 15:51:26 CEST 2010
Ronald S. Bultje a ?crit :
> 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:
> or (which is what most people do, so I'd prefer this):
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.
> 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!
New patch attached for & ~1 change and added comment.
:-) Basty/CDGS (-:
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 1571 bytes
Desc: not available
More information about the ffmpeg-devel