diff mbox series

[FFmpeg-devel,v3] avcodec/cbs_vp8: Use little endian in fixed() and improve the pos check

Message ID CH3PR11MB79370540F53B4AEF7E59BD91B1662@CH3PR11MB7937.namprd11.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,v3] avcodec/cbs_vp8: Use little endian in fixed() and improve the pos check | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

Dai, Jianhui J Jan. 5, 2024, 12:34 a.m. UTC
This commit adds value range checks to cbs_vp8_read_unsigned_le,
migrates fixed() to use it, and enforces little-endian consistency for
all read methods.

The VP8 compressed header may not be byte-aligned due to boolean coding.
Use bitwise comparison to prevent the potential overread.

TETS: ffmpeg -i fate-suite/vp8-test-vectors-r1/* -vcodec copy -bsf:v
trace_headers -f null -

Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
---
 libavcodec/cbs_vp8.c | 53 +++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 16 deletions(-)

Comments

Dai, Jianhui J Jan. 5, 2024, 12:38 a.m. UTC | #1
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Dai,
> Jianhui J
> Sent: Friday, January 5, 2024 8:35 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v3] avcodec/cbs_vp8: Use little endian in fixed()
> and improve the pos check
> 
> This commit adds value range checks to cbs_vp8_read_unsigned_le, migrates
> fixed() to use it, and enforces little-endian consistency for all read methods.
> 
> The VP8 compressed header may not be byte-aligned due to boolean coding.
> Use bitwise comparison to prevent the potential overread.
> 
> TETS: ffmpeg -i fate-suite/vp8-test-vectors-r1/* -vcodec copy -bsf:v
> trace_headers -f null -
> 
> Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
> ---
>  libavcodec/cbs_vp8.c | 53 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c index
> 065156c248..338d56ed7f 100644
> --- a/libavcodec/cbs_vp8.c
> +++ b/libavcodec/cbs_vp8.c
> @@ -33,22 +33,22 @@ extern const uint8_t
> ff_vp8_token_update_probs[4][8][3][11];
>  typedef struct CBSVP8BoolDecoder {
>      GetBitContext *gbc;
> 
> -    uint8_t value;
>      uint8_t range;
> 
> -    uint8_t count; // Store the number of bits in the `value` buffer.
> -
> +    uint8_t value;
> +    // Store the number of bits in the `value` buffer.
> +    uint8_t count;
>  } CBSVP8BoolDecoder;
> 
> -static int cbs_vp8_bool_decoder_init(CBSVP8BoolDecoder *decoder,
> GetBitContext *gbc)
> +static int cbs_vp8_bool_decoder_init(CBSVP8BoolDecoder *decoder,
> +                                     GetBitContext *gbc)
>  {
>      av_assert0(decoder);
>      av_assert0(gbc);
> 
>      decoder->gbc = gbc;
> -    decoder->value = 0;
>      decoder->range = 255;
> -
> +    decoder->value = 0;
>      decoder->count = 0;
> 
>      return 0;
> @@ -60,7 +60,7 @@ static bool
> cbs_vp8_bool_decoder_fill_value(CBSVP8BoolDecoder *decoder)
> 
>      av_assert0(decoder->count <= 8);
>      if (decoder->count == 8) {
> -      return true;
> +        return true;
>      }
> 
>      if (get_bits_left(decoder->gbc) >= bits) { @@ -141,7 +141,7 @@ static int
> cbs_vp8_bool_decoder_read_unsigned(
>      }
> 
>      if (trace_enable) {
> -      CBS_TRACE_READ_END();
> +        CBS_TRACE_READ_END();
>      }
> 
>      *write_to = value;
> @@ -181,9 +181,11 @@ static int cbs_vp8_bool_decoder_read_signed(
>      return 0;
>  }
> 
> -static int cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx,
> GetBitContext *gbc,
> -                                 int width, const char *name,
> -                                 const int *subscripts, uint32_t *write_to)
> +static int cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx,
> +                                    GetBitContext *gbc, int width,
> +                                    const char *name, const int *subscripts,
> +                                    uint32_t *write_to, uint32_t range_min,
> +                                    uint32_t range_max)
>  {
>      int32_t value;
> 
> @@ -200,6 +202,14 @@ static int
> cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx, GetBitContext *g
> 
>      CBS_TRACE_READ_END();
> 
> +    if (value < range_min || value > range_max) {
> +        av_log(ctx->log_ctx, AV_LOG_ERROR,
> +               "%s out of range: "
> +               "%" PRIu32 ", but must be in [%" PRIu32 ",%" PRIu32 "].\n",
> +               name, value, range_min, range_max);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
>      *write_to = value;
>      return 0;
>  }
> @@ -246,15 +256,16 @@ static int
> cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx, GetBitContext *g
>      do { \
>          uint32_t value; \
>          CHECK(cbs_vp8_read_unsigned_le(ctx, rw, width, #name, \
> -                                    SUBSCRIPTS(subs, __VA_ARGS__), &value)); \
> +                                       SUBSCRIPTS(subs, __VA_ARGS__), &value, \
> +                                       0, MAX_UINT_BITS(width))); \
>          current->name = value; \
>      } while (0)
> 
>  #define fixed(width, name, value) \
>      do { \
>          uint32_t fixed_value; \
> -        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, 0, &fixed_value, \
> -                                   value, value)); \
> +        CHECK(cbs_vp8_read_unsigned_le(ctx, rw, width, #name, 0, &fixed_value, \
> +                                       value, value)); \
>      } while (0)
> 
>  #define bc_unsigned_subs(width, prob, enable_trace, name, subs, ...) \ @@ -
> 277,6 +288,15 @@ static int cbs_vp8_read_unsigned_le(CodedBitstreamContext
> *ctx, GetBitContext *g
> 
>  #include "cbs_vp8_syntax_template.c"
> 
> +#undef READ
> +#undef READWRITE
> +#undef RWContext
> +#undef CBSVP8BoolCodingRW
> +#undef xf
> +#undef fixed
> +#undef bc_unsigned_subs
> +#undef bc_signed_subs
> +
>  static int cbs_vp8_split_fragment(CodedBitstreamContext *ctx,
>                                    CodedBitstreamFragment *frag, int header)  { @@ -327,9
> +347,10 @@ static int cbs_vp8_read_unit(CodedBitstreamContext *ctx,
>      if (err < 0)
>          return err;
> 
> +    // Position may not be byte-aligned after compressed header; use bit-level
> +    // comparison.
>      pos = get_bits_count(&gbc);
> -    pos /= 8;
> -    av_assert0(pos <= unit->data_size);
> +    av_assert0(pos <= unit->data_size * 8);
> 
>      frame->data_ref = av_buffer_ref(unit->data_ref);
>      if (!frame->data_ref)
> --

@andreas, refined the commit message and fixed the comment of previous patch. Please take a look.

> 2.25.1
> 
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c
index 065156c248..338d56ed7f 100644
--- a/libavcodec/cbs_vp8.c
+++ b/libavcodec/cbs_vp8.c
@@ -33,22 +33,22 @@  extern const uint8_t ff_vp8_token_update_probs[4][8][3][11];
 typedef struct CBSVP8BoolDecoder {
     GetBitContext *gbc;
 
-    uint8_t value;
     uint8_t range;
 
-    uint8_t count; // Store the number of bits in the `value` buffer.
-
+    uint8_t value;
+    // Store the number of bits in the `value` buffer.
+    uint8_t count;
 } CBSVP8BoolDecoder;
 
-static int cbs_vp8_bool_decoder_init(CBSVP8BoolDecoder *decoder, GetBitContext *gbc)
+static int cbs_vp8_bool_decoder_init(CBSVP8BoolDecoder *decoder,
+                                     GetBitContext *gbc)
 {
     av_assert0(decoder);
     av_assert0(gbc);
 
     decoder->gbc = gbc;
-    decoder->value = 0;
     decoder->range = 255;
-
+    decoder->value = 0;
     decoder->count = 0;
 
     return 0;
@@ -60,7 +60,7 @@  static bool cbs_vp8_bool_decoder_fill_value(CBSVP8BoolDecoder *decoder)
 
     av_assert0(decoder->count <= 8);
     if (decoder->count == 8) {
-      return true;
+        return true;
     }
 
     if (get_bits_left(decoder->gbc) >= bits) {
@@ -141,7 +141,7 @@  static int cbs_vp8_bool_decoder_read_unsigned(
     }
 
     if (trace_enable) {
-      CBS_TRACE_READ_END();
+        CBS_TRACE_READ_END();
     }
 
     *write_to = value;
@@ -181,9 +181,11 @@  static int cbs_vp8_bool_decoder_read_signed(
     return 0;
 }
 
-static int cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx, GetBitContext *gbc,
-                                 int width, const char *name,
-                                 const int *subscripts, uint32_t *write_to)
+static int cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx,
+                                    GetBitContext *gbc, int width,
+                                    const char *name, const int *subscripts,
+                                    uint32_t *write_to, uint32_t range_min,
+                                    uint32_t range_max)
 {
     int32_t value;
 
@@ -200,6 +202,14 @@  static int cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx, GetBitContext *g
 
     CBS_TRACE_READ_END();
 
+    if (value < range_min || value > range_max) {
+        av_log(ctx->log_ctx, AV_LOG_ERROR,
+               "%s out of range: "
+               "%" PRIu32 ", but must be in [%" PRIu32 ",%" PRIu32 "].\n",
+               name, value, range_min, range_max);
+        return AVERROR_INVALIDDATA;
+    }
+
     *write_to = value;
     return 0;
 }
@@ -246,15 +256,16 @@  static int cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx, GetBitContext *g
     do { \
         uint32_t value; \
         CHECK(cbs_vp8_read_unsigned_le(ctx, rw, width, #name, \
-                                    SUBSCRIPTS(subs, __VA_ARGS__), &value)); \
+                                       SUBSCRIPTS(subs, __VA_ARGS__), &value, \
+                                       0, MAX_UINT_BITS(width))); \
         current->name = value; \
     } while (0)
 
 #define fixed(width, name, value) \
     do { \
         uint32_t fixed_value; \
-        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, 0, &fixed_value, \
-                                   value, value)); \
+        CHECK(cbs_vp8_read_unsigned_le(ctx, rw, width, #name, 0, &fixed_value, \
+                                       value, value)); \
     } while (0)
 
 #define bc_unsigned_subs(width, prob, enable_trace, name, subs, ...) \
@@ -277,6 +288,15 @@  static int cbs_vp8_read_unsigned_le(CodedBitstreamContext *ctx, GetBitContext *g
 
 #include "cbs_vp8_syntax_template.c"
 
+#undef READ
+#undef READWRITE
+#undef RWContext
+#undef CBSVP8BoolCodingRW
+#undef xf
+#undef fixed
+#undef bc_unsigned_subs
+#undef bc_signed_subs
+
 static int cbs_vp8_split_fragment(CodedBitstreamContext *ctx,
                                   CodedBitstreamFragment *frag, int header)
 {
@@ -327,9 +347,10 @@  static int cbs_vp8_read_unit(CodedBitstreamContext *ctx,
     if (err < 0)
         return err;
 
+    // Position may not be byte-aligned after compressed header; use bit-level
+    // comparison.
     pos = get_bits_count(&gbc);
-    pos /= 8;
-    av_assert0(pos <= unit->data_size);
+    av_assert0(pos <= unit->data_size * 8);
 
     frame->data_ref = av_buffer_ref(unit->data_ref);
     if (!frame->data_ref)