[FFmpeg-devel] [PATCH] AAC Decoder round 4
Robert Swain
robert.swain
Wed Jul 30 20:24:44 CEST 2008
2008/7/30 Michael Niedermayer <michaelni at gmx.at>:
> 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}
I suppose that avoids more variables. I'll do that.
>> Should this be done just for
>> max_sfb and num_swb or are there others?
>
> does the code contain more that are security relevant?
What makes a context variable security relevant? If it's used for array indices?
>> > [...]
>> >> >> + }
>> >> >> + }
>> >> >> + }
>> >> >> + 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
Ah, sorry. I was pasting in-line as for some reason I thought it would
be easier to maintain context as you've been quoting the main patch
and commenting on it. I forgot that GMail does word wrapping after the
text has been written. See attached (01-band_types.diff).
> [...]
>>
>> >> >> + 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++)
Again, I thought you mean line-breaks in the code, not in how the
patch was coming through in the e-mail. Again, see attached
(20080728-1344-refactor_excluded_channels.diff).
Sorry for the confusion and in-line patches.
Regards,
Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-band_types.diff
Type: text/x-diff
Size: 3146 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080730/17a7ec15/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20080728-1344-refactor_excluded_channels.diff
Type: text/x-diff
Size: 1855 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080730/17a7ec15/attachment-0001.diff>
More information about the ffmpeg-devel
mailing list