Re: [FFmpeg-devel] [PATCH 1/4] avformat/iamf_parse: Check extradata size

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

James Almer:
> On 6/23/2025 9:44 AM, Michael Niedermayer wrote:
>> On Fri, Jun 20, 2025 at 12:28:13AM +0200, Andreas Rheinhardt wrote:
>>> Michael Niedermayer:
>>>> Fixes: Assertion n>=0 && n<=32 failed at ./libavcodec/get_bits.h:406
>>>> Fixes: 398527871/clusterfuzz-testcase-minimized-
>>>> ffmpeg_dem_IAMF_fuzzer-6602025714647040
>>>>
>>>> Found-by: continuous fuzzing process https://github.com/google/oss-
>>>> fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>>   libavformat/iamf_parse.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c
>>>> index 71497876ac3..330e01733dd 100644
>>>> --- a/libavformat/iamf_parse.c
>>>> +++ b/libavformat/iamf_parse.c
>>>> @@ -305,6 +305,8 @@ static int update_extradata(AVCodecParameters
>>>> *codecpar)
>>>>           skip_bits(&gb, 4);
>>>>           put_bits(&pb, 4, codecpar->ch_layout.nb_channels); // set
>>>> channel config
>>>>           ret = put_bits_left(&pb);
>>>> +        if (ret < 0)
>>>> +            return AVERROR_INVALIDDATA;
>>>>           while (ret >= 32) {
>>>>              put_bits32(&pb, get_bits_long(&gb, 32));
>>>>              ret -= 32;
>>>
>>> There is only one way for put_bits_left() to return a negative value: If
>>> there is more data in the internal buffer than can be written out. And
>>> this scenario is already a violation of the PutBit API. Given that the
>>> size of the internal buffer depends upon the arch, it could be that one
>>> would have already hit an assert in case one is not using x64. In other
>>> words, your check is too late.
>>
>> the patches puprose was mainly to show that
>> 3f9420132441345b7ccd57001f230bb98f655696
>> was insufficient to fix 398527871
>>
>> I do not expect my patch would be the correct solution even if the
>> check is done earlier. IAMF is cursed
> 
> Does increasing buf from 6 bytes to 8 or more fix it? I see putbits may
> do an AV_W*64(), so six bytes sounds like it was never safe.
> 
That is only executed when the internal bit buffer is full; you will
never reach it on x64. The problem is that you initialize the put bits
buffer with FFMIN(codecpar->extradata_size, sizeof(buf)) instead of
sizeof(buf). If this were not so, there would always be bits left.
But this only fixes the API violations, it does not guarantee that the
written data is actually correct. What is actually in the data that gets
written in the loop?

- Andreas

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