From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 4585641A73 for ; Sat, 19 Mar 2022 17:52:44 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1DF3E68AF21; Sat, 19 Mar 2022 19:52:42 +0200 (EET) Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id D3CFD68A5C0 for ; Sat, 19 Mar 2022 19:52:35 +0200 (EET) Received: from localhost (213-47-68-29.cable.dynamic.surfer.at [213.47.68.29]) (Authenticated sender: michael@niedermayer.cc) by mail.gandi.net (Postfix) with ESMTPSA id D248CFF802 for ; Sat, 19 Mar 2022 17:52:34 +0000 (UTC) Date: Sat, 19 Mar 2022 18:52:34 +0100 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20220319175234.GB2829255@pb2> References: <20220317233019.12049-1-michael@niedermayer.cc> <20220317233019.12049-2-michael@niedermayer.cc> <74943f9e-b07a-7fe4-fa83-09a9cbda21a0@gmail.com> <20220318112758.GR2829255@pb2> MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH 2/3] avcodec/wmalosslessdec: Check channel mask against num channels X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: multipart/mixed; boundary="===============8546768172418243830==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============8546768172418243830== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="InmmQoCe1lQn0YGU" Content-Disposition: inline --InmmQoCe1lQn0YGU Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 18, 2022 at 08:38:28AM -0300, James Almer wrote: >=20 >=20 > On 3/18/2022 8:27 AM, Michael Niedermayer wrote: > > On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote: > > >=20 > > >=20 > > > On 3/17/2022 9:07 PM, James Almer wrote: > > > >=20 > > > >=20 > > > > 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 > > > > > >=20 > > > > > >=20 > > > > > > Found-by: continuous fuzzing process > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer > > > > > > --- > > > > > > =A0 libavcodec/wmalosslessdec.c | 3 +++ > > > > > > =A0 1 file changed, 3 insertions(+) > > > > > >=20 > > > > > > diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalossle= ssdec.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) > > > > > > =A0=A0=A0=A0=A0 av_channel_layout_uninit(&avctx->ch_layout); > > > > > > =A0=A0=A0=A0=A0 av_channel_layout_from_mask(&avctx->ch_layout,= channel_mask); > > > > > > +=A0=A0=A0 if (s->num_channels !=3D avctx->ch_layout.nb_channel= s) > > > > > > +=A0=A0=A0=A0=A0=A0=A0 return AVERROR_PATCHWELCOME; //are there= non fuzzed > > > > > > files with this or is it an error ? > > > > >=20 > > > > > s->num_channels at this point is set to the channels count the us= er > > > > > 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. > > > > >=20 > > > > > Also, can you reproduce this crash before 3c933af493? > > > > > s->num_channels was being set to the user set channel count too, > > > > > same as now. > > > >=20 > > > > 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 !=3D 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. > > > >=20 > > > > I think my suggestion above to use the extradata channel mask and > > > > ignoring the user set channel count is the best approach for this. > > >=20 > > > Like this maybe (channel_mask could in theory be zero, so in that cas= e the > > > user set value should be used). > > >=20 > > > > diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessde= c.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; > > > > } > > > >=20 > > > > - s->max_frame_size =3D MAX_FRAMESIZE * avctx->ch_layout.nb_chan= nels; > > > > - s->frame_data =3D av_mallocz(s->max_frame_size + AV_INPUT_BUFF= ER_PADDING_SIZE); > > > > - if (!s->frame_data) > > > > - return AVERROR(ENOMEM); > > > > - > > > > - s->avctx =3D avctx; > > > > - ff_llauddsp_init(&s->dsp); > > > > - init_put_bits(&s->pb, s->frame_data, s->max_frame_size); > > > > - > > > > if (avctx->extradata_size >=3D 18) { > > > > s->decode_flags =3D AV_RL16(edata_ptr + 14); > > > > channel_mask =3D AV_RL32(edata_ptr + 2); > > > > @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext = *avctx) > > > > return AVERROR_PATCHWELCOME; > > > > } > > > >=20 > > > > + if (channel_mask) { > > > > + av_channel_layout_uninit(&avctx->ch_layout); > > > > + av_channel_layout_from_mask(&avctx->ch_layout, channel_mas= k); > > > > + } else > > > > + avctx->ch_layout.order =3D AV_CHANNEL_ORDER_UNSPEC; > > > > + > > > > + s->num_channels =3D avctx->ch_layout.nb_channels; > > > > + > > > > + /* extract lfe channel position */ > > > > + s->lfe_channel =3D -1; > > > > + > > > > + if (channel_mask & 8) { > > > > + unsigned int mask; > > > > + for (mask =3D 1; mask < 16; mask <<=3D 1) > > > > + if (channel_mask & mask) > > > > + ++s->lfe_channel; > > > > + } > > > > + > > > > + s->max_frame_size =3D MAX_FRAMESIZE * avctx->ch_layout.nb_chan= nels; > > > > + s->frame_data =3D av_mallocz(s->max_frame_size + AV_INPUT_BUFF= ER_PADDING_SIZE); > > > > + if (!s->frame_data) > > > > + return AVERROR(ENOMEM); > > > > + > > > > + s->avctx =3D avctx; > > > > + ff_llauddsp_init(&s->dsp); > > > > + init_put_bits(&s->pb, s->frame_data, s->max_frame_size); > > > > + > > > > /* generic init */ > > > > s->log2_frame_size =3D av_log2(avctx->block_align) + 4; > > > >=20 > > > > @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext= *avctx) > > > > return AVERROR_INVALIDDATA; > > > > } > > > >=20 > > > > - s->num_channels =3D avctx->ch_layout.nb_channels; > > > > - > > > > - /* extract lfe channel position */ > > > > - s->lfe_channel =3D -1; > > > > - > > > > - if (channel_mask & 8) { > > > > - unsigned int mask; > > > > - for (mask =3D 1; mask < 16; mask <<=3D 1) > > > > - if (channel_mask & mask) > > > > - ++s->lfe_channel; > > > > - } > > > > - > > > > s->frame =3D av_frame_alloc(); > > > > if (!s->frame) > > > > return AVERROR(ENOMEM); > > > >=20 > > > > - av_channel_layout_uninit(&avctx->ch_layout); > > > > - av_channel_layout_from_mask(&avctx->ch_layout, channel_mask); > > > > return 0; > > > > } > >=20 > > this will change the output of the "lossless" decoder if this case ever > > occurs besides this >=20 > 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 [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato=20 --InmmQoCe1lQn0YGU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYjYYXgAKCRBhHseHBAsP qw87AJ9Mxogn943XiRiPaU9asuYd9Xg2VACeLdhNmnlW9wMvFSdXobvzFFKZiZU= =TvZb -----END PGP SIGNATURE----- --InmmQoCe1lQn0YGU-- --===============8546768172418243830== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ 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". --===============8546768172418243830==--