[FFmpeg-devel] [PATCH 1/3] avcodec/dfpwmdec: Check packet size more completely

[FFmpeg-devel] [PATCH 1/3] avcodec/dfpwmdec: Check packet size more completely

[Michael Niedermayer]

Fixes: out of array access
Fixes: 45497/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_DFPWM_fuzzer-5239786212818944.fuzz
Fixes: 45510/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_DFPWM_fuzzer-4947856883056640

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/dfpwmdec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavcodec/dfpwmdec.c b/libavcodec/dfpwmdec.c
index 27b60a4ea4..e11f393ee4 100644
--- a/libavcodec/dfpwmdec.c
+++ b/libavcodec/dfpwmdec.c
@@ -106,7 +106,10 @@ static int dfpwm_dec_frame(struct AVCodecContext *ctx, void *data,
     AVFrame *frame = data;
     int ret;
 
-    frame->nb_samples = packet->size * 8 / ctx->ch_layout.nb_channels;
+    if (packet->size * 8LL % ctx->ch_layout.nb_channels)
+        return AVERROR_PATCHWELCOME;
+
+    frame->nb_samples = packet->size * 8LL / ctx->ch_layout.nb_channels;
     if (frame->nb_samples <= 0) {
         av_log(ctx, AV_LOG_ERROR, "invalid number of samples in packet\n");
         return AVERROR_INVALIDDATA;
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

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@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 ?
+
     return 0;
 }
 
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[James Almer]

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

> +
>       return 0;
>   }
>   
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@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;
>>   }
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[James Almer]

On 3/17/2022 9:07 PM, James Almer wrote:
> 
> 
> 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@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.

Like this maybe (channel_mask could in theory be zero, so in that case 
the user set value should be used).

> diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
> index cd05b22689..915add1962 100644
> --- a/libavcodec/wmalosslessdec.c
> +++ b/libavcodec/wmalosslessdec.c
> @@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
>          return AVERROR_PATCHWELCOME;
>      }
> 
> -    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> -    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> -    if (!s->frame_data)
> -        return AVERROR(ENOMEM);
> -
> -    s->avctx = avctx;
> -    ff_llauddsp_init(&s->dsp);
> -    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> -
>      if (avctx->extradata_size >= 18) {
>          s->decode_flags    = AV_RL16(edata_ptr + 14);
>          channel_mask       = AV_RL32(edata_ptr +  2);
> @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
>          return AVERROR_PATCHWELCOME;
>      }
> 
> +    if (channel_mask) {
> +        av_channel_layout_uninit(&avctx->ch_layout);
> +        av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> +    } else
> +        avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> +
> +    s->num_channels = avctx->ch_layout.nb_channels;
> +
> +    /* extract lfe channel position */
> +    s->lfe_channel = -1;
> +
> +    if (channel_mask & 8) {
> +        unsigned int mask;
> +        for (mask = 1; mask < 16; mask <<= 1)
> +            if (channel_mask & mask)
> +                ++s->lfe_channel;
> +    }
> +
> +    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> +    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +    if (!s->frame_data)
> +        return AVERROR(ENOMEM);
> +
> +    s->avctx = avctx;
> +    ff_llauddsp_init(&s->dsp);
> +    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> +
>      /* generic init */
>      s->log2_frame_size = av_log2(avctx->block_align) + 4;
> 
> @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
>          return AVERROR_INVALIDDATA;
>      }
> 
> -    s->num_channels = avctx->ch_layout.nb_channels;
> -
> -    /* extract lfe channel position */
> -    s->lfe_channel = -1;
> -
> -    if (channel_mask & 8) {
> -        unsigned int mask;
> -        for (mask = 1; mask < 16; mask <<= 1)
> -            if (channel_mask & mask)
> -                ++s->lfe_channel;
> -    }
> -
>      s->frame = av_frame_alloc();
>      if (!s->frame)
>          return AVERROR(ENOMEM);
> 
> -    av_channel_layout_uninit(&avctx->ch_layout);
> -    av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>      return 0;
>  }
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 5842 bytes --]

On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote:
> 
> 
> On 3/17/2022 9:07 PM, James Almer wrote:
> > 
> > 
> > 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@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.
> 
> Like this maybe (channel_mask could in theory be zero, so in that case the
> user set value should be used).
> 
> > diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
> > index cd05b22689..915add1962 100644
> > --- a/libavcodec/wmalosslessdec.c
> > +++ b/libavcodec/wmalosslessdec.c
> > @@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >          return AVERROR_PATCHWELCOME;
> >      }
> > 
> > -    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> > -    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > -    if (!s->frame_data)
> > -        return AVERROR(ENOMEM);
> > -
> > -    s->avctx = avctx;
> > -    ff_llauddsp_init(&s->dsp);
> > -    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> > -
> >      if (avctx->extradata_size >= 18) {
> >          s->decode_flags    = AV_RL16(edata_ptr + 14);
> >          channel_mask       = AV_RL32(edata_ptr +  2);
> > @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >          return AVERROR_PATCHWELCOME;
> >      }
> > 
> > +    if (channel_mask) {
> > +        av_channel_layout_uninit(&avctx->ch_layout);
> > +        av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> > +    } else
> > +        avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> > +
> > +    s->num_channels = avctx->ch_layout.nb_channels;
> > +
> > +    /* extract lfe channel position */
> > +    s->lfe_channel = -1;
> > +
> > +    if (channel_mask & 8) {
> > +        unsigned int mask;
> > +        for (mask = 1; mask < 16; mask <<= 1)
> > +            if (channel_mask & mask)
> > +                ++s->lfe_channel;
> > +    }
> > +
> > +    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> > +    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > +    if (!s->frame_data)
> > +        return AVERROR(ENOMEM);
> > +
> > +    s->avctx = avctx;
> > +    ff_llauddsp_init(&s->dsp);
> > +    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> > +
> >      /* generic init */
> >      s->log2_frame_size = av_log2(avctx->block_align) + 4;
> > 
> > @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >          return AVERROR_INVALIDDATA;
> >      }
> > 
> > -    s->num_channels = avctx->ch_layout.nb_channels;
> > -
> > -    /* extract lfe channel position */
> > -    s->lfe_channel = -1;
> > -
> > -    if (channel_mask & 8) {
> > -        unsigned int mask;
> > -        for (mask = 1; mask < 16; mask <<= 1)
> > -            if (channel_mask & mask)
> > -                ++s->lfe_channel;
> > -    }
> > -
> >      s->frame = av_frame_alloc();
> >      if (!s->frame)
> >          return AVERROR(ENOMEM);
> > 
> > -    av_channel_layout_uninit(&avctx->ch_layout);
> > -    av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> >      return 0;
> >  }

this will change the output of the "lossless" decoder if this case ever
occurs besides this

LGTM

thx

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

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[James Almer]

On 3/18/2022 8:27 AM, Michael Niedermayer wrote:
> On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote:
>>
>>
>> On 3/17/2022 9:07 PM, James Almer wrote:
>>>
>>>
>>> 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@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.
>>
>> Like this maybe (channel_mask could in theory be zero, so in that case the
>> user set value should be used).
>>
>>> diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
>>> index cd05b22689..915add1962 100644
>>> --- a/libavcodec/wmalosslessdec.c
>>> +++ b/libavcodec/wmalosslessdec.c
>>> @@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>           return AVERROR_PATCHWELCOME;
>>>       }
>>>
>>> -    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
>>> -    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>> -    if (!s->frame_data)
>>> -        return AVERROR(ENOMEM);
>>> -
>>> -    s->avctx = avctx;
>>> -    ff_llauddsp_init(&s->dsp);
>>> -    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
>>> -
>>>       if (avctx->extradata_size >= 18) {
>>>           s->decode_flags    = AV_RL16(edata_ptr + 14);
>>>           channel_mask       = AV_RL32(edata_ptr +  2);
>>> @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>           return AVERROR_PATCHWELCOME;
>>>       }
>>>
>>> +    if (channel_mask) {
>>> +        av_channel_layout_uninit(&avctx->ch_layout);
>>> +        av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>>> +    } else
>>> +        avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
>>> +
>>> +    s->num_channels = avctx->ch_layout.nb_channels;
>>> +
>>> +    /* extract lfe channel position */
>>> +    s->lfe_channel = -1;
>>> +
>>> +    if (channel_mask & 8) {
>>> +        unsigned int mask;
>>> +        for (mask = 1; mask < 16; mask <<= 1)
>>> +            if (channel_mask & mask)
>>> +                ++s->lfe_channel;
>>> +    }
>>> +
>>> +    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
>>> +    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>> +    if (!s->frame_data)
>>> +        return AVERROR(ENOMEM);
>>> +
>>> +    s->avctx = avctx;
>>> +    ff_llauddsp_init(&s->dsp);
>>> +    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
>>> +
>>>       /* generic init */
>>>       s->log2_frame_size = av_log2(avctx->block_align) + 4;
>>>
>>> @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>           return AVERROR_INVALIDDATA;
>>>       }
>>>
>>> -    s->num_channels = avctx->ch_layout.nb_channels;
>>> -
>>> -    /* extract lfe channel position */
>>> -    s->lfe_channel = -1;
>>> -
>>> -    if (channel_mask & 8) {
>>> -        unsigned int mask;
>>> -        for (mask = 1; mask < 16; mask <<= 1)
>>> -            if (channel_mask & mask)
>>> -                ++s->lfe_channel;
>>> -    }
>>> -
>>>       s->frame = av_frame_alloc();
>>>       if (!s->frame)
>>>           return AVERROR(ENOMEM);
>>>
>>> -    av_channel_layout_uninit(&avctx->ch_layout);
>>> -    av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>>>       return 0;
>>>   }
> 
> this will change the output of the "lossless" decoder if this case ever
> occurs besides this

As i interpret it, you could initialize the decoder using any arbitrary 
number of channels in avctx. And if channel_mask in extradata is not 
zero, then it must be considered to be the valid channel count for the 
stream, and the value passed in avctx should be ignored.

> 
> LGTM
> 
> thx

Will apply.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 6859 bytes --]

On Fri, Mar 18, 2022 at 08:38:28AM -0300, James Almer wrote:
> 
> 
> On 3/18/2022 8:27 AM, Michael Niedermayer wrote:
> > On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote:
> > > 
> > > 
> > > On 3/17/2022 9:07 PM, James Almer wrote:
> > > > 
> > > > 
> > > > 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@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.
> > > 
> > > Like this maybe (channel_mask could in theory be zero, so in that case the
> > > user set value should be used).
> > > 
> > > > diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
> > > > index cd05b22689..915add1962 100644
> > > > --- a/libavcodec/wmalosslessdec.c
> > > > +++ b/libavcodec/wmalosslessdec.c
> > > > @@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > >           return AVERROR_PATCHWELCOME;
> > > >       }
> > > > 
> > > > -    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> > > > -    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > > > -    if (!s->frame_data)
> > > > -        return AVERROR(ENOMEM);
> > > > -
> > > > -    s->avctx = avctx;
> > > > -    ff_llauddsp_init(&s->dsp);
> > > > -    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> > > > -
> > > >       if (avctx->extradata_size >= 18) {
> > > >           s->decode_flags    = AV_RL16(edata_ptr + 14);
> > > >           channel_mask       = AV_RL32(edata_ptr +  2);
> > > > @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > >           return AVERROR_PATCHWELCOME;
> > > >       }
> > > > 
> > > > +    if (channel_mask) {
> > > > +        av_channel_layout_uninit(&avctx->ch_layout);
> > > > +        av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> > > > +    } else
> > > > +        avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> > > > +
> > > > +    s->num_channels = avctx->ch_layout.nb_channels;
> > > > +
> > > > +    /* extract lfe channel position */
> > > > +    s->lfe_channel = -1;
> > > > +
> > > > +    if (channel_mask & 8) {
> > > > +        unsigned int mask;
> > > > +        for (mask = 1; mask < 16; mask <<= 1)
> > > > +            if (channel_mask & mask)
> > > > +                ++s->lfe_channel;
> > > > +    }
> > > > +
> > > > +    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> > > > +    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > > > +    if (!s->frame_data)
> > > > +        return AVERROR(ENOMEM);
> > > > +
> > > > +    s->avctx = avctx;
> > > > +    ff_llauddsp_init(&s->dsp);
> > > > +    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> > > > +
> > > >       /* generic init */
> > > >       s->log2_frame_size = av_log2(avctx->block_align) + 4;
> > > > 
> > > > @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > >           return AVERROR_INVALIDDATA;
> > > >       }
> > > > 
> > > > -    s->num_channels = avctx->ch_layout.nb_channels;
> > > > -
> > > > -    /* extract lfe channel position */
> > > > -    s->lfe_channel = -1;
> > > > -
> > > > -    if (channel_mask & 8) {
> > > > -        unsigned int mask;
> > > > -        for (mask = 1; mask < 16; mask <<= 1)
> > > > -            if (channel_mask & mask)
> > > > -                ++s->lfe_channel;
> > > > -    }
> > > > -
> > > >       s->frame = av_frame_alloc();
> > > >       if (!s->frame)
> > > >           return AVERROR(ENOMEM);
> > > > 
> > > > -    av_channel_layout_uninit(&avctx->ch_layout);
> > > > -    av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> > > >       return 0;
> > > >   }
> > 
> > this will change the output of the "lossless" decoder if this case ever
> > occurs besides this
> 
> As i interpret it, you could initialize the decoder using any arbitrary
> number of channels in avctx. And if channel_mask in extradata is not zero,
> then it must be considered to be the valid channel count for the stream, and
> the value passed in avctx should be ignored.

without a specification and no non fuzzed testfile its really not possible
to say for sure what this combination is intended to produce on the output

What you say sounds reasonable but it can also be something else

thx

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 7550 bytes --]

On Sat, Mar 19, 2022 at 06:52:34PM +0100, Michael Niedermayer wrote:
> On Fri, Mar 18, 2022 at 08:38:28AM -0300, James Almer wrote:
> > 
> > 
> > On 3/18/2022 8:27 AM, Michael Niedermayer wrote:
> > > On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote:
> > > > 
> > > > 
> > > > On 3/17/2022 9:07 PM, James Almer wrote:
> > > > > 
> > > > > 
> > > > > 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@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.
> > > > 
> > > > Like this maybe (channel_mask could in theory be zero, so in that case the
> > > > user set value should be used).
> > > > 
> > > > > diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
> > > > > index cd05b22689..915add1962 100644
> > > > > --- a/libavcodec/wmalosslessdec.c
> > > > > +++ b/libavcodec/wmalosslessdec.c
> > > > > @@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > > >           return AVERROR_PATCHWELCOME;
> > > > >       }
> > > > > 
> > > > > -    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> > > > > -    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > > > > -    if (!s->frame_data)
> > > > > -        return AVERROR(ENOMEM);
> > > > > -
> > > > > -    s->avctx = avctx;
> > > > > -    ff_llauddsp_init(&s->dsp);
> > > > > -    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> > > > > -
> > > > >       if (avctx->extradata_size >= 18) {
> > > > >           s->decode_flags    = AV_RL16(edata_ptr + 14);
> > > > >           channel_mask       = AV_RL32(edata_ptr +  2);
> > > > > @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > > >           return AVERROR_PATCHWELCOME;
> > > > >       }
> > > > > 
> > > > > +    if (channel_mask) {
> > > > > +        av_channel_layout_uninit(&avctx->ch_layout);
> > > > > +        av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> > > > > +    } else
> > > > > +        avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> > > > > +
> > > > > +    s->num_channels = avctx->ch_layout.nb_channels;
> > > > > +
> > > > > +    /* extract lfe channel position */
> > > > > +    s->lfe_channel = -1;
> > > > > +
> > > > > +    if (channel_mask & 8) {
> > > > > +        unsigned int mask;
> > > > > +        for (mask = 1; mask < 16; mask <<= 1)
> > > > > +            if (channel_mask & mask)
> > > > > +                ++s->lfe_channel;
> > > > > +    }
> > > > > +
> > > > > +    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> > > > > +    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > > > > +    if (!s->frame_data)
> > > > > +        return AVERROR(ENOMEM);
> > > > > +
> > > > > +    s->avctx = avctx;
> > > > > +    ff_llauddsp_init(&s->dsp);
> > > > > +    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> > > > > +
> > > > >       /* generic init */
> > > > >       s->log2_frame_size = av_log2(avctx->block_align) + 4;
> > > > > 
> > > > > @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > > >           return AVERROR_INVALIDDATA;
> > > > >       }
> > > > > 
> > > > > -    s->num_channels = avctx->ch_layout.nb_channels;
> > > > > -
> > > > > -    /* extract lfe channel position */
> > > > > -    s->lfe_channel = -1;
> > > > > -
> > > > > -    if (channel_mask & 8) {
> > > > > -        unsigned int mask;
> > > > > -        for (mask = 1; mask < 16; mask <<= 1)
> > > > > -            if (channel_mask & mask)
> > > > > -                ++s->lfe_channel;
> > > > > -    }
> > > > > -
> > > > >       s->frame = av_frame_alloc();
> > > > >       if (!s->frame)
> > > > >           return AVERROR(ENOMEM);
> > > > > 
> > > > > -    av_channel_layout_uninit(&avctx->ch_layout);
> > > > > -    av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> > > > >       return 0;
> > > > >   }
> > > 
> > > this will change the output of the "lossless" decoder if this case ever
> > > occurs besides this
> > 
> > As i interpret it, you could initialize the decoder using any arbitrary
> > number of channels in avctx. And if channel_mask in extradata is not zero,
> > then it must be considered to be the valid channel count for the stream, and
> > the value passed in avctx should be ignored.
> 
> without a specification and no non fuzzed testfile its really not possible
> to say for sure what this combination is intended to produce on the output
> 
> What you say sounds reasonable but it can also be something else

ping, i think your patch or something similar should be applied
The fuzzer found
46008/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-4681245747970048
which too crashes because of the channel number

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[James Almer]

On 3/29/2022 4:02 PM, Michael Niedermayer wrote:
> On Sat, Mar 19, 2022 at 06:52:34PM +0100, Michael Niedermayer wrote:
>> On Fri, Mar 18, 2022 at 08:38:28AM -0300, James Almer wrote:
>>>
>>>
>>> On 3/18/2022 8:27 AM, Michael Niedermayer wrote:
>>>> On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote:
>>>>>
>>>>>
>>>>> On 3/17/2022 9:07 PM, James Almer wrote:
>>>>>>
>>>>>>
>>>>>> 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@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.
>>>>>
>>>>> Like this maybe (channel_mask could in theory be zero, so in that case the
>>>>> user set value should be used).
>>>>>
>>>>>> diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
>>>>>> index cd05b22689..915add1962 100644
>>>>>> --- a/libavcodec/wmalosslessdec.c
>>>>>> +++ b/libavcodec/wmalosslessdec.c
>>>>>> @@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>>>>            return AVERROR_PATCHWELCOME;
>>>>>>        }
>>>>>>
>>>>>> -    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
>>>>>> -    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>> -    if (!s->frame_data)
>>>>>> -        return AVERROR(ENOMEM);
>>>>>> -
>>>>>> -    s->avctx = avctx;
>>>>>> -    ff_llauddsp_init(&s->dsp);
>>>>>> -    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
>>>>>> -
>>>>>>        if (avctx->extradata_size >= 18) {
>>>>>>            s->decode_flags    = AV_RL16(edata_ptr + 14);
>>>>>>            channel_mask       = AV_RL32(edata_ptr +  2);
>>>>>> @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>>>>            return AVERROR_PATCHWELCOME;
>>>>>>        }
>>>>>>
>>>>>> +    if (channel_mask) {
>>>>>> +        av_channel_layout_uninit(&avctx->ch_layout);
>>>>>> +        av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>>>>>> +    } else
>>>>>> +        avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
>>>>>> +
>>>>>> +    s->num_channels = avctx->ch_layout.nb_channels;
>>>>>> +
>>>>>> +    /* extract lfe channel position */
>>>>>> +    s->lfe_channel = -1;
>>>>>> +
>>>>>> +    if (channel_mask & 8) {
>>>>>> +        unsigned int mask;
>>>>>> +        for (mask = 1; mask < 16; mask <<= 1)
>>>>>> +            if (channel_mask & mask)
>>>>>> +                ++s->lfe_channel;
>>>>>> +    }
>>>>>> +
>>>>>> +    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
>>>>>> +    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>> +    if (!s->frame_data)
>>>>>> +        return AVERROR(ENOMEM);
>>>>>> +
>>>>>> +    s->avctx = avctx;
>>>>>> +    ff_llauddsp_init(&s->dsp);
>>>>>> +    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
>>>>>> +
>>>>>>        /* generic init */
>>>>>>        s->log2_frame_size = av_log2(avctx->block_align) + 4;
>>>>>>
>>>>>> @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>>>>            return AVERROR_INVALIDDATA;
>>>>>>        }
>>>>>>
>>>>>> -    s->num_channels = avctx->ch_layout.nb_channels;
>>>>>> -
>>>>>> -    /* extract lfe channel position */
>>>>>> -    s->lfe_channel = -1;
>>>>>> -
>>>>>> -    if (channel_mask & 8) {
>>>>>> -        unsigned int mask;
>>>>>> -        for (mask = 1; mask < 16; mask <<= 1)
>>>>>> -            if (channel_mask & mask)
>>>>>> -                ++s->lfe_channel;
>>>>>> -    }
>>>>>> -
>>>>>>        s->frame = av_frame_alloc();
>>>>>>        if (!s->frame)
>>>>>>            return AVERROR(ENOMEM);
>>>>>>
>>>>>> -    av_channel_layout_uninit(&avctx->ch_layout);
>>>>>> -    av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>>>>>>        return 0;
>>>>>>    }
>>>>
>>>> this will change the output of the "lossless" decoder if this case ever
>>>> occurs besides this
>>>
>>> As i interpret it, you could initialize the decoder using any arbitrary
>>> number of channels in avctx. And if channel_mask in extradata is not zero,
>>> then it must be considered to be the valid channel count for the stream, and
>>> the value passed in avctx should be ignored.
>>
>> without a specification and no non fuzzed testfile its really not possible
>> to say for sure what this combination is intended to produce on the output
>>
>> What you say sounds reasonable but it can also be something else
> 
> ping, i think your patch or something similar should be applied
> The fuzzer found
> 46008/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-4681245747970048
> which too crashes because of the channel number
> 
> thx

Applied.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/3] avcodec/alsdec: Set channels from data after data is set

[Michael Niedermayer]

Fixes: out of array write
Fixes: 45624/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALS_fuzzer-6473487382872064
Fixes: 45626/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALS_fuzzer-4874997192065024

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/alsdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
index 822cf211b0..73af829178 100644
--- a/libavcodec/alsdec.c
+++ b/libavcodec/alsdec.c
@@ -1986,7 +1986,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
     unsigned int c;
     unsigned int channel_size;
     int num_buffers, ret;
-    int channels = avctx->ch_layout.nb_channels;
+    int channels;
     ALSDecContext *ctx = avctx->priv_data;
     ALSSpecificConfig *sconf = &ctx->sconf;
     ctx->avctx = avctx;
@@ -2000,6 +2000,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
         av_log(avctx, AV_LOG_ERROR, "Reading ALSSpecificConfig failed.\n");
         return ret;
     }
+    channels = avctx->ch_layout.nb_channels;
 
     if ((ret = check_specific_config(ctx)) < 0) {
         return ret;
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/alsdec: Set channels from data after data is set

[James Almer]

On 3/17/2022 8:30 PM, Michael Niedermayer wrote:
> Fixes: out of array write
> Fixes: 45624/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALS_fuzzer-6473487382872064
> Fixes: 45626/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALS_fuzzer-4874997192065024
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavcodec/alsdec.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
> index 822cf211b0..73af829178 100644
> --- a/libavcodec/alsdec.c
> +++ b/libavcodec/alsdec.c
> @@ -1986,7 +1986,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
>       unsigned int c;
>       unsigned int channel_size;
>       int num_buffers, ret;
> -    int channels = avctx->ch_layout.nb_channels;
> +    int channels;
>       ALSDecContext *ctx = avctx->priv_data;
>       ALSSpecificConfig *sconf = &ctx->sconf;
>       ctx->avctx = avctx;
> @@ -2000,6 +2000,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
>           av_log(avctx, AV_LOG_ERROR, "Reading ALSSpecificConfig failed.\n");
>           return ret;
>       }
> +    channels = avctx->ch_layout.nb_channels;
>   
>       if ((ret = check_specific_config(ctx)) < 0) {
>           return ret;

LGTM
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/alsdec: Set channels from data after data is set

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1727 bytes --]

On Thu, Mar 17, 2022 at 08:40:48PM -0300, James Almer wrote:
> On 3/17/2022 8:30 PM, Michael Niedermayer wrote:
> > Fixes: out of array write
> > Fixes: 45624/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALS_fuzzer-6473487382872064
> > Fixes: 45626/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALS_fuzzer-4874997192065024
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavcodec/alsdec.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
> > index 822cf211b0..73af829178 100644
> > --- a/libavcodec/alsdec.c
> > +++ b/libavcodec/alsdec.c
> > @@ -1986,7 +1986,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >       unsigned int c;
> >       unsigned int channel_size;
> >       int num_buffers, ret;
> > -    int channels = avctx->ch_layout.nb_channels;
> > +    int channels;
> >       ALSDecContext *ctx = avctx->priv_data;
> >       ALSSpecificConfig *sconf = &ctx->sconf;
> >       ctx->avctx = avctx;
> > @@ -2000,6 +2000,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >           av_log(avctx, AV_LOG_ERROR, "Reading ALSSpecificConfig failed.\n");
> >           return ret;
> >       }
> > +    channels = avctx->ch_layout.nb_channels;
> >       if ((ret = check_specific_config(ctx)) < 0) {
> >           return ret;
> 
> LGTM

will apply

thx

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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/dfpwmdec: Check packet size more completely

[Paul B Mahol]

lgtm
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/dfpwmdec: Check packet size more completely

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 232 bytes --]

On Fri, Mar 18, 2022 at 09:31:27AM +0100, Paul B Mahol wrote:
> lgtm

will apply

thx

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

You can kill me, but you cannot change the truth.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".