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

[Mark Thompson]

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 at 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