[FFmpeg-devel] [PATCH v3 2/4] cbs_mpeg2: Improve checks for invalid values

[James Almer]

On 4/23/2019 7:55 PM, Andreas Rheinhardt wrote:
> horizontal/vertical_size_value (containing the twelve least significant
> bits of the frame size) mustn't be zero according to the specifications;
> and the value 0 is forbidden for the colour_description elements.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> The actual function calls after macro expansion are the same as in the
> earlier versions with the exception of the extra_bit_slice calls.
>  libavcodec/cbs_mpeg2.c                 | 14 ++++++++------
>  libavcodec/cbs_mpeg2_syntax_template.c | 20 ++++++++++----------
>  2 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index cdde68ea38..fc21745a51 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -41,20 +41,22 @@
>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>  
>  #define ui(width, name) \
> -        xui(width, name, current->name, 0)
> +        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
> +#define uir(width, name, range_min, range_max) \
> +        xui(width, name, current->name, range_min, range_max, 0)
>  #define uis(width, name, subs, ...) \
> -        xui(width, name, current->name, subs, __VA_ARGS__)
> +        xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>  
>  
>  #define READ
>  #define READWRITE read
>  #define RWContext GetBitContext
>  
> -#define xui(width, name, var, subs, ...) do { \
> +#define xui(width, name, var, range_min, range_max, subs, ...) do { \
>          uint32_t value = 0; \
>          CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \
>                                     SUBSCRIPTS(subs, __VA_ARGS__), \
> -                                   &value, 0, (1 << width) - 1)); \
> +                                   &value, range_min, range_max)); \
>          var = value; \
>      } while (0)
>  
> @@ -81,10 +83,10 @@
>  #define READWRITE write
>  #define RWContext PutBitContext
>  
> -#define xui(width, name, var, subs, ...) do { \
> +#define xui(width, name, var, range_min, range_max, subs, ...) do { \
>          CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \
>                                      SUBSCRIPTS(subs, __VA_ARGS__), \
> -                                    var, 0, (1 << width) - 1)); \
> +                                    var, range_min, range_max)); \
>      } while (0)
>  
>  #define marker_bit() do { \
> diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c
> index 10aaea7734..27dcaad316 100644
> --- a/libavcodec/cbs_mpeg2_syntax_template.c
> +++ b/libavcodec/cbs_mpeg2_syntax_template.c
> @@ -26,8 +26,8 @@ static int FUNC(sequence_header)(CodedBitstreamContext *ctx, RWContext *rw,
>  
>      ui(8,  sequence_header_code);
>  
> -    ui(12, horizontal_size_value);
> -    ui(12, vertical_size_value);
> +    uir(12, horizontal_size_value, 1, 4095);
> +    uir(12, vertical_size_value,   1, 4095);
>  
>      mpeg2->horizontal_size = current->horizontal_size_value;
>      mpeg2->vertical_size   = current->vertical_size_value;
> @@ -79,7 +79,7 @@ static int FUNC(user_data)(CodedBitstreamContext *ctx, RWContext *rw,
>  #endif
>  
>      for (k = 0; k < current->user_data_length; k++)
> -        xui(8, user_data, current->user_data[k], 0);
> +        xui(8, user_data, current->user_data[k], 0, 255, 0);

Not sure why this is using xui() to being with, when it could simply be

uis(8, user_data[k], 1, k);

And it would also look better in the trace output.

>  
>      return 0;
>  }
> @@ -125,9 +125,9 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex
>  
>      ui(1, colour_description);
>      if (current->colour_description) {
> -        ui(8, colour_primaries);
> -        ui(8, transfer_characteristics);
> -        ui(8, matrix_coefficients);
> +        uir(8, colour_primaries,         1, 255);
> +        uir(8, transfer_characteristics, 1, 255);
> +        uir(8, matrix_coefficients,      1, 255);
>      }
>  
>      ui(14, display_horizontal_size);
> @@ -366,16 +366,16 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
>                  if (!current->extra_information)
>                      return AVERROR(ENOMEM);
>                  for (k = 0; k < current->extra_information_length; k++) {
> -                    xui(1, extra_bit_slice, bit, 0);
> +                    xui(1, extra_bit_slice, bit, 1, 1, 0);
>                      xui(8, extra_information_slice[k],
> -                        current->extra_information[k], 1, k);
> +                        current->extra_information[k], 0, 255, 1, k);
>                  }
>              }
>  #else
>              for (k = 0; k < current->extra_information_length; k++) {
> -                xui(1, extra_bit_slice, 1, 0);
> +                xui(1, extra_bit_slice, 1, 1, 1, 0);
>                  xui(8, extra_information_slice[k],
> -                    current->extra_information[k], 1, k);
> +                    current->extra_information[k], 0, 255, 1, k);
>              }
>  #endif
>          }

Should be good in any case. Mark can comment on the above and/or push it.