[FFmpeg-soc] [soc]: r2578 - mlp/mlpdec.c

[Michael Niedermayer]

On Sat, Jun 28, 2008 at 02:59:51PM +0100, Ramiro Polla wrote:
> Michael Niedermayer wrote:
> > On Fri, Jun 27, 2008 at 10:33:54PM +0100, Ramiro Polla wrote:
> >> Hi,
> >>
> >> Michael Niedermayer wrote:
> >>> On Thu, Jun 26, 2008 at 07:26:20PM +0100, Ramiro Polla wrote:
> >>>> Hello,
> >>>>
> >>>> ramiro wrote:
> >>>>> Author: ramiro
> >>>>> Date: Thu Jun 26 19:33:28 2008
> >>>>> New Revision: 2578
> >>>>>
> >>>>> Log:
> >>>>> More checks to see if there is enough data.
> >>>>>
> >>>>> Modified:
> >>>>>    mlp/mlpdec.c
> >>>>>
> >>>>> Modified: mlp/mlpdec.c
> >>>>> ==============================================================================
> >>>>> --- mlp/mlpdec.c	(original)
> >>>>> +++ mlp/mlpdec.c	Thu Jun 26 19:33:28 2008
> >>>>> @@ -1009,6 +1009,9 @@ static int read_access_unit(AVCodecConte
> >>>>>      for (substr = 0; substr < m->num_substreams; substr++) {
> >>>>>          int extraword_present, checkdata_present, end;
> >>>>>  +        if (bytes_left < 2)
> >>>>> +            return -1;
> >>>>> +
> >>>>>          extraword_present = get_bits1(&gb);
> >>>>>          skip_bits1(&gb);
> >>>>>          checkdata_present = get_bits1(&gb);
> >>>>> @@ -1022,6 +1025,8 @@ static int read_access_unit(AVCodecConte
> >>>>>          bytes_left -= 2;
> >>>>>           if (extraword_present) {
> >>>>> +            if (bytes_left < 2)
> >>>>> +                return -1;
> >>>>>              skip_bits(&gb, 16);
> >>>>>              parity_bits ^= *buf++;
> >>>>>              parity_bits ^= *buf++;
> >>>> This seems like a long and painful road... While we're still reading 
> >>>> headers with easily calculated size, this isn't too hard, and there won't 
> >>>> be much overhead in the checks. But on more complicated headers and 
> >>>> specially the VLC values it's not easy to see if we still have enough 
> >>>> data.
> >>>>
> >>>> Lots of other codecs I've looked at seem to just trust init_get_bits() 
> >>>> with the remaining bytes.
> >>>>
> >>>> What's the best practice here? Should the header checksum && coded 
> >>>> lengths be enough to validate the input size?
> >>> All _writes_ must be checked, the reads are not critical.
> >>> also dont forget FF_INPUT_BUFFER_PADDING_SIZE
> >> Ok. So is it ok to apply these patches?
> >> revert_1 just revert the last commit of extra checks.
> >> revert_2 removes keeping track of bytes left. the value is never used later 
> > 
> > ok
> 
> Ok, I think the code is ready for another review now.

Please post a patch, ill review it ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080629/22ac46e6/attachment.pgp>