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 5228A4A48C for ; Tue, 2 Jul 2024 00:01:12 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 5BA2968D85B; Tue, 2 Jul 2024 03:01:10 +0300 (EEST) Received: from mail-pl1-f170.google.com (mail-pl1-f170.google.com [209.85.214.170]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 97BD768D60B for ; Tue, 2 Jul 2024 03:01:03 +0300 (EEST) Received: by mail-pl1-f170.google.com with SMTP id d9443c01a7336-1f9cd92b146so23442935ad.3 for ; Mon, 01 Jul 2024 17:01:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1719878460; x=1720483260; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=2zOGFYGIgVVPoSruxX2jkry0QnC4yqrLgI0zSXRimZs=; b=H4DbugTZ3Pp1nG9OhfaIWxIs7tQjNYGK3s3tQiKk9iKWolK0K0H6OAYMMSelLx9+eR XJ0w4Zapf5Fk/+6yZaKEofIYsyxq7U9zSmSourJ8+Aem37Cx57tvXitsV+ijVtQtdd8H inSe//jkdjXnePJZd8q4owW0R9eKGkti+RMD9CRiGb9FHZp73aeuvqVJi14/PYYLB5dh HXUURvmugZLfPnPlnsr+Nq0mEqCfqDzZPIkIdnt6xCQhiwlpqm5ueVOI4pd2aydF3LM/ hwbXIcF9cJ1stpl+uey81Ie/tNel43YXUc+63HEhVNJ4VSK34DRjtxRwmFF5yImGlDBG CsFw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719878460; x=1720483260; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=2zOGFYGIgVVPoSruxX2jkry0QnC4yqrLgI0zSXRimZs=; b=e0ppUbcvHZQ9nu+LklnRmkzrzQIWfd+0plzGN6+5tosaLwPWSfd4UB3w+Fo5DjDt+O nO446YFhXlpAVIX1gIQF4txXJw/vbcAuFk1W1rSW0HDmM2CK9KSfKb7VgydAZH0rF9Xi SZWXUIv/wjXtdcdCKT/PQmNTGrdHQahozCC/Pe7nQi9XRtJlqeBcP4MCV+kWDXfa9+Wz 5WHVbaaX/I5HiC0Cr6Rcy514KxIbMQTkaAG24WgDFhXEk2qLZrYnzu9ARlJc8vPQMtnX pnDqAi8LMNQshj9WwAMIqSmn2KUuBNqEnPzEz3sj5gbMniar/wUggpSP1y+wH73GJQgY 6wQA== X-Gm-Message-State: AOJu0YzZggC3hAoJwxuPF73ZRriu9mUQi4n6RNScY43J8pFlRrq+QMcz cfMAFd2kFUR3gNqdfuBoPSgkGBb8eyMeZsSsfEOug8l9HnGIcBfow7IsMg== X-Google-Smtp-Source: AGHT+IGhGsJSZi3PaJnXr6lmNE9HRUS+hc0LE+PWg/W/6NTHxwxlIvP+KLxZ2d4Ly7dvYleTcferzA== X-Received: by 2002:a17:90a:7d0e:b0:2c2:df58:bb8c with SMTP id 98e67ed59e1d1-2c93d6ff1cfmr5742873a91.18.1719878459801; Mon, 01 Jul 2024 17:00:59 -0700 (PDT) Received: from [192.168.0.16] ([190.194.167.233]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2c91ce4526esm7402723a91.20.2024.07.01.17.00.58 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 01 Jul 2024 17:00:59 -0700 (PDT) Message-ID: Date: Mon, 1 Jul 2024 21:01:28 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: ffmpeg-devel@ffmpeg.org 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> Content-Language: en-US From: James Almer In-Reply-To: <20240701234205.GL4991@pb2> 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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_fuzzer-5791144363491328 >>>>> >>>>> 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(+) >>>>> >>>>> 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 = avio_rb32(pb); >>>>> st->codecpar->bits_per_coded_sample = avio_rb32(pb); >>>>> + if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS || >>>>> + st->codecpar->bits_per_coded_sample > 64) >>>> >>>> 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 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_CHANNELS >>>> is set to is not ok. >>> >>> libavutil/channel_layout.c:627:17 >>> or it will OOM before depending on how much memory is available >> >> Does this fix the timeout? >> >>> 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 AVChannelLayout *channel_layout, >>> for (i = 0; i < channel_layout->nb_channels; i++) { >>> enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i); >>> >>> + if (!av_bprint_is_complete(bp)) >>> + break; >>> if (i) >>> av_bprintf(bp, "+"); >>> av_channel_name_bprint(bp, ch); >> >> 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. > > 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 If you try to process that file without the fuzzer, does it also take 7 seconds before it stops? If not, then the fuzzer is having Valgrind levels of penalty hit, and i don't think adding dubious checks in our codebase just to appease it is correct. > > 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/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_stack.cc:86:3 > #1 0x2a30aae in fuzzer::Fuzzer::AlarmCallback() Fuzzer/build/../FuzzerLoop.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_party/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_par.c:235:15 > #7 0x71923e in avformat_find_stream_info ffmpeg/libavformat/demux.c:2579:15 > #8 0x4cd17b in LLVMFuzzerTestOneInput ffmpeg/tools/target_dem_fuzzer.c:207:11 > #9 0x2a31b1d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > #10 0x2a266f2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > #11 0x2a2b8f1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char 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) > > >> >>> >>> >>>> >>>> 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. >>> >>> We generally check read values for validity at the earliest, >>> bits_per_coded_sample of more than 64 seem not valid to me. >> >> I agree the check is fine, but my point is that, assuming this is affecting >> 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() with >> an unsupported bits_per_coded_sample value), so it should be looked at more >> closely because said codec_id could happen with valid files using codecs the >> demuxer does not know about. >> > >> 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 ossfuzz >> issue. > > that is strictly true > > thx > > [...] > > > _______________________________________________ > 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". _______________________________________________ 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".