Re: [FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps

[James Almer <jamrial@gmail.com>]

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@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@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".