[FFmpeg-devel] [PATCH 2/3] avcodec/wmalosslessdec: Check channel mask against num channels

[James Almer]

On 3/17/2022 8:52 PM, James Almer wrote:
> On 3/17/2022 8:30 PM, Michael Niedermayer wrote:
>> Fixes: Out of array write
>> Fixes: 
>> 45613/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-4539073606320128 
>>
>>
>> Found-by: continuous fuzzing process 
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> ---
>>   libavcodec/wmalosslessdec.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
>> index cd05b22689..1728920729 100644
>> --- a/libavcodec/wmalosslessdec.c
>> +++ b/libavcodec/wmalosslessdec.c
>> @@ -281,6 +281,9 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>       av_channel_layout_uninit(&avctx->ch_layout);
>>       av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>> +    if (s->num_channels != avctx->ch_layout.nb_channels)
>> +        return AVERROR_PATCHWELCOME; //are there non fuzzed files 
>> with this or is it an error ?
> 
> s->num_channels at this point is set to the channels count the user set 
> before calling avcodec_open2() (Normally from lavf), but it could be 
> anything.
> If channel_mask is taken from extradata, maybe it should be used to set 
> s->num_channels instead of aborting because the user set value and 
> extradata disagreed.
> 
> Also, can you reproduce this crash before 3c933af493? s->num_channels 
> was being set to the user set channel count too, same as now.

Right, before that commit s->num_channels and avctx->channels were 
always the same, but avctx->channel_layout was whatever came from 
extradata, and its popcnt could be != avctx->channels.
After it, avctx->ch_layout.nb_channels is always the same as 
popcnt(avctx->ch_layout.u.mask), which can be different than 
s->num_channels.

I think my suggestion above to use the extradata channel mask and 
ignoring the user set channel count is the best approach for this.

> 
>> +
>>       return 0;
>>   }