[FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps
James Almer
jamrial at gmail.com
Tue Jul 2 03:01:28 EEST 2024
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 <michael at niedermayer.cc>
>>>>> ---
>>>>> 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list