[Ffmpeg-cvslog] r8498 - trunk/libavcodec/lzw.c

[Baptiste Coudurier]

Michael Niedermayer wrote:
> Hi
> 
> On Sun, Mar 25, 2007 at 01:09:51PM +0200, Baptiste Coudurier wrote:
>> Michael Niedermayer wrote:
>>> Hi
>>>
>>> On Sun, Mar 25, 2007 at 12:23:05AM +0100, bcoudurier wrote:
>>>> Author: bcoudurier
>>>> Date: Sun Mar 25 00:23:05 2007
>>>> New Revision: 8498
>>>>
>>>> Modified:
>>>>    trunk/libavcodec/lzw.c
>>>>
>>>> Log:
>>>> break if eob is reached to avoid reading one too much byte
>>>>
>>>> Modified: trunk/libavcodec/lzw.c
>>>> ==============================================================================
>>>> --- trunk/libavcodec/lzw.c	(original)
>>>> +++ trunk/libavcodec/lzw.c	Sun Mar 25 00:23:05 2007
>>>> @@ -77,6 +77,7 @@ static int lzw_get_code(struct LZWState 
>>>>                  s->bs = sizbuf;
>>>>                  if(!sizbuf) {
>>>>                      s->eob_reached = 1;
>>>> +                    break;
>>>>                  }
>>> * this check does not prevent reading over the end
>> at least it stops reading before the end.
> 
> it does not, sizbuf is read from the stream and its value is at the
> mercy of whoever encoded the file

when sizbuf is good and 0, code must break.

> furthermore you broke the code, the return value is uninitalized after
> your change

c is initialized after the while (c = s->bbuf & s->curmask),
so return value IS initialized, what do you mean by uninitialized ?

>>> * this check only catches one eob case in the GIF branch of the if()
>>> * you are not maintainer of LZW
>> nobody is, 
> 
> true which means you should send a patch to ffmpeg-dev or if you dont _and_
> you break it then at least fix it and dont expect others to cleanup after you!

I did not break it, I ensured gif decoding was still working after that
change,
else I would not have commited it.

>> it works for me (tm), 
> 
> it worked before your change too

It was buggy, it must not read one byte too much.

>> and Im maintainer of gif, 
> 
> true
> 
> 
>> that case if
>> specific to gif, so Its like I am maintainer of that part of lzw, which
>> was in gifdec.c before.
>>
>>> * how do you know the code isnt intended to read a byte or 2 too much for
>>>   simplicity and optimization reasons like almost all other code in lavc
>>>   too?
>> Because buf_size is one less than what's actually read, and that's
>> problematic.
> 
> ill remind you of the following in avcodec.h, just to prevent future
> missunderstandings
> 
> /**
>  * Required number of additionally allocated bytes at the end of the input bitstream for decoding.
>  * this is mainly needed because some optimized bitstream readers read
>  * 32 or 64 bit at once and could read over the end<br>
>  * Note, if the first 23 bits of the additional bytes are not 0 then damaged
>  * MPEG bitstreams could cause overread and segfault
>  */
> #define FF_INPUT_BUFFER_PADDING_SIZE 8

that does not excuse the fact that lzw decoder reads one byte too much.
I want to check that last byte of gif is ';' and lzw decoder reads it,
while it should not so I can't check.

>>> considering these, please revert it
>> feel free to do IF you correct the code in a better way.
> 
> the original code was correct, it was just not documented that the lzw
> code could read a little over the end

It was NOT correct, it read one byte too much.

> changing it so it does not read over the end is something which needs to
> be disscussed and of course if we decide to change its behaviour it should
> really not read over the end and not just check 1 out of several cases
> of reading over the end

It's not following gif specs ! I can't check gif EOF.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312