[FFmpeg-devel] [PATCH 2/3] avcodec/wmalosslessdec: Check channel mask against num channels
Michael Niedermayer
michael at niedermayer.cc
Sat Mar 19 19:52:34 EET 2022
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 at niedermayer.cc>
> > > > > > ---
> > > > > > libavcodec/wmalosslessdec.c | 3 +++
> > > > > > 1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
> > > > > > index cd05b22689..1728920729 100644
> > > > > > --- a/libavcodec/wmalosslessdec.c
> > > > > > +++ b/libavcodec/wmalosslessdec.c
> > > > > > @@ -281,6 +281,9 @@ static av_cold int
> > > > > > decode_init(AVCodecContext *avctx)
> > > > > > av_channel_layout_uninit(&avctx->ch_layout);
> > > > > > av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> > > > > > + if (s->num_channels != avctx->ch_layout.nb_channels)
> > > > > > + return AVERROR_PATCHWELCOME; //are there non fuzzed
> > > > > > files with this or is it an error ?
> > > > >
> > > > > s->num_channels at this point is set to the channels count the user
> > > > > set before calling avcodec_open2() (Normally from lavf), but it
> > > > > could be anything.
> > > > > If channel_mask is taken from extradata, maybe it should be used to
> > > > > set s->num_channels instead of aborting because the user set value
> > > > > and extradata disagreed.
> > > > >
> > > > > Also, can you reproduce this crash before 3c933af493?
> > > > > s->num_channels was being set to the user set channel count too,
> > > > > same as now.
> > > >
> > > > Right, before that commit s->num_channels and avctx->channels were
> > > > always the same, but avctx->channel_layout was whatever came from
> > > > extradata, and its popcnt could be != avctx->channels.
> > > > After it, avctx->ch_layout.nb_channels is always the same as
> > > > popcnt(avctx->ch_layout.u.mask), which can be different than
> > > > s->num_channels.
> > > >
> > > > I think my suggestion above to use the extradata channel mask and
> > > > ignoring the user set channel count is the best approach for this.
> > >
> > > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220319/f7e86687/attachment.sig>
More information about the ffmpeg-devel
mailing list