[FFmpeg-devel] [PATCH] MLP/TrueHD decoder

[Ian Caulfield]

Hi,

New version attached

On Jan 6, 2008 10:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> On Tue, Dec 18, 2007 at 11:38:09AM +0000, Ian Caulfield wrote:
> >
> > +typedef struct MLPDecodeContext {
> > +    AVCodecContext *avctx;
> > +
> > +    uint8_t     params_valid;
> > +    uint8_t     max_decoded_substream;
> > +    int         access_unit_size;
> > +    int         access_unit_size_pow2;
> > +    uint8_t     restart_seen[MAX_SUBSTREAMS];
> > +
> > +    uint8_t     num_substreams;
> > +
> > +    //@{
> > +    /** Restart header data */
>
> > +    uint16_t    restart_sync[MAX_SUBSTREAMS];
>
> shouldnt this rather be called sync_word ?

I've changed it to restart_sync_word (to distinguish it from the major
sync word if that ever needs to be stored in the context)

> > +} MLPDecodeContext;
>
> some comments explaing what each var is, would be nice

I've put comments for most of the variables (I thought some were
sufficiently obvious to not require explanation - will fill those in
too if you like).


> > +
> > +    if (codebook > 0)
> > +        result = get_vlc2(gbp, huff_vlc[codebook-1].table,
> > +                          VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7;
>
> the -7 can be merged into sign_huff_offset

Done

> > +static void generate_noise_2(MLPDecodeContext *m, unsigned int substr)
> > +{
> > +    unsigned int i;
> > +    uint32_t seed = m->noisegen_seed[substr];
> > +
> > +    for (i = 0; i < m->access_unit_size_pow2; i++) {
>
> > +        uint8_t tmp = seed >> 15;
>
> please name this seed_shr15

Done

> > +    if (*data_size < m->avctx->channels * m->access_unit_size
> > +                      * (m->avctx->sample_fmt == SAMPLE_FMT_S32 ? 4 : 2))
> > +        return -1;
>
> its nice that you check that, it would be nicer i you did after setting
> the variables, like that it is just exploitable

Moved to output_data_internal

>
> [...]
> > +        skip_bits(&gb, (-get_bits_count(&gb)) & 15);
> > +        if (show_bits_long(&gb, 32) == 0xd234d234 ||
> > +            show_bits_long(&gb, 18) == 0xd234e) {
> > +            skip_bits(&gb, 18);
> > +            av_log(m->avctx, AV_LOG_INFO, "End of stream indicated\n");
> > +            if (get_bits1(&gb))
> > +                m->blockpos[substr] -= get_bits(&gb, 13);
> > +            else
> > +                skip_bits(&gb, 13);
>
> exploitable, blockpos overflows and is later used as array limit
> (for exmple in rematrix_channels)
> iam starting to be scared by your patches, can you _please_ take this more
> serious, and check the variables used to decide where to write data to?
> all of them, double check your code, if i find another such issue i will not
> review this patch again and permanently reject it!

Sorry about that - I was very careful and checked all loops to make
sure the bounds were checked properly. I was then given a new sample
that decoded incorrectly and added the above code to handle it without
thinking fully. I'll avoid making any non-review changes until
reviewing is over, and try to be more paranoid in general. Hopefully
there aren't any holes left...

Ian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlpdec.patch
Type: text/x-diff
Size: 41044 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080107/4535117a/attachment.patch>