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

Submitted by Andreas Rheinhardt on April 23, 2019, 8:32 p.m.

Details

Message ID 20190423203230.4742-2-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt April 23, 2019, 8:32 p.m.
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@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.
 libavcodec/cbs_mpeg2.c                 | 17 ++++++++++-------
 libavcodec/cbs_mpeg2_syntax_template.c | 24 ++++++++++++------------
 2 files changed, 22 insertions(+), 19 deletions(-)

Comments

James Almer April 23, 2019, 9:03 p.m.
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@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
>          }
>

Patch hide | download patch | download mbox

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
         }