[FFmpeg-devel] [PATCH] AAC Decoder round 4

[Michael Niedermayer]

On Wed, Jul 30, 2008 at 05:10:42PM +0100, Robert Swain wrote:
> 2008/7/30 Michael Niedermayer <michaelni at gmx.at>:
> > On Wed, Jul 30, 2008 at 01:41:54PM +0100, Robert Swain wrote:
[..]
> > [...]
> >> >> +    ics->num_window_groups = 1;
> >> >> +    ics->group_len[0] = 1;
> >> >> +    if (ics->window_sequence == EIGHT_SHORT_SEQUENCE) {
> >> >> +        int i;
> >> >> +        ics->max_sfb = get_bits(gb, 4);
> >> >> +        grouping = get_bits(gb, 7);
> >> >> +        for (i = 0; i < 7; i++) {
> >> >> +            if (grouping & (1<<(6-i))) {
> >> >> +                ics->group_len[ics->num_window_groups-1]++;
> >> >> +            } else {
> >> >> +                ics->num_window_groups++;
> >> >> +                ics->group_len[ics->num_window_groups-1] = 1;
> >> >> +            }
> >> >> +        }
> >> >> +        ics->swb_offset    =    swb_offset_128[ac->m4ac.sampling_index];
> >> >> +        ics->num_swb       =       num_swb_128[ac->m4ac.sampling_index];
> >> >
> >> >> +        if(ics->max_sfb > ics->num_swb) {
> >> >> +            av_log(ac->avccontext, AV_LOG_ERROR,
> >> >> +                "Number of scalefactor bands in group (%d) exceeds limit (%d).\n",
> >> >> +                ics->max_sfb, ics->num_swb);
> >> >> +            return -1;
> >> >> +        }
> >> >
> >> > is it safe to write invalid values in the context and then exit with an
> >> > error? are they gurranteed not to be used or that their use is harmless?
> >>
> >> If this function returns -1 it will fall through to aac_decode_frame
> >> returning -1.
> >
> > yes but i think it can end up being used in future frames without
> > the code above being reexecuted to clean it up.
> > These may be bugs elsewhere but still i dont like security related
> > code that depends on the absence of bugs in large amounts of
> > distant code.
> 
> So how should this be handled? I apologise if this is a stupid
> question. Should the values be assigned to temporary local vars and
> written to the context after validation? 

thats an option, alternative is to zero them in the if(){return -1}

> Should this be done just for
> max_sfb and num_swb or are there others?

does the code contain more that are security relevant?


> 
> > [...]
> >> >> +            }
> >> >> +        }
> >> >> +    }
> >> >> +    return 0;
> >> >> +}
> >> >> +
> >> >> +/**
> >> >> + * Decode scale_factor_data; reference: table 4.47.
> >> >> + *
> >> >
> >> >> + * @param   mix_gain            channel gain (Not used by AAC bitstream.)
> >> >> + * @param   global_gain         first scalefactor value as scalefactors are differentially coded
> >> >> + * @param   band_type           array of the band type used for a window group's scalefactor band
> >> >
> >> >> + * @param   band_type_run_end   array of the last scalefactor band of a band type run for a window group's scalefactor band
> >> >
> >> > this sounds a little confusing
> >>
> >> Someone else (Diego?) said this was confusing but I'm not sure how to
> >> simplify it and keep the same information present. I wanted to clarify
> >> what the indices of the variable were. At the time of complaint I
> >> suggested something like "array of the last scalefactor band of a band
> >> type run with indices [window group][scalefactor band]"
> >
> > well then add an example to calrify what the 2 arrays contain
> 
> I'm not really sure what to suggest other than something like this:

you need to work on the line breaks ...


> 
> Index: aac.c
> ===================================================================
> --- aac.c	(revision 2930)
> +++ aac.c	(working copy)
> @@ -1052,8 +1052,10 @@
>  /**
>   * Decode section_data payload; reference: table 4.46.
>   *
> - * @param   band_type           array of the band type used for a
> window group's scalefactor band
> - * @param   band_type_run_end   array of the last scalefactor band of
> a band type run for a window group's scalefactor band
> + * @param   band_type           array of the used band type
> + * @param   band_type_run_end   array of the last scalefactor band of
> a band type run
> + *

honestly, what are you doing? :)
disable the darn line wraping of your editor/mua or attach the patch please
i would review it but i honestly cannot read this


[...]
> 
> >> >> +    int i;
> >> >> +    int n = 1;
> >> >> +    int num_excl_chan = 7;
> >> >> +
> >> >> +    for (i = 0; i < 7; i++)
> >> >> +         ac->che_drc.exclude_mask[i] = get_bits1(gb);
> >> >> +
> >> >> +    while (n <= MAX_CHANNELS && num_excl_chan < MAX_CHANNELS - 7 && get_bits1(gb)) {
> >> >> +        ac->che_drc.additional_excluded_chns[n-1]=1;
> >> >> +        for (i = num_excl_chan; i < num_excl_chan+7; i++)
> >> >> +            ac->che_drc.exclude_mask[i] = get_bits1(gb);
> >> >> +        n++;
> >> >> +        num_excl_chan += 7;
> >> >> +    }
> >> >
> >> > the for and while can maybe be merged
> >>
> >> How about this?
> >
> > lin
> > e b
> > rea
> > ks
> 
> :) Where? Does this help?:

no, see yourself:

[...]
> +         (((i+1)%7) || (ac->che_drc.additional_excluded_chns[n++] =
> get_bits1(gb)));
> +         i++)


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20080730/6d79edc2/attachment.pgp>