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

[James Almer]

On 4/23/2019 5:32 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>
> ---
> Given that xui is directly used in the syntax template, I had the
> options a) to add xuir (as I did in my earlier attempt),
> b) to change xui to accept ranges and change the syntax template (add
> the ranges equaling "no custom range") or
> c) change xui and add a new macro for what xui currently does.
> I opted for c). The new macro is named xuib, b in analogy to your
> proposal for cbs_h2645. Unfortunately, the naming of ui is inconsistent
> with this, but changing this would basically touch every line in the
> syntax template.

I think b) is the best option, since my idea was precisely to not add a
new x* macro that duplicates or wraps another x* macro.
If you look at cbs_h2645, the xu() macro is also used directly in the
syntax template, and called with the default range equaling "no custom
range" for the requested width.

Also, unrelated to this patch, but when you send updated patch sets
consider starting a new thread for them rather than as a reply to the
previous one, and adding a v2, v3, etc, depending on what iteration
we're talking about, to each email's subject after the patch number.
That way is easier to locate and also doesn't get mixed with previous
sets and their review replies.

>  libavcodec/cbs_mpeg2.c                 | 17 ++++++++++-------
>  libavcodec/cbs_mpeg2_syntax_template.c | 24 ++++++++++++------------
>  2 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index cdde68ea38..4025812531 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -41,20 +41,23 @@
>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>  
>  #define ui(width, name) \
> -        xui(width, name, current->name, 0)
> +        xuib(width, name, current->name, 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__)
> -
> +        xuib(width, name, current->name, subs, __VA_ARGS__)
> +#define xuib(width, name, var, subs, ...) \
> +        xui(width, name, var, 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 +84,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..73bfeae29e 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);
> +        xuib(8, user_data, current->user_data[k], 0);
>  
>      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(8, extra_information_slice[k],
> -                        current->extra_information[k], 1, k);
> +                    xuib(1, extra_bit_slice, bit, 0);
> +                    xuib(8, extra_information_slice[k],
> +                         current->extra_information[k], 1, k);
>                  }
>              }
>  #else
>              for (k = 0; k < current->extra_information_length; k++) {
> -                xui(1, extra_bit_slice, 1, 0);
> -                xui(8, extra_information_slice[k],
> -                    current->extra_information[k], 1, k);
> +                xuib(1, extra_bit_slice, 1, 0);
> +                xuib(8, extra_information_slice[k],
> +                     current->extra_information[k], 1, k);
>              }
>  #endif
>          }
>