[FFmpeg-cvslog] r19991 - trunk/libavcodec/vorbis_dec.c

[Reimar Döffinger]

On Wed, Sep 23, 2009 at 02:50:41PM +0200, Michael Niedermayer wrote:
> On Wed, Sep 23, 2009 at 02:22:21PM +0200, Reimar D?ffinger wrote:
> > On Wed, Sep 23, 2009 at 02:17:54PM +0200, michael wrote:
> > > Author: michael
> > > Date: Wed Sep 23 14:17:54 2009
> > > New Revision: 19991
> > > 
> > > Log:
> > > Check masterbook index and subclass book index.
> > > 14_floor_masterbook_index.patch by chrome
> > > 
> > > Modified:
> > >    trunk/libavcodec/vorbis_dec.c
> > > 
> > > Modified: trunk/libavcodec/vorbis_dec.c
> > > ==============================================================================
> > > --- trunk/libavcodec/vorbis_dec.c	Wed Sep 23 14:09:33 2009	(r19990)
> > > +++ trunk/libavcodec/vorbis_dec.c	Wed Sep 23 14:17:54 2009	(r19991)
> > > @@ -487,13 +487,23 @@ static int vorbis_parse_setup_hdr_floors
> > >                  AV_DEBUG(" %d floor %d class dim: %d subclasses %d \n", i, j, floor_setup->data.t1.class_dimensions[j], floor_setup->data.t1.class_subclasses[j]);
> > >  
> > >                  if (floor_setup->data.t1.class_subclasses[j]) {
> > > -                    floor_setup->data.t1.class_masterbook[j]=get_bits(gb, 8);
> > > +                    int bits=get_bits(gb, 8);
> > > +                    if (bits>=vc->codebook_count) {
> > > +                        av_log(vc->avccontext, AV_LOG_ERROR, "Masterbook index %d is out of range.\n", bits);
> > > +                        return 1;
> > > +                    }
> > > +                    floor_setup->data.t1.class_masterbook[j]=bits;
> > >  
> > >                      AV_DEBUG("   masterbook: %d \n", floor_setup->data.t1.class_masterbook[j]);
> > >                  }
> > >  
> > >                  for(k=0;k<(1<<floor_setup->data.t1.class_subclasses[j]);++k) {
> > > -                    floor_setup->data.t1.subclass_books[j][k]=(int16_t)get_bits(gb, 8)-1;
> > > +                    int16_t bits=get_bits(gb, 8)-1;
> > > +                    if (bits!=-1 && bits>=vc->codebook_count) {
> > > +                        av_log(vc->avccontext, AV_LOG_ERROR, "Subclass book index %d is out of range.\n", bits);
> > > +                        return 1;
> > > +                    }
> > > +                    floor_setup->data.t1.subclass_books[j][k]=bits;
> > >  
> > >                      AV_DEBUG("    book %d. : %d \n", k, floor_setup->data.t1.subclass_books[j][k]);
> > >                  }
> > 
> > I feel compelled that these checks, are inconsistent with the other
> > checks due to using that bits intermediate variable (in particular these
> > also couldn't use the macro I proposed because of that)...
> 
> do you think the intermediate is unneeded ?

I think so. The whole thing relies on data.t1.subclass_books... etc not
being used at all after the return, since if codebook_count is 0 the
default value of 0 it already has is outside the range, too!
Or am I missing some effect that intermediate variable might have?
Also as far as I checked, returning for these functions indeed ensures
nothing more is done at all.

> > I also think there is some inconsistency as in whether return 1 or
> > return -1 is use...
> 
> yes, lets see what i can do about that ...

I don't necessarily meant to tell _you_ to do something about it.
I guess I can find some time to help out, too, though I would have to
have an idea what is desired (and then you might actually be faster just
cleaning it up yourself than explaining to me how you want it ;-) ).