[FFmpeg-cvslog] r15812 - in trunk/libavcodec: ac3dec.c ac3dec.h ac3dec_data.c ac3dec_data.h eac3dec.c

[Michael Niedermayer]

On Thu, Nov 13, 2008 at 04:18:13AM +0100, jbr wrote:
> Author: jbr
> Date: Thu Nov 13 04:18:13 2008
> New Revision: 15812
> 
> Log:
> add support for spectral extension

This code looks like it completely lacks validity checks and likely
exploitable at several points.
I am not asking you to revert it but i would be happy if you did anyway.
This code should have passed review before commiting IMHO

Below review is incomplete, there likely are more issues, also iam not
mentioning the exploitable code as this patch needs to be reviewed completely
for security issues (which i did not do) not just the one issue ive found
fixed.


[...]

> @@ -88,6 +93,26 @@ typedef struct {
>      int cpl_coords[AC3_MAX_CHANNELS][18];   ///< coupling coordinates                   (cplco)
>  ///@}
>  
> +///@defgroup spx spectral extension
> +///@{

> +    int spx_in_use[MAX_BLOCKS];             ///< spectral extension in use              (spxinu)

doesnt need to be an array


> +    int channel_in_spx[AC3_MAX_CHANNELS];   ///< channel in spectral extension          (chinspx)

uint8_t


> +    int spx_atten_code[AC3_MAX_CHANNELS];   ///< spx attenuation code                   (spxattencod)

int8_t
and many others also waste space


> +    int spx_coords_exist[AC3_MAX_CHANNELS]; ///< indicates if a channel has spx coords  (spxcoe)

unused


> +    int spx_start_subband;                  ///< spx beginning frequency band           (spxbegf)
> +    int spx_start_freq;                     ///< spx start frequency bin
> +    int spx_end_freq;                       ///< spx end frequency bin
> +    int spx_copy_start_freq;                ///< spx starting frequency for copying     (copystartmant)

> +    int num_spx_subbands;                   ///< number of spectral extension subbands

unused, and ive not checked variables exhaustivles so there may be more


[...]

> Modified: trunk/libavcodec/ac3dec_data.c
> ==============================================================================
> --- trunk/libavcodec/ac3dec_data.c	(original)
> +++ trunk/libavcodec/ac3dec_data.c	Thu Nov 13 04:18:13 2008
> @@ -1127,6 +1127,52 @@ const uint8_t ff_eac3_frm_expstr[32][6] 
>  const uint8_t ff_eac3_default_cpl_band_struct[18] =
>  { 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 1, 0, 1, 1, 1, 1, 1 };
>  
> + /**
> + * Table E2.15 Default Spectral Extension Banding Structure
> + */

align of *


[...]

> +        if ((bin + bandsize) > s->spx_start_freq) {
> +            copy_sizes[num_copy_sections++] = bin - s->spx_copy_start_freq;
> +            bin = s->spx_copy_start_freq;
> +            wrapflag[bnd] = 1;
> +        }
> +        for (i = 0; i < bandsize; i++) {
> +            if (bin == s->spx_start_freq) {
> +                copy_sizes[num_copy_sections++] = bin - s->spx_copy_start_freq;
> +                bin = s->spx_copy_start_freq;
> +            }
> +            bin++;
> +        }

these 2 look rather similar and the second looks inefficient


[...]

> +            bin = s->spx_start_freq - 2;
> +            for (i = 0; i < 5; i++) {
> +                s->fixed_coeffs[ch][bin] = ((int64_t)atten_tab[2-abs(i-2)] *
> +                        (int64_t)s->fixed_coeffs[ch][bin]) >> 23;
> +                bin++;
> +            }

> +            /* apply notch at all other wrap points */
> +            bin += s->spx_band_sizes[0];
> +            for (bnd = 1; bnd < s->num_spx_bands; bnd++) {
> +                if (wrapflag[bnd]) {
> +                    bin -= 5;

> +                    for (i = 0; i < 5; i++) {
> +                        s->fixed_coeffs[ch][bin] = (atten_tab[2-abs(i-2)] *
> +                                (int64_t)s->fixed_coeffs[ch][bin]) >> 23;
> +                        bin++;
> +                    }

these 2 look like they could be in 1 loop, also it might be worth
to benchmark this against a unrolled one


> +                }
> +                bin += s->spx_band_sizes[bnd];
> +            }
> +        }
> +
> +        /* Apply noise-blended coefficient scaling based on previously
> +           calculated RMS energy, blending factors, and SPX coordinates for
> +           each band. */
> +        bin = s->spx_start_freq;
> +        for (bnd = 0; bnd < s->num_spx_bands; bnd++) {
> +            int64_t nscale, sscale, spxco;

> +            nscale = (s->spx_noise_blend [ch][bnd] * rms_energy[bnd]) >> 23;
> +            nscale = (nscale * 14529495) >> 23;

14529495 can be merged into some constants used to build spx_noise_blend


> +            sscale = s->spx_signal_blend[ch][bnd];
> +            spxco  = s->spx_coords[ch][bnd];

> +            for (i = 0; i < s->spx_band_sizes[bnd]; i++) {
> +                int64_t noise  = (nscale * (((int)av_lfg_get(&s->dith_state))>>8)) >> 23;
> +                int64_t signal = (sscale * s->fixed_coeffs[ch][bin]) >> 23;
> +                s->fixed_coeffs[ch][bin++] = ((noise + signal) * spxco) >> 23;

spxco can be multiplied into nscale and sscale thus avoiding the second
multiply
(noise>>23) + (signal>>23)
could be changed to
(noise + signal)>>23


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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- 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-cvslog/attachments/20081113/202083f8/attachment.pgp>