[FFmpeg-devel] [PATCH 2/6] avcodec/cbs: Do not assert on traces beyond 255 bits

[Mark Thompson]

On 23/10/2023 21:53, Michael Niedermayer wrote:
> On Sun, Oct 22, 2023 at 03:34:20PM +0100, Mark Thompson wrote:
>> On 22/10/2023 01:35, Michael Niedermayer wrote:
>>> Fixes: Assertion length < 256 failed at libavcodec/cbs.c:517
>>> Fixes: 62673/clusterfuzz-testcase-minimized-ffmpeg_BSF_TRACE_HEADERS_fuzzer-6490971837431808
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>    libavcodec/cbs.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
>>> index cdd7adebebd..2f5d0334a2a 100644
>>> --- a/libavcodec/cbs.c
>>> +++ b/libavcodec/cbs.c
>>> @@ -514,6 +514,11 @@ void ff_cbs_trace_read_log(void *trace_context,
>>>        position = get_bits_count(gbc);
>>> +    if (length >= 256) {
>>> +        av_log(ctx->log_ctx, ctx->trace_level, "trace of %d bits truncated at 255\n", length);
>>> +        length = 255;
>>> +    }
>>> +
>>>        av_assert0(length < 256);
>>>        for (i = 0; i < length; i++)
>>>            bits[i] = get_bits1(gbc) ? '1' : '0';
>>
>> IMO the assert is sensible (no syntax element is that large) and so this must be catching a bug somewhere else.  Please don't nullify the assert to hide the bug.
>>
>> Can you share the input stream which hit this case?
> 
> will mail it to you
> 
> the backtrce is this:
> 
>      #7 0x505748 in ff_cbs_trace_read_log ffmpeg/libavcodec/cbs.c:517:5
>      #8 0x5273f1 in cbs_av1_read_uvlc ffmpeg/libavcodec/cbs_av1.c:67:5
>      #9 0x5273f1 in cbs_av1_read_timing_info ffmpeg/libavcodec/cbs_av1_syntax_template.c:168
>      #10 0x5273f1 in cbs_av1_read_sequence_header_obu ffmpeg/libavcodec/cbs_av1_syntax_template.c:214
>      #11 0x51278a in cbs_av1_read_unit ffmpeg/libavcodec/cbs_av1.c:856:19
>      #12 0x4ff30a in cbs_read_fragment_content ffmpeg/libavcodec/cbs.c:209:15
>      #13 0x4ff30a in cbs_read_data ffmpeg/libavcodec/cbs.c:276
>      #14 0x4edc32 in trace_headers ffmpeg/libavcodec/trace_headers_bsf.c:113:11
>      #15 0x4c9898 in LLVMFuzzerTestOneInput ffmpeg/tools/target_bsf_fuzzer.c:154:16
>      #16 0x136900d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
>      #17 0x135dbe2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6
>      #18 0x1362de1 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
>      #19 0x135d8c0 in main Fuzzer/build/../FuzzerMain.cpp:20:10
>      #20 0x7f456b8b8c86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310
>      #21 0x41f179 in _start (ffmpeg/tools/target_bsf_trace_headers_fuzzer+0x41f179)

This is the case in <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-October/315983.html>, and would be fixed by that patch.

Since the problem is a dubious feature of the standard which other implementations then do not follow I would appreciate thoughts from other people on what to do with it, though.

Thanks,

- Mark