[02/14] cbs_h264: Fix overflow in shifts

Message ID 20190504121427.9638-3-lu_zero@gentoo.org
State New
Headers show
Series
  • [01/14] maint: Ignore dot dirs
Related show

Commit Message

Luca Barbato May 4, 2019, 12:14 p.m.
From: Mark Thompson <sw@jkqxz.net>

The type of the result of a shift operation is unaffected by the type of
the right operand, so some existing code overflows with undefined behaviour
when the element length is 32.  Add a helper macro to calculate the maximum
value correctly and then use it everywhere this pattern appears.

Found-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
---
 libavcodec/cbs_h264_syntax_template.c | 22 +++++++++++-----------
 libavcodec/cbs_internal.h             |  4 ++++
 2 files changed, 15 insertions(+), 11 deletions(-)

Comments

Alexandra Hájková May 13, 2019, 8:38 a.m. | #1
On Sat, May 4, 2019 at 2:14 PM Luca Barbato <lu_zero@gentoo.org> wrote:
>
> From: Mark Thompson <sw@jkqxz.net>
>
> The type of the result of a shift operation is unaffected by the type of
> the right operand, so some existing code overflows with undefined behaviour
> when the element length is 32.  Add a helper macro to calculate the maximum
> value correctly and then use it everywhere this pattern appears.
>
> Found-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
> ---
>  libavcodec/cbs_h264_syntax_template.c | 22 +++++++++++-----------
>  libavcodec/cbs_internal.h             |  4 ++++
>  2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c
> index 1aa7888584..92c1b67862 100644
> --- a/libavcodec/cbs_h264_syntax_template.c
> +++ b/libavcodec/cbs_h264_syntax_template.c
> @@ -342,8 +342,8 @@ static int FUNC(sps_extension)(CodedBitstreamContext *ctx, RWContext *rw,
>          flag(alpha_incr_flag);
>
>          bits = current->bit_depth_aux_minus8 + 9;
> -        u(bits, alpha_opaque_value,      0, (1 << bits) - 1);
> -        u(bits, alpha_transparent_value, 0, (1 << bits) - 1);
> +        u(bits, alpha_opaque_value,      0, MAX_UINT_BITS(bits));
> +        u(bits, alpha_transparent_value, 0, MAX_UINT_BITS(bits));
>      }
>
>      flag(additional_extension_flag);
> @@ -483,10 +483,10 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>              length = sps->vui.nal_hrd_parameters.initial_cpb_removal_delay_length_minus1 + 1;
>              xu(length, initial_cpb_removal_delay[SchedSelIdx],
>                 current->nal.initial_cpb_removal_delay[i],
> -               0, (1 << (uint64_t)length) - 1);
> +               1, MAX_UINT_BITS(length));
>              xu(length, initial_cpb_removal_delay_offset[SchedSelIdx],
>                 current->nal.initial_cpb_removal_delay_offset[i],
> -               0, (1 << (uint64_t)length) - 1);
> +               0, MAX_UINT_BITS(length));
>          }
>      }
>
> @@ -495,10 +495,10 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>              length = sps->vui.vcl_hrd_parameters.initial_cpb_removal_delay_length_minus1 + 1;
>              xu(length, initial_cpb_removal_delay[SchedSelIdx],
>                 current->vcl.initial_cpb_removal_delay[i],
> -               0, (1 << (uint64_t)length) - 1);
> +               1, MAX_UINT_BITS(length));
>              xu(length, initial_cpb_removal_delay_offset[SchedSelIdx],
>                 current->vcl.initial_cpb_removal_delay_offset[i],
> -               0, (1 << (uint64_t)length) - 1);
> +               0, MAX_UINT_BITS(length));
>          }
>      }
>
> @@ -548,7 +548,7 @@ static int FUNC(sei_pic_timestamp)(CodedBitstreamContext *ctx, RWContext *rw,
>
>      if (time_offset_length > 0)
>          u(time_offset_length, time_offset,
> -          0, (1 << (uint64_t)time_offset_length) - 1);
> +          0, MAX_UINT_BITS(time_offset_length));
>      else
>          infer(time_offset, 0);
>
> @@ -600,9 +600,9 @@ static int FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, RWContext *rw,
>          }
>
>          u(hrd->cpb_removal_delay_length_minus1 + 1, cpb_removal_delay,
> -          0, (1 << (uint64_t)hrd->cpb_removal_delay_length_minus1) + 1);
> +          0, MAX_UINT_BITS(hrd->cpb_removal_delay_length_minus1 + 1));
>          u(hrd->dpb_output_delay_length_minus1 + 1, dpb_output_delay,
> -          0, (1 << (uint64_t)hrd->dpb_output_delay_length_minus1) + 1);
> +          0, MAX_UINT_BITS(hrd->dpb_output_delay_length_minus1 + 1));
>      }
>
>      if (sps->vui.pic_struct_present_flag) {
> @@ -1123,7 +1123,7 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
>          u(2, colour_plane_id, 0, 2);
>
>      u(sps->log2_max_frame_num_minus4 + 4, frame_num,
> -      0, (1 << (sps->log2_max_frame_num_minus4 + 4)) - 1);
> +      0, MAX_UINT_BITS(sps->log2_max_frame_num_minus4 + 4));
>
>      if (!sps->frame_mbs_only_flag) {
>          flag(field_pic_flag);
> @@ -1141,7 +1141,7 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
>
>      if (sps->pic_order_cnt_type == 0) {
>          u(sps->log2_max_pic_order_cnt_lsb_minus4 + 4, pic_order_cnt_lsb,
> -          0, (1 << (sps->log2_max_pic_order_cnt_lsb_minus4 + 4)) - 1);
> +          0, MAX_UINT_BITS(sps->log2_max_pic_order_cnt_lsb_minus4 + 4));
>          if (pps->bottom_field_pic_order_in_frame_present_flag &&
>              !current->field_pic_flag)
>              se(delta_pic_order_cnt_bottom, INT32_MIN + 1, INT32_MAX);
> diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
> index 4c6f421d19..54265d8e0e 100644
> --- a/libavcodec/cbs_internal.h
> +++ b/libavcodec/cbs_internal.h
> @@ -79,6 +79,10 @@ int ff_cbs_write_unsigned(CodedBitstreamContext *ctx, PutBitContext *pbc,
>                            int width, const char *name, uint32_t value,
>                            uint32_t range_min, uint32_t range_max);
>
> +// The largest value representable in N bits, suitable for use as
> +// range_max in the above functions.
> +#define MAX_UINT_BITS(length) ((UINT64_C(1) << (length)) - 1)
> +
>
>  extern const CodedBitstreamType ff_cbs_type_h264;
>  extern const CodedBitstreamType ff_cbs_type_h265;
> --
> 2.12.2
>

OK

Patch

diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c
index 1aa7888584..92c1b67862 100644
--- a/libavcodec/cbs_h264_syntax_template.c
+++ b/libavcodec/cbs_h264_syntax_template.c
@@ -342,8 +342,8 @@  static int FUNC(sps_extension)(CodedBitstreamContext *ctx, RWContext *rw,
         flag(alpha_incr_flag);
 
         bits = current->bit_depth_aux_minus8 + 9;
-        u(bits, alpha_opaque_value,      0, (1 << bits) - 1);
-        u(bits, alpha_transparent_value, 0, (1 << bits) - 1);
+        u(bits, alpha_opaque_value,      0, MAX_UINT_BITS(bits));
+        u(bits, alpha_transparent_value, 0, MAX_UINT_BITS(bits));
     }
 
     flag(additional_extension_flag);
@@ -483,10 +483,10 @@  static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
             length = sps->vui.nal_hrd_parameters.initial_cpb_removal_delay_length_minus1 + 1;
             xu(length, initial_cpb_removal_delay[SchedSelIdx],
                current->nal.initial_cpb_removal_delay[i],
-               0, (1 << (uint64_t)length) - 1);
+               1, MAX_UINT_BITS(length));
             xu(length, initial_cpb_removal_delay_offset[SchedSelIdx],
                current->nal.initial_cpb_removal_delay_offset[i],
-               0, (1 << (uint64_t)length) - 1);
+               0, MAX_UINT_BITS(length));
         }
     }
 
@@ -495,10 +495,10 @@  static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
             length = sps->vui.vcl_hrd_parameters.initial_cpb_removal_delay_length_minus1 + 1;
             xu(length, initial_cpb_removal_delay[SchedSelIdx],
                current->vcl.initial_cpb_removal_delay[i],
-               0, (1 << (uint64_t)length) - 1);
+               1, MAX_UINT_BITS(length));
             xu(length, initial_cpb_removal_delay_offset[SchedSelIdx],
                current->vcl.initial_cpb_removal_delay_offset[i],
-               0, (1 << (uint64_t)length) - 1);
+               0, MAX_UINT_BITS(length));
         }
     }
 
@@ -548,7 +548,7 @@  static int FUNC(sei_pic_timestamp)(CodedBitstreamContext *ctx, RWContext *rw,
 
     if (time_offset_length > 0)
         u(time_offset_length, time_offset,
-          0, (1 << (uint64_t)time_offset_length) - 1);
+          0, MAX_UINT_BITS(time_offset_length));
     else
         infer(time_offset, 0);
 
@@ -600,9 +600,9 @@  static int FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, RWContext *rw,
         }
 
         u(hrd->cpb_removal_delay_length_minus1 + 1, cpb_removal_delay,
-          0, (1 << (uint64_t)hrd->cpb_removal_delay_length_minus1) + 1);
+          0, MAX_UINT_BITS(hrd->cpb_removal_delay_length_minus1 + 1));
         u(hrd->dpb_output_delay_length_minus1 + 1, dpb_output_delay,
-          0, (1 << (uint64_t)hrd->dpb_output_delay_length_minus1) + 1);
+          0, MAX_UINT_BITS(hrd->dpb_output_delay_length_minus1 + 1));
     }
 
     if (sps->vui.pic_struct_present_flag) {
@@ -1123,7 +1123,7 @@  static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
         u(2, colour_plane_id, 0, 2);
 
     u(sps->log2_max_frame_num_minus4 + 4, frame_num,
-      0, (1 << (sps->log2_max_frame_num_minus4 + 4)) - 1);
+      0, MAX_UINT_BITS(sps->log2_max_frame_num_minus4 + 4));
 
     if (!sps->frame_mbs_only_flag) {
         flag(field_pic_flag);
@@ -1141,7 +1141,7 @@  static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
 
     if (sps->pic_order_cnt_type == 0) {
         u(sps->log2_max_pic_order_cnt_lsb_minus4 + 4, pic_order_cnt_lsb,
-          0, (1 << (sps->log2_max_pic_order_cnt_lsb_minus4 + 4)) - 1);
+          0, MAX_UINT_BITS(sps->log2_max_pic_order_cnt_lsb_minus4 + 4));
         if (pps->bottom_field_pic_order_in_frame_present_flag &&
             !current->field_pic_flag)
             se(delta_pic_order_cnt_bottom, INT32_MIN + 1, INT32_MAX);
diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
index 4c6f421d19..54265d8e0e 100644
--- a/libavcodec/cbs_internal.h
+++ b/libavcodec/cbs_internal.h
@@ -79,6 +79,10 @@  int ff_cbs_write_unsigned(CodedBitstreamContext *ctx, PutBitContext *pbc,
                           int width, const char *name, uint32_t value,
                           uint32_t range_min, uint32_t range_max);
 
+// The largest value representable in N bits, suitable for use as
+// range_max in the above functions.
+#define MAX_UINT_BITS(length) ((UINT64_C(1) << (length)) - 1)
+
 
 extern const CodedBitstreamType ff_cbs_type_h264;
 extern const CodedBitstreamType ff_cbs_type_h265;