| Message ID | 20220317233019.12049-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/dfpwmdec: Check packet size more completely | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
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; > } >
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; >> }
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; > }
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 [...]
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.
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 [...]
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 [...]
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.
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; }
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(+)