[FFmpeg-devel] [PATCH 14/17] avformat/avc: Be more strict when parsing SPS

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Thu Jul 9 22:20:19 EEST 2020


This commit adds some basic checks for invalid values when parsing an
SPS; e.g. the earlier code did not check whether the values read from
the bitstream get truncated when returned via the H264SPS struct (whose
members are uint8_t), so that a caller could not even check for this
error itself.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
---
 libavformat/avc.c    | 91 ++++++++++++++++++++++++++++----------------
 libavformat/avc.h    |  4 +-
 libavformat/mxfenc.c |  2 +-
 3 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/libavformat/avc.c b/libavformat/avc.c
index 9f375ca992..6246410ad0 100644
--- a/libavformat/avc.c
+++ b/libavformat/avc.c
@@ -27,19 +27,32 @@
 #include "avc.h"
 #include "avio_internal.h"
 
-static inline unsigned get_ue_golomb(GetBitContext *gb)
+static int get_ue_golomb(GetBitContext *gb, unsigned *val)
 {
     int i;
     for (i = 1; i <= 32; i++) {
         if (get_bits_left(gb) < i)
-            return 0;
+            return AVERROR_INVALIDDATA;
         if (show_bits1(gb))
             break;
         skip_bits1(gb);
     }
     if (i > 32)
-        return 0;
-    return get_bits_long(gb, i) - 1;
+        return AVERROR_INVALIDDATA;
+    *val = get_bits_long(gb, i) - 1;
+    return 0;
+}
+
+static int get_se_golomb(GetBitContext *gb, int *val)
+{
+    unsigned v;
+    int sign, ret = get_ue_golomb(gb, &v);
+
+    if (ret < 0)
+        return ret;
+    sign = -(v & 1);
+    *val = ((v >> 1) ^ sign) - sign;
+    return 0;
 }
 
 static const uint8_t *avc_find_startcode_internal(const uint8_t *p, const uint8_t *end)
@@ -242,8 +255,8 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len)
             goto fail;
 
         avio_w8(pb, 0xfc |  seq.chroma_format_idc); /* 6 bits reserved (111111) + chroma_format_idc */
-        avio_w8(pb, 0xf8 | (seq.bit_depth_luma - 8)); /* 5 bits reserved (11111) + bit_depth_luma_minus8 */
-        avio_w8(pb, 0xf8 | (seq.bit_depth_chroma - 8)); /* 5 bits reserved (11111) + bit_depth_chroma_minus8 */
+        avio_w8(pb, 0xf8 | seq.bit_depth_luma_minus8); /* 5 bits reserved (11111) + bit_depth_luma_minus8 */
+        avio_w8(pb, 0xf8 | seq.bit_depth_chroma_minus8); /* 5 bits reserved (11111) + bit_depth_chroma_minus8 */
         avio_w8(pb, nb_sps_ext); /* number of sps ext */
         if (nb_sps_ext)
             avio_write(pb, sps_ext, sps_ext_size);
@@ -357,12 +370,26 @@ static const AVRational avc_sample_aspect_ratio[17] = {
     {   2,  1 },
 };
 
-static inline int get_se_golomb(GetBitContext *gb) {
-    unsigned v = get_ue_golomb(gb) + 1;
-    int sign = -(v & 1);
-    return ((v >> 1) ^ sign) - sign;
-}
-
+#define GET_UE_GOLOMB(dst, max) do { \
+    unsigned val;                    \
+    ret = get_ue_golomb(&gb, &val);  \
+    if (ret < 0)                     \
+        goto end;                    \
+    if (val > (max)) {               \
+        ret = AVERROR_INVALIDDATA;   \
+        goto end;                    \
+    }                                \
+    (dst) = val;                     \
+    } while (0)
+#define GET_SE_GOLOMB(dst) do {      \
+    int val;                         \
+    ret = get_se_golomb(&gb, &val);  \
+    if (ret < 0)                     \
+        goto end;                    \
+    (dst) = val;                     \
+    } while (0)
+#define SKIP_UE_GOLOMB GET_UE_GOLOMB(val, UINT_MAX)
+#define SKIP_SE_GOLOMB GET_SE_GOLOMB(val)
 int ff_avc_decode_sps(H264SPS *sps, const uint8_t *buf, int buf_size)
 {
     int i, j, ret, rbsp_size, aspect_ratio_idc, pic_order_cnt_type;
@@ -391,19 +418,19 @@ int ff_avc_decode_sps(H264SPS *sps, const uint8_t *buf, int buf_size)
     sps->constraint_set_flags |= get_bits1(&gb) << 5; // constraint_set5_flag
     skip_bits(&gb, 2); // reserved_zero_2bits
     sps->level_idc = get_bits(&gb, 8);
-    sps->id = get_ue_golomb(&gb);
+    GET_UE_GOLOMB(sps->id, H264_MAX_SPS_COUNT - 1);
 
     if (sps->profile_idc == 100 || sps->profile_idc == 110 ||
         sps->profile_idc == 122 || sps->profile_idc == 244 || sps->profile_idc ==  44 ||
         sps->profile_idc ==  83 || sps->profile_idc ==  86 || sps->profile_idc == 118 ||
         sps->profile_idc == 128 || sps->profile_idc == 138 || sps->profile_idc == 139 ||
         sps->profile_idc == 134) {
-        sps->chroma_format_idc = get_ue_golomb(&gb); // chroma_format_idc
+        GET_UE_GOLOMB(sps->chroma_format_idc, 3);
         if (sps->chroma_format_idc == 3) {
             skip_bits1(&gb); // separate_colour_plane_flag
         }
-        sps->bit_depth_luma = get_ue_golomb(&gb) + 8;
-        sps->bit_depth_chroma = get_ue_golomb(&gb) + 8;
+        GET_UE_GOLOMB(sps->bit_depth_luma_minus8,   6);
+        GET_UE_GOLOMB(sps->bit_depth_chroma_minus8, 6);
         skip_bits1(&gb); // qpprime_y_zero_transform_bypass_flag
         if (get_bits1(&gb)) { // seq_scaling_matrix_present_flag
             for (i = 0; i < ((sps->chroma_format_idc != 3) ? 8 : 12); i++) {
@@ -414,7 +441,7 @@ int ff_avc_decode_sps(H264SPS *sps, const uint8_t *buf, int buf_size)
                 sizeOfScalingList = i < 6 ? 16 : 64;
                 for (j = 0; j < sizeOfScalingList; j++) {
                     if (nextScale != 0) {
-                        delta_scale = get_se_golomb(&gb);
+                        GET_SE_GOLOMB(delta_scale);
                         nextScale = (lastScale + delta_scale) & 0xff;
                     }
                     lastScale = nextScale == 0 ? lastScale : nextScale;
@@ -423,28 +450,26 @@ int ff_avc_decode_sps(H264SPS *sps, const uint8_t *buf, int buf_size)
         }
     } else {
         sps->chroma_format_idc = 1;
-        sps->bit_depth_luma = 8;
-        sps->bit_depth_chroma = 8;
     }
 
-    get_ue_golomb(&gb); // log2_max_frame_num_minus4
-    pic_order_cnt_type = get_ue_golomb(&gb);
+    SKIP_UE_GOLOMB; // log2_max_frame_num_minus4
+    GET_UE_GOLOMB(pic_order_cnt_type, 2);
 
     if (pic_order_cnt_type == 0) {
-        get_ue_golomb(&gb); // log2_max_pic_order_cnt_lsb_minus4
+        SKIP_UE_GOLOMB; // log2_max_pic_order_cnt_lsb_minus4
     } else if (pic_order_cnt_type == 1) {
         skip_bits1(&gb);    // delta_pic_order_always_zero
-        get_se_golomb(&gb); // offset_for_non_ref_pic
-        get_se_golomb(&gb); // offset_for_top_to_bottom_field
-        num_ref_frames_in_pic_order_cnt_cycle = get_ue_golomb(&gb);
+        SKIP_SE_GOLOMB; // offset_for_non_ref_pic
+        SKIP_SE_GOLOMB; // offset_for_top_to_bottom_field
+        GET_UE_GOLOMB(num_ref_frames_in_pic_order_cnt_cycle, 255);
         for (i = 0; i < num_ref_frames_in_pic_order_cnt_cycle; i++)
-            get_se_golomb(&gb); // offset_for_ref_frame
+            SKIP_SE_GOLOMB; // offset_for_ref_frame
     }
 
-    get_ue_golomb(&gb); // max_num_ref_frames
+    SKIP_UE_GOLOMB; // max_num_ref_frames
     skip_bits1(&gb); // gaps_in_frame_num_value_allowed_flag
-    get_ue_golomb(&gb); // pic_width_in_mbs_minus1
-    get_ue_golomb(&gb); // pic_height_in_map_units_minus1
+    SKIP_UE_GOLOMB; // pic_width_in_mbs_minus1
+    SKIP_UE_GOLOMB; // pic_height_in_map_units_minus1
 
     sps->frame_mbs_only_flag = get_bits1(&gb);
     if (!sps->frame_mbs_only_flag)
@@ -453,10 +478,10 @@ int ff_avc_decode_sps(H264SPS *sps, const uint8_t *buf, int buf_size)
     skip_bits1(&gb); // direct_8x8_inference_flag
 
     if (get_bits1(&gb)) { // frame_cropping_flag
-        get_ue_golomb(&gb); // frame_crop_left_offset
-        get_ue_golomb(&gb); // frame_crop_right_offset
-        get_ue_golomb(&gb); // frame_crop_top_offset
-        get_ue_golomb(&gb); // frame_crop_bottom_offset
+        SKIP_UE_GOLOMB; // frame_crop_left_offset
+        SKIP_UE_GOLOMB; // frame_crop_right_offset
+        SKIP_UE_GOLOMB; // frame_crop_top_offset
+        SKIP_UE_GOLOMB; // frame_crop_bottom_offset
     }
 
     if (get_bits1(&gb)) { // vui_parameters_present_flag
diff --git a/libavformat/avc.h b/libavformat/avc.h
index b3df0a7b6b..db1d3334dd 100644
--- a/libavformat/avc.h
+++ b/libavformat/avc.h
@@ -44,8 +44,8 @@ typedef struct {
     uint8_t level_idc;
     uint8_t constraint_set_flags;
     uint8_t chroma_format_idc;
-    uint8_t bit_depth_luma;
-    uint8_t bit_depth_chroma;
+    uint8_t bit_depth_luma_minus8;
+    uint8_t bit_depth_chroma_minus8;
     uint8_t frame_mbs_only_flag;
     AVRational sar;
 } H264SPS;
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 5a3a609bf6..5eeeb9ab84 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2206,7 +2206,7 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
                       sc->aspect_ratio.num, sc->aspect_ratio.den, 1024*1024);
             intra_only = (sps->constraint_set_flags >> 3) & 1;
             sc->interlaced = !sps->frame_mbs_only_flag;
-            sc->component_depth = sps->bit_depth_luma;
+            sc->component_depth = sps->bit_depth_luma_minus8 + 8;
 
             buf = nal_end;
             break;
-- 
2.20.1



More information about the ffmpeg-devel mailing list