[FFmpeg-devel] Neither vorbis_parse_setup_hdr_codebooks nor ff_vorbis_len2vlc verify data

[Michael Niedermayer]

On Wed, Jul 08, 2009 at 10:52:42AM +0200, Reimar D?ffinger wrote:
> On Wed, Jul 08, 2009 at 02:26:50AM +0200, Michael Niedermayer wrote:
> > On Sun, Jul 05, 2009 at 10:21:01AM +0200, Reimar D?ffinger wrote:
> > > On Sun, Jul 05, 2009 at 10:18:02AM +0200, Reimar D?ffinger wrote:
> > > > sample is ogv/smclock.ogv.2.164.ogv from issue 1240.
> > > > vorbis_parse_setup_hdr_codebooks can at least create values up to 33
> > > > (get_bits(gb, 5)+1)
> > 
> > am i missing something? get_bits(gb, 5)+1 should be 32 max not 33
> 
> No, I am silly.
> 
> > > > ff_vorbis_len2vlc which these values are then passed to on the other
> > > > hand just assumes that the bits values are at most 32, otherwise it just
> > > > writes beyond the exit_at_level and onto the stack, overwriting the
> > > > return address.
> > > > Since there is no documentation for the function the question is which
> > > > is wrong? Should ff_vorbis_len2vlc have a check or should
> > > > vorbis_parse_setup_hdr_codebooks?
> > 
> > i would guess that vorbis_parse_setup_hdr_codebooks() is the one that needs
> > a check as one side of the ordered branch can reach higher values, this
> > dosent look too right but its just my gut feeling.
> 
> Well, this one seems to work for the sample at least:
> 
> Index: vorbis_dec.c
> ===================================================================
> --- vorbis_dec.c        (revision 19365)
> +++ vorbis_dec.c        (working copy)
> @@ -292,7 +292,7 @@
>              AV_DEBUG(" ordered, current length: %d \n", current_length);  //FIXME
>  
>              used_entries=entries;
> -            for(;current_entry<used_entries;++current_length) {
> +            for(;current_entry<used_entries && current_length <= 32;++current_length) {
>                  uint_fast16_t i, number;
>  
>                  AV_DEBUG(" number bits: %d ", ilog(entries - current_entry));

ok


> 
> Though I am not 100% confident this is all that is needed, so I'd prefer
> to apply the other one at least in addition.

iam perfectily fine with that though would suggest with comments clarifying
the possible redundancy

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- 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-devel/attachments/20090708/398ccb81/attachment.pgp>