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 06F9D4B231 for ; Mon, 1 Jul 2024 23:42:18 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 4205C68C5EC; Tue, 2 Jul 2024 02:42:15 +0300 (EEST) Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4F9BA68C5EC for ; Tue, 2 Jul 2024 02:42:07 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id 4DF2CC0002 for ; Mon, 1 Jul 2024 23:42:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1719877326; 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=tKGbQg0LcKoi6jwbshVgryeEKw0/IyvweOVsbYPmPlQ=; b=EZdBd1iSoma/9bfJYx+jCl3SE4h9mfYstsXHj3BOhQC4KcYSEVSlikWHhwI47I4n/RxvY6 s9uwxEAZ07plydsP/Gj95DPWv6Z0BgwNenkT15uyoM6gQ9kBNKdDD2YkgJoJFfJSKyNh64 zeW5fUc6yBToY7RsTYZSk1QUAHF1UnEXxmWxkaIqk2Q0cR0ZauUEBUIHsQ1yws3P6lvgja /tNhLUBAPzXMQsqM3RM3cIs7P/YXjaDbu7zaaoo5A8Xp2mRqmMrbU1Jih7sZi2Gts+80BD I//wCpqvHXeJgUfjsZi8Q58UNBFXHufAcRNHgaleJ+G6dikKFUXOdEZGivHL8Q== Date: Tue, 2 Jul 2024 01:42:05 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240701234205.GL4991@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> MIME-Version: 1.0 In-Reply-To: <527948e0-a335-4ad8-87fa-0a3443b32203@gmail.com> 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="===============3919724443275126961==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============3919724443275126961== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="zvf7DYrKnARJlhgd" Content-Disposition: inline --zvf7DYrKnARJlhgd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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_fuzzer-5= 791144363491328 > > > >=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_CHANNELS = || > > > > + st->codecpar->bits_per_coded_sample > 64) > > >=20 > > > Where does the process take so long that oss-fuzz gets a timeout if t= hese > > > are unreasonably high? I don't see nb_channels used anywhere in here = 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_C= HANNELS > > > 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 AVChann= elLayout *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 plac= e, > so something else would have to be done. This makes the testcase which is 108 bytes long take a bit more than 7 seco= nds which is below the threshold for the timeout, but i would guess 30x on the = channel number would bring it well above The next location it gets stuck on if the timeout threshold is reduced: #0 0x4a5b41 in __sanitizer_print_stack_trace /b/swarming/w/ir/cache/bui= lder/src/third_party/llvm/compiler-rt/lib/asan/asan_stack.cc:86:3 #1 0x2a30aae in fuzzer::Fuzzer::AlarmCallback() Fuzzer/build/../FuzzerL= oop.cpp:248:7 #2 0x7fbe0815e41f (/lib/x86_64-linux-gnu/libpthread.so.0+0x1441f) #3 0x7fbe07c73e08 (/lib/x86_64-linux-gnu/libc.so.6+0xbbe08) #4 0x49c747 in __asan_memcpy /b/swarming/w/ir/cache/builder/src/third_p= arty/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:22:3 #5 0x2873f59 in av_channel_layout_copy ffmpeg/libavutil/channel_layout.= c:452:9 #6 0x1895763 in avcodec_parameters_to_context ffmpeg/libavcodec/codec_p= ar.c:235:15 #7 0x71923e in avformat_find_stream_info ffmpeg/libavformat/demux.c:257= 9:15 #8 0x4cd17b in LLVMFuzzerTestOneInput ffmpeg/tools/target_dem_fuzzer.c:= 207:11 #9 0x2a31b1d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, u= nsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 #10 0x2a266f2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsig= ned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 #11 0x2a2b8f1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned c= har const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 #12 0x2a263d0 in main Fuzzer/build/../FuzzerMain.cpp:20:10 #13 0x7fbe07bdc082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.= 6+0x24082) #14 0x42529d in _start (ffmpeg/tools/target_dem_caf_fuzzer+0x42529d) >=20 > >=20 > >=20 > > >=20 > > > And is the bits_per_coded_sample > 64 check to prevent codec_id being > > > AV_CODEC_ID_NONE? if so, how does that affect demuxing time? > > > AV_CODEC_ID_NONE for that matter could happen for valid files with a = codec > > > we don't currently support. > >=20 > > We generally check read values for validity at the earliest, > > bits_per_coded_sample of more than 64 seem not valid to me. >=20 > I agree the check is fine, but my point is that, assuming this is affecti= ng > demuxing time, this check as is may be hiding an issue related to codec_id > being set to AV_CODEC_ID_NONE here (the result of ff_get_pcm_codec_id() w= ith > an unsupported bits_per_coded_sample value), so it should be looked at mo= re > closely because said codec_id could happen with valid files using codecs = the > demuxer does not know about. >=20 > If it does not affect demuxing time and is a "just in case" check, then it > doesn't belong in a patch that says "Fixes: Timeout" and mentions an ossf= uzz > issue. that is strictly true thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Any man who breaks a law that conscience tells him is unjust and willingly= =20 accepts the penalty by staying in jail in order to arouse the conscience of= =20 the community on the injustice of the law is at that moment expressing the= =20 very highest respect for law. - Martin Luther King Jr --zvf7DYrKnARJlhgd Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZoM+yQAKCRBhHseHBAsP q1yAAJ9qBP5YHyfCY7CKw5zaaKOg811fcQCfb6Q7PmAYyA4uNkKasv4NVsL+Xqs= =ZDCj -----END PGP SIGNATURE----- --zvf7DYrKnARJlhgd-- --===============3919724443275126961== 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". --===============3919724443275126961==--