diff mbox series

[FFmpeg-devel,2/2] avcodec/cbs_vp9: Try to store fewer than 2 things in the same bit

Message ID 20240807235333.2148870-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/cfhdenc: Clear dwt_tmp | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished

Commit Message

Michael Niedermayer Aug. 7, 2024, 11:53 p.m. UTC
Fixes: use of uninitialized value
Fixes: 70907/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_METADATA_fuzzer-6339363208757248

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/cbs_vp9.c                 | 9 ++++++---
 libavcodec/cbs_vp9.h                 | 3 +++
 libavcodec/cbs_vp9_syntax_template.c | 6 +++---
 3 files changed, 12 insertions(+), 6 deletions(-)

Comments

Mark Thompson Aug. 11, 2024, 6:05 p.m. UTC | #1
On 08/08/2024 00:53, Michael Niedermayer wrote:
> Fixes: use of uninitialized value
> Fixes: 70907/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_METADATA_fuzzer-6339363208757248
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/cbs_vp9.c                 | 9 ++++++---
>  libavcodec/cbs_vp9.h                 | 3 +++
>  libavcodec/cbs_vp9_syntax_template.c | 6 +++---
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c
> index 816d06da04d..7b8bc9c7985 100644
> --- a/libavcodec/cbs_vp9.c
> +++ b/libavcodec/cbs_vp9.c
> @@ -541,9 +541,12 @@ static int cbs_vp9_assemble_fragment(CodedBitstreamContext *ctx,
>              size_len = av_log2(max) / 8 + 1;
>          av_assert0(size_len <= 4);
>  
> -        sfi.superframe_marker            = VP9_SUPERFRAME_MARKER;
> -        sfi.bytes_per_framesize_minus_1  = size_len - 1;
> -        sfi.frames_in_superframe_minus_1 = frag->nb_units - 1;
> +        sfi.superframe_marker            =
> +        sfi.superframe_marker_2          = VP9_SUPERFRAME_MARKER;
> +        sfi.bytes_per_framesize_minus_1  =
> +        sfi.bytes_per_framesize_minus_1_2= size_len - 1;
> +        sfi.frames_in_superframe_minus_1   =
> +        sfi.frames_in_superframe_minus_1_2 = frag->nb_units - 1;
>  
>          size = 2;
>          for (i = 0; i < frag->nb_units; i++) {
> diff --git a/libavcodec/cbs_vp9.h b/libavcodec/cbs_vp9.h
> index af15eb4bace..428662a8cdb 100644
> --- a/libavcodec/cbs_vp9.h
> +++ b/libavcodec/cbs_vp9.h
> @@ -174,6 +174,9 @@ typedef struct VP9RawSuperframeIndex {
>      uint8_t bytes_per_framesize_minus_1;
>      uint8_t frames_in_superframe_minus_1;
>      uint32_t frame_sizes[VP9_MAX_FRAMES_IN_SUPERFRAME];
> +    uint8_t superframe_marker_2;
> +    uint8_t bytes_per_framesize_minus_1_2;
> +    uint8_t frames_in_superframe_minus_1_2;
>  } VP9RawSuperframeIndex;
>  
>  typedef struct VP9RawSuperframe {
> diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
> index 2f08eccf180..3f542d0c5d5 100644
> --- a/libavcodec/cbs_vp9_syntax_template.c
> +++ b/libavcodec/cbs_vp9_syntax_template.c
> @@ -421,9 +421,9 @@ static int FUNC(superframe_index)(CodedBitstreamContext *ctx, RWContext *rw,
>              frame_sizes[i], 1, i);
>      }
>  
> -    f(3, superframe_marker);
> -    f(2, bytes_per_framesize_minus_1);
> -    f(3, frames_in_superframe_minus_1);
> +    f(3, superframe_marker_2);
> +    f(2, bytes_per_framesize_minus_1_2);
> +    f(3, frames_in_superframe_minus_1_2);

This gets the syntax element names wrong in traces.

>  
>      return 0;
>  }

The two instances of the syntax elements must be identical, if they aren't then the file is invalid.

The correct fix therefore would be to constrain the second read values to be identical to the first, not to introduce new syntax elements not in the standard to cover the invalid case.

Thanks,

- Mark
diff mbox series

Patch

diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c
index 816d06da04d..7b8bc9c7985 100644
--- a/libavcodec/cbs_vp9.c
+++ b/libavcodec/cbs_vp9.c
@@ -541,9 +541,12 @@  static int cbs_vp9_assemble_fragment(CodedBitstreamContext *ctx,
             size_len = av_log2(max) / 8 + 1;
         av_assert0(size_len <= 4);
 
-        sfi.superframe_marker            = VP9_SUPERFRAME_MARKER;
-        sfi.bytes_per_framesize_minus_1  = size_len - 1;
-        sfi.frames_in_superframe_minus_1 = frag->nb_units - 1;
+        sfi.superframe_marker            =
+        sfi.superframe_marker_2          = VP9_SUPERFRAME_MARKER;
+        sfi.bytes_per_framesize_minus_1  =
+        sfi.bytes_per_framesize_minus_1_2= size_len - 1;
+        sfi.frames_in_superframe_minus_1   =
+        sfi.frames_in_superframe_minus_1_2 = frag->nb_units - 1;
 
         size = 2;
         for (i = 0; i < frag->nb_units; i++) {
diff --git a/libavcodec/cbs_vp9.h b/libavcodec/cbs_vp9.h
index af15eb4bace..428662a8cdb 100644
--- a/libavcodec/cbs_vp9.h
+++ b/libavcodec/cbs_vp9.h
@@ -174,6 +174,9 @@  typedef struct VP9RawSuperframeIndex {
     uint8_t bytes_per_framesize_minus_1;
     uint8_t frames_in_superframe_minus_1;
     uint32_t frame_sizes[VP9_MAX_FRAMES_IN_SUPERFRAME];
+    uint8_t superframe_marker_2;
+    uint8_t bytes_per_framesize_minus_1_2;
+    uint8_t frames_in_superframe_minus_1_2;
 } VP9RawSuperframeIndex;
 
 typedef struct VP9RawSuperframe {
diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
index 2f08eccf180..3f542d0c5d5 100644
--- a/libavcodec/cbs_vp9_syntax_template.c
+++ b/libavcodec/cbs_vp9_syntax_template.c
@@ -421,9 +421,9 @@  static int FUNC(superframe_index)(CodedBitstreamContext *ctx, RWContext *rw,
             frame_sizes[i], 1, i);
     }
 
-    f(3, superframe_marker);
-    f(2, bytes_per_framesize_minus_1);
-    f(3, frames_in_superframe_minus_1);
+    f(3, superframe_marker_2);
+    f(2, bytes_per_framesize_minus_1_2);
+    f(3, frames_in_superframe_minus_1_2);
 
     return 0;
 }