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

[James Almer]

On 6/23/2025 12:14 PM, James Almer wrote:

On 6/23/2025 9:57 AM, Andreas Rheinhardt wrote:

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

words, your check is too late.

There is only one way for put_bits_left() to return a negative value: If 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

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?

The remaining bits in the GetBitContext buffer in order to fill the PutBitContext buffer.

The following probably fixes it, based on what you said.

diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c
index 71497876ac..19c549d4ac 100644
--- a/libavformat/iamf_parse.c
+++ b/libavformat/iamf_parse.c

         uint8_t buf[6];
         int size = FFMIN(codecpar->extradata_size, sizeof(buf));

-        init_put_bits(&pb, buf, size);
+        init_put_bits(&pb, buf, sizeof(buf));
         ret = init_get_bits8(&gb, codecpar->extradata, size);
         if (ret < 0)
             return ret;

         put_bits(&pb, ret, get_bits_long(&gb, ret));
         flush_put_bits(&pb);

+        if (get_bits_left(&gb) < 0)
+           return AVERROR_INVALIDDATA;
+
         memcpy(codecpar->extradata, buf, put_bytes_output(&pb));
         break;
     }

@@ -288,7 +288,7 @@ static int update_extradata(AVCodecParameters *codecpar) @@ -312,6 +312,9 @@ static int update_extradata(AVCodecParameters *codecpar)

Actually no, that broke FATE.

This maybe:

diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c
index 71497876ac..0bd4696b83 100644
--- a/libavformat/iamf_parse.c
+++ b/libavformat/iamf_parse.c
@@ -288,7 +288,7 @@ static int update_extradata(AVCodecParameters *codecpar)
         uint8_t buf[6];
         int size = FFMIN(codecpar->extradata_size, sizeof(buf));

-        init_put_bits(&pb, buf, size);
+        init_put_bits(&pb, buf, sizeof(buf));
         ret = init_get_bits8(&gb, codecpar->extradata, size);
         if (ret < 0)
             return ret;
@@ -304,7 +304,10 @@ 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);
+        ret = get_bits_left(&gb);
+        if (ret < 0)
+            return AVERROR_INVALIDDATA;
+        ret = FFMIN(ret, put_bits_left(&pb));
         while (ret >= 32) {
            put_bits32(&pb, get_bits_long(&gb, 32));
            ret -= 32;

OpenPGP_signature.asc
Description: OpenPGP digital signature

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".