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

Michael Niedermayer michaelni
Sun Jan 6 23:17:00 CET 2008


On Tue, Dec 18, 2007 at 11:38:09AM +0000, Ian Caulfield wrote:
> Hi,
> 
> New version attached, hopefully addressing most of your comments.
> 
> On Dec 10, 2007 11:20 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> > On Tue, Dec 04, 2007 at 03:50:50PM +0000, Ian Caulfield wrote:
> > [...]
> > > +/** Initialize the decoder. */
> > > +
> > > +static int mlp_decode_init(AVCodecContext *avctx)
> > > +{
> > > +    MLPDecodeContext *m = avctx->priv_data;
> > > +
> > > +    init_static();
> > > +    m->avctx = avctx;
> > > +    memset(m->lossless_check_data, 0xff, sizeof(m->lossless_check_data));
> > > +    return 0;
> > > +}
> >
> > is there a special reason why init_static() is a seperate function?
> 
> Not really - I can fold it in if you'd like.
> 
> > [...]
> > > +    dprintf(m->avctx, "Filter %d, order %d\n", filter, order);
> >
> > please merge these hundreads of dprintf() in few proper
> > if(debug & FF_DEBUG_*)
> >     av_log()
> >
> > or dprintf() instead of av_log() for the more obscure/less usefull ones
> >
> > having this debug stuff interleaved with normal code makes reading it
> > harder ...
> 
> I've deleted a lot of the dprintfs that weren't especially useful to
> me any more.
> 
> >
> > [...]
> > > +        if (restart_absent && !m->restart_seen[substr]) {
> > > +            av_log(m->avctx, AV_LOG_ERROR,
> > > +                   "No restart header indicated for substream %d", substr);
> > > +            goto error;
> > > +        }
> >
> > what is this check and the whole complicated restart_seen logic good
> > for?
> 
> The main point is to detect if the restart header containing the
> needed decoding parameters is missing and to error out if so. I've
> reworked the check to be somewhat saner.
[...]
> +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 ?


> +    uint8_t     min_channel[MAX_SUBSTREAMS];
> +    uint8_t     max_channel[MAX_SUBSTREAMS];
> +    uint8_t     max_matrix_channel[MAX_SUBSTREAMS];
> +    uint8_t     noise_shift[MAX_SUBSTREAMS];
> +    uint32_t    noisegen_seed[MAX_SUBSTREAMS];
> +    uint8_t     data_check_present[MAX_SUBSTREAMS];
> +    uint8_t     param_presence_flags[MAX_SUBSTREAMS];
> +    //@}
> +
> +    //@{
> +    /** Matrix data */
> +    uint8_t     num_primitive_matrices[MAX_SUBSTREAMS];
> +    uint8_t     matrix_ch[MAX_SUBSTREAMS][MAX_MATRICES];
> +    uint8_t     lsb_bypass[MAX_SUBSTREAMS][MAX_MATRICES];
> +    int32_t     matrix_coeff[MAX_SUBSTREAMS][MAX_MATRICES][MAX_CHANNELS+2];
> +    uint8_t     matrix_noise_shift[MAX_SUBSTREAMS][MAX_MATRICES];
> +    //@}
> +
> +    uint16_t    blocksize[MAX_SUBSTREAMS];
> +    uint16_t    blockpos[MAX_SUBSTREAMS];
> +    int8_t      output_shift[MAX_SUBSTREAMS][MAX_CHANNELS];
> +    uint8_t     quant_step_size[MAX_SUBSTREAMS][MAX_CHANNELS];
> +
> +    //@{
> +    /* Filter data. Filter 0 is an FIR filter, filter 1 IIR. */
> +    uint8_t     filter_order[MAX_CHANNELS][2];
> +    uint8_t     filter_coeff_q[MAX_CHANNELS][2];
> +    int32_t     filter_coeff[MAX_CHANNELS][2][MAX_FILTER_ORDER];
> +    int32_t     filter_state[MAX_CHANNELS][2][MAX_FILTER_ORDER];
> +    //@}
> +
> +    //@{
> +    /** Sample data coding infomation */
> +    int16_t     huff_offset[MAX_CHANNELS];
> +    int32_t     sign_huff_offset[MAX_CHANNELS];
> +    uint8_t     codebook[MAX_CHANNELS];
> +    uint8_t     huff_lsbs[MAX_CHANNELS];
> +    //@}
> +
> +    int32_t     lossless_check_data[MAX_SUBSTREAMS];
> +
> +    int8_t      noise_buffer[MAX_BLOCKSIZE_POW2];
> +    int8_t      bypassed_lsbs[MAX_BLOCKSIZE][MAX_CHANNELS];
> +    int32_t     sample_buffer[MAX_BLOCKSIZE][MAX_CHANNELS+2];
> +} MLPDecodeContext;

some comments explaing what each var is, would be nice


[...]
> +static inline void calculate_sign_huff(MLPDecodeContext *m, unsigned int substr,
> +                                       unsigned int ch)
> +{
> +    int lsb_bits = m->huff_lsbs[ch] - m->quant_step_size[substr][ch];
> +    int sign_shift = lsb_bits + (m->codebook[ch] ? 2 - m->codebook[ch] : -1);
> +
> +    m->sign_huff_offset[ch] = m->huff_offset[ch];
> +
> +    if (sign_shift >= 0)
> +        m->sign_huff_offset[ch] -= 1 << sign_shift;
> +}
> +
> +/** Read a sample, consisting of either, both or neither of entropy-coded MSBs
> + *  and plain LSBs.
> + */
> +
> +static inline int read_huff(MLPDecodeContext *m, GetBitContext *gbp,
> +                            unsigned int substr, unsigned int channel)
> +{
> +    int codebook = m->codebook[channel];
> +    int quant_step_size = m->quant_step_size[substr][channel];
> +    int lsb_bits = m->huff_lsbs[channel] - quant_step_size;
> +    int result = 0;
> +
> +    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


[...]
> +static void generate_noise_1(MLPDecodeContext *m, unsigned int substr)
> +{
> +    unsigned int i;
> +    uint32_t seed = m->noisegen_seed[substr];
> +    unsigned int maxchan = m->max_matrix_channel[substr];
> +
> +    for (i = 0; i < m->blockpos[substr]; i++) {
> +        uint16_t seed_shr7 = seed >> 7;
> +        m->sample_buffer[i][maxchan+1] = ((int8_t)(seed >> 15)) << m->noise_shift[substr];
> +        m->sample_buffer[i][maxchan+2] = ((int8_t) seed_shr7)   << m->noise_shift[substr];
> +
> +        seed = (seed << 16) ^ seed_shr7 ^ (seed_shr7 << 5);
> +    }
> +
> +    m->noisegen_seed[substr] = seed;
> +}
> +
> +/** Generate a block of noise, used when restart_sync == 0x31eb. */
> +
> +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


[...]
> +static int read_access_unit(AVCodecContext *avctx, void* data, int *data_size,
> +                            uint8_t *buf, int buf_size)
> +{
> +    MLPDecodeContext *m = avctx->priv_data;
> +    GetBitContext gb;
> +    unsigned int length, substr;
> +    unsigned int substream_start;
> +    unsigned int header_size;
> +    uint8_t substream_parity_present[MAX_SUBSTREAMS];
> +    uint16_t substream_data_len[MAX_SUBSTREAMS];
> +
> +    if (buf_size < 2)
> +        return 0;
> +
> +    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


[...]
> +        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!

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- 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/20080106/756f763f/attachment.pgp>



More information about the ffmpeg-devel mailing list