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

Michael Niedermayer michaelni
Sun Mar 25 16:25:28 CEST 2007


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

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


> 
> > * 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!


> it works for me (tm), 

it worked before your change too


> 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


> 
> > 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
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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20070325/93a2aef9/attachment.pgp>



More information about the ffmpeg-cvslog mailing list