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

[Thilo Borgmann]

Am 16.12.09 05:07, schrieb 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

Done.

> 
> 
>> +
>> +        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

Yes, number of iterations of the while-loop is limitied now.


> 
> 
>> +
>> +    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)

Is there a way to find out if the stack memory gets close to zero and
would this be an appropriate check?
In this patch, the stop_flag will always be set so the number of
iterations of the while-loop is always finite.

> 
> 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.

I played around with the trasher a lot and got some pitfalls removed. I
end up in demuxer segfaults for my trashed files.

I also don't see any more dangerous loops in the code added.

-Thilo
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: als_mcc.rev1.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091221/b7c20328/attachment.asc>