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 9D0764A2A0 for ; Tue, 2 Jul 2024 10:50:32 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id B000168D8C7; Tue, 2 Jul 2024 13:50:30 +0300 (EEST) Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 95DEA68D8A9 for ; Tue, 2 Jul 2024 13:50:23 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id 9726E1C0012 for ; Tue, 2 Jul 2024 10:50:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1719917422; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Wo/ulFJx9rlnCh0UWWCwlhtDGJ/rq9g8vSiCaCBEUQ8=; b=d23b/DU9jpwfMwosLmvsDAxhZ4i69kTowFmZoFpYdIMXgbdxeDpwwLogUuWWXAujHCyHXw Q1gYG3HgzYt6kRbqtn48W+Vh1sGfsRKVEF8Z5QXd1uC/LuuY+hzKDOFozopw5A+OcrFSmm /l9444338b48nf5gVN6WskhSp43KchBjwp+LVDBytDtsrjdfxbX6FhQEl6+d1oWwACSx9K 3yvTZmLrNyoU5fyG6RssHBWe0UqR7qG/ZnBktRMPiJxhyY/Mj59bgmyIu7EouGLe7e/a7n RYRRc9A+dGavNB7Ugf/U02oO6Uns5ifTV4F/EY6R+GcQ1pIoqBXQAyQhDrn7Yg== Date: Tue, 2 Jul 2024 12:50:21 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240702105021.GN4991@pb2> References: <20240322230818.18997-1-michael@niedermayer.cc> <40908cf3-7e3a-4a57-a23e-43bf153c20bd@gmail.com> <20240629233705.GB4991@pb2> <527948e0-a335-4ad8-87fa-0a3443b32203@gmail.com> <20240701234205.GL4991@pb2> MIME-Version: 1.0 In-Reply-To: X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps 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="===============3358000149593693488==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============3358000149593693488== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="wTzoGrkn2AhIv4sC" Content-Disposition: inline --wTzoGrkn2AhIv4sC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jul 01, 2024 at 09:01:28PM -0300, James Almer wrote: > On 7/1/2024 8:42 PM, Michael Niedermayer wrote: > > On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote: > > > On 6/29/2024 8:37 PM, Michael Niedermayer wrote: > > > > On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote: > > > > > On 3/22/2024 8:08 PM, Michael Niedermayer wrote: > > > > > > Fixes: Timeout > > > > > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzz= er-5791144363491328 > > > > > >=20 > > > > > > Found-by: continuous fuzzing process https://github.com/google/= oss-fuzz/tree/master/projects/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer > > > > > > --- > > > > > > libavformat/cafdec.c | 5 +++++ > > > > > > 1 file changed, 5 insertions(+) > > > > > >=20 > > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c > > > > > > index 426c56b9bd..334077efb5 100644 > > > > > > --- a/libavformat/cafdec.c > > > > > > +++ b/libavformat/cafdec.c > > > > > > @@ -33,6 +33,7 @@ > > > > > > #include "isom.h" > > > > > > #include "mov_chan.h" > > > > > > #include "libavcodec/flac.h" > > > > > > +#include "libavcodec/internal.h" > > > > > > #include "libavutil/intreadwrite.h" > > > > > > #include "libavutil/intfloat.h" > > > > > > #include "libavutil/dict.h" > > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *= s) > > > > > > st->codecpar->ch_layout.nb_channels =3D avio_rb32(pb); > > > > > > st->codecpar->bits_per_coded_sample =3D avio_rb32(pb); > > > > > > + if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANN= ELS || > > > > > > + st->codecpar->bits_per_coded_sample > 64) > > > > >=20 > > > > > Where does the process take so long that oss-fuzz gets a timeout = if these > > > > > are unreasonably high? I don't see nb_channels used anywhere in h= ere where > > > > > that matters. > > > > > Is it in the PCM decoder? Because that decoder is meant to handle= any > > > > > arbitrary amount of channels, so limiting it to whatever FF_SANE_= NB_CHANNELS > > > > > is set to is not ok. > > > >=20 > > > > libavutil/channel_layout.c:627:17 > > > > or it will OOM before depending on how much memory is available > > >=20 > > > Does this fix the timeout? > > >=20 > > > > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c > > > > index 2d6963b6df..a4fcd199e1 100644 > > > > --- a/libavutil/channel_layout.c > > > > +++ b/libavutil/channel_layout.c > > > > @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVC= hannelLayout *channel_layout, > > > > for (i =3D 0; i < channel_layout->nb_channels; i++) { > > > > enum AVChannel ch =3D av_channel_layout_channel_from_= index(channel_layout, i); > > > >=20 > > > > + if (!av_bprint_is_complete(bp)) > > > > + break; > > > > if (i) > > > > av_bprintf(bp, "+"); > > > > av_channel_name_bprint(bp, ch); > > >=20 > > > But this is not ok as it will affect the buffer len value > > > av_channel_layout_describe() returns on success when truncation took = place, > > > so something else would have to be done. > >=20 > > This makes the testcase which is 108 bytes long take a bit more than 7 = seconds > > which is below the threshold for the timeout, but i would guess 30x on = the channel > > number would bring it well above >=20 > If you try to process that file without the fuzzer, does it also take 7 > seconds before it stops? real 0m3.232s user 0m1.064s sys 0m2.156s for a 108 byte file with your speed up patch, without that patch i didnt tr= y as i have not enough patience ATM with 66 instead of 33 million channels real 0m6.181s user 0m1.774s sys 0m4.385s > If not, then the fuzzer is having Valgrind levels > of penalty hit, and i don't think adding dubious checks in our codebase j= ust > to appease it is correct. Please think again The fuzzer is slower than non instrumented code, but its purpose is to dete= ct issues First i dont see how that leads to "dubious checks" or "appease" these are = loaded words not appropriate for a technical discusison Second, if the fuzzer spends the majority of its time in a busy loop copyin= g millions of channels. Thats time it will not find any issues. So both the fuzzing process itself as well as DOS of users are affected Third if it goes over its threshold, for its purpose it found an issue. If thats not the case the fuzzers configuration does not match the definiti= on of a DOS issue. Thats a problem in itself. (this isnt applying as the 7 sec is b= elow) Fourth, we should try to investigate and fix issues (truth we often dont ha= ve the resources to fully investigate everything). Here the issue is clearly 33 mi= llion channels being worked on as valid data. It would take time, alot of time to= process each sample times 33 million. The user does not have 33 million speakers and its= not reasonable for her computer to have to process files just to later reject t= hem. She should have a way to reject this earlier without the processing. This s= ample also is no worst case at all. We can apply your speedup and wait for the fu= zzer to find a worse one, this may take time or might never happen. This doesnt mean the worst case is 6 seconds for a 108 byte file. That said a web brows= er opening 100 media files each 108 bytes and consuming 6 seconds cpu time is already quite bad, also the memory requirement is bad too. This is a ran= dom example maybe its not possible exactly like that, its more to show this iss= ue 100 108 byte files is 10kb 10minutes of cpu time is alot for that, maybe a = real exploit would use a differnt demuxer or whatever. The issue iam having is t= hat Adding a check on channels is blocked on arguments that a specific testcase= isnt bad enough NOT on arguments that the WORST case would not be bad enough. Thats like saying going 1 byte over the array but still being in an allocat= ed struct is not so bad disregarding what the maximum would be. thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope --wTzoGrkn2AhIv4sC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZoPbZwAKCRBhHseHBAsP q7ZTAJ4+5Ko5LbB4cqb0OcJY4hjCnmCn+gCePeWC0P16XJaSmJTDl8h4S0oC5Xo= =97n1 -----END PGP SIGNATURE----- --wTzoGrkn2AhIv4sC-- --===============3358000149593693488== 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". --===============3358000149593693488==--