[FFmpeg-devel] [PATCH] Multi-Channel Correlation in ALS

[Michael Niedermayer]

On Tue, Dec 15, 2009 at 04:20:33PM +0100, Thilo Borgmann wrote:
> Hi,
> 
> this patch adds MCC decoding.
[...]
> @@ -%ld,%ld +%ld,%ld @@
>  }
>  
>  
> +/** Reads the channel data.
> +  */
> +static void read_channel_data(ALSDecContext *ctx, ALSChannelData *cd, int c)
> +{
> +    GetBitContext *gb       = &ctx->gb;
> +    ALSChannelData *current = cd;
> +

> +    while (!(current->stop_flag = get_bits1(gb))) {

> +        current->master_channel = get_bits_long(gb, av_ceil_log2(ctx->avctx->channels));

this is missing a check for master_channel < ctx->avctx->channels i suspect


> +
> +        if (current->master_channel != c) {
> +            current->time_diff_flag = get_bits1(gb);
> +            current->weighting[0]   = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +            current->weighting[1]   = mcc_weightings[av_clip(decode_rice(gb, 2) + 14, 0, 32)];
> +            current->weighting[2]   = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +
> +            if (current->time_diff_flag) {
> +                current->weighting[3] = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +                current->weighting[4] = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +                current->weighting[5] = mcc_weightings[av_clip(decode_rice(gb, 1) + 16, 0, 32)];
> +
> +                current->time_diff_sign  = get_bits1(gb);
> +                current->time_diff_index = get_bits(gb, ctx->ltp_lag_length - 3) + 3;
> +                if (current->time_diff_sign)
> +                    current->time_diff_index = -current->time_diff_index;
> +            }
> +        }
> +
> +        current++;
> +    }

buffer overflow


> +
> +    align_get_bits(gb);
> +}
> +
> +

> +/** Recursively reverts the inter-channel correlation for a block.
> + */
> +static void revert_channel_correlation(ALSDecContext *ctx, ALSBlockData *bd,
> +                                       ALSChannelData **cd, int *reverted,
> +                                       unsigned int offset, int c)
> +{
> +    ALSChannelData *ch = cd[c];
> +    unsigned int   dep = 0;
> +
> +    if (reverted[c])
> +        return;
> +
> +    while (!ch[dep].stop_flag) {
> +        if (!reverted[ch[dep].master_channel])
> +            revert_channel_correlation(ctx, bd, cd, reverted, offset,
> +                                       ch[dep].master_channel);
> +
> +        dep++;
> +    }

Make sure this cannot overflow the stack (every recursive call needs space
there)

also dont forget to test the code with a fuzzer to make sure it doesnt
crash and please also go over it to make sure there are no other buffer
overflows, you know the code better then i do so i might not spot all.


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

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/20091216/5d02f51c/attachment.pgp>