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 F2C5043577 for ; Thu, 16 Jun 2022 23:30:32 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 9D02F68B865; Fri, 17 Jun 2022 02:30:29 +0300 (EEST) Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 685BB68B7E9 for ; Fri, 17 Jun 2022 02:30:22 +0300 (EEST) 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 B2468240004 for ; Thu, 16 Jun 2022 23:30:21 +0000 (UTC) Date: Fri, 17 Jun 2022 01:30:20 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20220616233020.GR396728@pb2> References: <20220329212459.23440-1-michael@niedermayer.cc> <62c40964-d827-d044-cec3-233bfc04aaf1@gmail.com> <20220330105802.GO2829255@pb2> <20220410141413.GC2829255@pb2> <8b56d0cd-9f30-b4ab-7b18-4b2d6cfc5e11@gmail.com> MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH] avcodec/pcm_rechunk_bsf: unref packet before putting a new one in 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="===============6989344036775147441==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============6989344036775147441== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="k8tuI73zFNkWlsjw" Content-Disposition: inline --k8tuI73zFNkWlsjw Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 11, 2022 at 07:56:07PM +0200, Andreas Rheinhardt wrote: > James Almer: > >=20 > >=20 > > On 4/10/2022 11:14 AM, Michael Niedermayer wrote: > >> On Sat, Apr 09, 2022 at 08:56:05PM +0200, Marton Balint wrote: > >>> > >>> > >>> On Wed, 30 Mar 2022, Michael Niedermayer wrote: > >>> > >>>> On Tue, Mar 29, 2022 at 06:33:06PM -0300, James Almer wrote: > >>>>> On 3/29/2022 6:24 PM, Michael Niedermayer wrote: > >>>>>> Fixes: memleak > >>>>>> Fixes: > >>>>>> 45982/clusterfuzz-testcase-minimized-ffmpeg_BSF_PCM_RECHUNK_fuzzer= -5562089618407424 > >>>>>> > >>>>>> > >>>>>> Found-by: continuous fuzzing process > >>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>>>>> Signed-off-by: Michael Niedermayer > >>>>>> --- > >>>>>> =A0=A0 libavcodec/pcm_rechunk_bsf.c | 1 + > >>>>>> =A0=A0 1 file changed, 1 insertion(+) > >>>>>> > >>>>>> diff --git a/libavcodec/pcm_rechunk_bsf.c > >>>>>> b/libavcodec/pcm_rechunk_bsf.c > >>>>>> index 108d9e90b9..3f43934fe9 100644 > >>>>>> --- a/libavcodec/pcm_rechunk_bsf.c > >>>>>> +++ b/libavcodec/pcm_rechunk_bsf.c > >>>>>> @@ -153,6 +153,7 @@ static int rechunk_filter(AVBSFContext *ctx, > >>>>>> AVPacket *pkt) > >>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 } > >>>>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 } > >>>>>> +=A0=A0=A0=A0=A0=A0=A0 av_packet_unref(s->in_pkt); > >>>>> > >>>>> This looks to me like it revealed a bug in the code above, which is > >>>>> meant to > >>>>> ensure s->in_pkt will be blank at this point. It should be fixed th= ere > >>>>> instead. > >>>> > >>>> IIRC the problem was a input packet with size 0 > >>>> the code seems to assume 0 meaning no packet > >>> > >>> Is that valid here? The docs says that the encoders can generate 0 si= zed > >>> packets if there is side data in them. However - the PCM rechunk BSF > >>> using > >>> PCM packets - I am not sure this is intentional here. > >> > >> where exactly is this written ? > >> > >> > >>> > >>> So overall it looks to me that the PCM rechunk BSF should reject 0 si= zed > >>> packets with AVERROR_INVALIDDATA, and the encoder or demuxer which > >>> produces > >>> the 0 sized packets should be fixed. > >> > >> There is no encoder or demuxer. There is just the fuzzer which excerci= es > >> the whole space of allowed parameters of the BSFs > >> and either such zero packets are valid or they are not. > >> if not, then a check could be added to av_bsf_send_packet() that feels= a > >> bit broad though. > >> > >> i can add a check to pcm_rechunk_bsf but it feels a bit odd if these a= re > >> valid and just not supposed to come out of the encoders > >> > >> do you see some problem with these packets ? > >> that makes it better to just reject them ? > >> > >> (error you enountered a packet which makes no difference seems a bit o= dd > >> =A0 in its own too. That probably should only be a warning) > >> =A0 thx > >> > >> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c > >> index 42cc1b5ab0..ae16112285 100644 > >> --- a/libavcodec/bsf.c > >> +++ b/libavcodec/bsf.c > >> @@ -212,6 +212,11 @@ int av_bsf_send_packet(AVBSFContext *ctx, > >> AVPacket *pkt) > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 return 0; > >> =A0=A0=A0=A0=A0 } > >> =A0 +=A0=A0=A0 if (pkt->size =3D=3D 0 && pkt->side_data_elems =3D=3D 0= ) { > >> +=A0=A0=A0=A0=A0=A0=A0 av_log(ctx, AV_LOG_ERROR, "Zero packet is not a= llowed.\n"); > >> +=A0=A0=A0=A0=A0=A0=A0 return AVERROR(EINVAL); > >> +=A0=A0=A0 } > >=20 > > To make this behave like avcodec_send_packet(), it should instead be > >=20 > > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c > > index 42cc1b5ab0..01ed9db258 100644 > > --- a/libavcodec/bsf.c > > +++ b/libavcodec/bsf.c > > @@ -205,6 +205,9 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket > > *pkt) > > =A0=A0=A0=A0 FFBSFContext *const bsfi =3D ffbsfcontext(ctx); > > =A0=A0=A0=A0 int ret; > >=20 > > +=A0=A0=A0 if (pkt && !pkt->size && pkt->data) > > +=A0=A0=A0=A0=A0=A0=A0 return AVERROR(EINVAL); > > + > > =A0=A0=A0=A0 if (!pkt || IS_EMPTY(pkt)) { > > =A0=A0=A0=A0=A0=A0=A0=A0 if (pkt) > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 av_packet_unref(pkt); >=20 > 1. None of your patches would actually fix the memleak: Packets with > data =3D=3D NULL, size =3D=3D 0 and side_data_elems !=3D 0 would still sl= ip > through and cause leaks (of the side data). > 2. I don't see why the behaviour of avcodec_send_packet should be the > model to emulate here. (E.g. av_packet_make_refcounted* creates packets > with size =3D=3D 0 and data set (pointing to a buffer of size > AV_INPUT_BUFFER_PADDING_SIZE) when the packet has no data.) > 3. avcodec_send_packet ignores its own documentation: Packets with data > =3D=3D NULL, size =3D=3D 0, but side_data_elems !=3D 0 are not considered > eof/flush packets. The reason for this is that avcodec_send_packet > offloads the flushing-decision to the underlying BSF and the BSF API has > slightly different semantics. > IMO we should only accept NULL packets/frames as flush packets as this > is the only thing that is always guaranteed to be an intentional flush > packet; any more additions to AVPacket might otherwise force us to > redefine what a flush packet is. > 4. The behaviour of avcodec_send_packet has a weird consequence: Say you > get new extradata via an out-of-band mechanism and want to send it to > the decoder via packet side-data. Given that it is an out-of-band > mechanism you don't have an ordinary packet yet to attach it, so you > simply send it via a packet with size 0. But given that packets with > data =3D=3D NULL and size =3D=3D 0 are documented to be flush packets (th= ey > aren't in practice) and flushing is not intended here, you use a packet > with size =3D=3D 0 and data !=3D NULL. And then avcodec_send_packet errors > out, forcing you to abandon this approach and attach the side-data to > the next real packet (which is sligtly less natural).** What exact check do you suggest ? (iam asking as i stumbled across this bug again and so got reminded) thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Modern terrorism, a quick summary: Need oil, start war with country that has oil, kill hundread thousand in war. Let country fall into chaos, be surprised about raise of fundamantalists. Drop more bombs, kill more people, be surprised about them taking revenge and drop even more bombs and strip your own citizens of their rights and freedoms. to be continued --k8tuI73zFNkWlsjw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYqu9CAAKCRBhHseHBAsP q5NZAKCVipmOIk3EZgSMHXgpEJq07+3TpQCeMiXBzqwSl+pmXFNWmQwWiuofmr4= =/XY2 -----END PGP SIGNATURE----- --k8tuI73zFNkWlsjw-- --===============6989344036775147441== 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". --===============6989344036775147441==--