avformat/avc, mxfenc: Avoid allocation of H264 SPS structure, fix memleak

Up until now, ff_avc_decode_sps would parse a SPS and return some
properties from it in a freshly allocated structure. Yet said structure
is very small and completely internal to libavformat, so there is no
reason to use the heap for it. This commit therefore changes the
function to return an int and to modify a caller-provided structure.
This will also allow ff_avc_decode_sps to return better error codes in
the future.

It also fixes a memleak in mxfenc: If a packet contained multiple SPS,
only the SPS structure belonging to the last SPS would be freed, the
other ones would leak when the pointer is overwritten to point to the
new SPS structure. Of course, without allocations there are no leaks.
This is Coverity issue #1445194.

Furthermore, the SPS structure has been renamed from
H264SequenceParameterSet to H264SPS in order to avoid overlong lines.

Reviewed-by: Tomas Härdin <tjoppen@acc.umu.se>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>

diff --git a/libavformat/avc.c b/libavformat/avc.c
index 975d37ae8253b8d5556f0879b23cfc782a1d5430..b5e29213881744cec92d066158c558a2f6d39ef0 100644 (file)
--- a/libavformat/avc.c
+++ b/libavformat/avc.c
@@ -196,18 +196,17 @@ int ff_isom_write_avcc(AVIOContext *pb, const uint8_t *data, int len)
     avio_write(pb, pps, pps_size);
 
     if (sps[3] != 66 && sps[3] != 77 && sps[3] != 88) {
-        H264SequenceParameterSet *seq = ff_avc_decode_sps(sps + 3, sps_size - 3);
-        if (!seq) {
-            ret = AVERROR(ENOMEM);
+        H264SPS seq;
+        ret = ff_avc_decode_sps(&seq, sps + 3, sps_size - 3);
+        if (ret < 0)
             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, 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, nb_sps_ext); /* number of sps ext */
         if (nb_sps_ext)
             avio_write(pb, sps_ext, sps_ext_size);
-        av_free(seq);
     }
 
 fail:
@@ -332,27 +331,24 @@ static inline int get_se_golomb(GetBitContext *gb) {
     return ((v >> 1) ^ sign) - sign;
 }
 
-H264SequenceParameterSet *ff_avc_decode_sps(const uint8_t *buf, int buf_size)
+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;
     int num_ref_frames_in_pic_order_cnt_cycle;
     int delta_scale, lastScale = 8, nextScale = 8;
     int sizeOfScalingList;
-    H264SequenceParameterSet *sps = NULL;
     GetBitContext gb;
     uint8_t *rbsp_buf;
 
     rbsp_buf = ff_nal_unit_extract_rbsp(buf, buf_size, &rbsp_size, 0);
     if (!rbsp_buf)
-        return NULL;
+        return AVERROR(ENOMEM);
 
     ret = init_get_bits8(&gb, rbsp_buf, rbsp_size);
     if (ret < 0)
         goto end;
 
-    sps = av_mallocz(sizeof(*sps));
-    if (!sps)
-        goto end;
+    memset(sps, 0, sizeof(*sps));
 
     sps->profile_idc = get_bits(&gb, 8);
     sps->constraint_set_flags |= get_bits1(&gb) << 0; // constraint_set0_flag
@@ -448,7 +444,8 @@ H264SequenceParameterSet *ff_avc_decode_sps(const uint8_t *buf, int buf_size)
         sps->sar.den = 1;
     }
 
+    ret = 0;
  end:
     av_free(rbsp_buf);
-    return sps;
+    return ret;
 }

diff --git a/libavformat/avc.h b/libavformat/avc.h
index 5286d19d8907a99f9d51d21ff37a87340bb24b89..9792b77913de2596cfa0cc81af3366a5d4b8c086 100644 (file)
--- a/libavformat/avc.h
+++ b/libavformat/avc.h
@@ -46,8 +46,8 @@ typedef struct {
     uint8_t bit_depth_chroma;
     uint8_t frame_mbs_only_flag;
     AVRational sar;
-} H264SequenceParameterSet;
+} H264SPS;
 
-H264SequenceParameterSet *ff_avc_decode_sps(const uint8_t *src, int src_len);
+int ff_avc_decode_sps(H264SPS *sps, const uint8_t *buf, int buf_size);
 
 #endif /* AVFORMAT_AVC_H */

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index c3b6809e984766ba72f1efdf1d839bfedbe7618c..5a3a609bf60b7cdf44fa8fdc7e306cde016e4323 100644 (file)
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2171,14 +2171,14 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
 {
     MXFContext *mxf = s->priv_data;
     MXFStreamContext *sc = st->priv_data;
-    H264SequenceParameterSet *sps = NULL;
+    H264SPS seq, *const sps = &seq;
     GetBitContext gb;
     const uint8_t *buf = pkt->data;
     const uint8_t *buf_end = pkt->data + pkt->size;
     const uint8_t *nal_end;
     uint32_t state = -1;
     int extra_size = 512; // support AVC Intra files without SPS/PPS header
-    int i, frame_size, slice_type, intra_only = 0;
+    int i, frame_size, slice_type, has_sps = 0, intra_only = 0, ret;
 
     for (;;) {
         buf = avpriv_find_start_code(buf, buf_end, &state);
@@ -2193,11 +2193,12 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
                 break;
 
             nal_end = ff_avc_find_startcode(buf, buf_end);
-            sps = ff_avc_decode_sps(buf, nal_end - buf);
-            if (!sps) {
+            ret = ff_avc_decode_sps(sps, buf, nal_end - buf);
+            if (ret < 0) {
                 av_log(s, AV_LOG_ERROR, "error parsing sps\n");
                 return 0;
             }
+            has_sps = 1;
 
             sc->aspect_ratio.num = st->codecpar->width * sps->sar.num;
             sc->aspect_ratio.den = st->codecpar->height * sps->sar.den;
@@ -2243,7 +2244,7 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
     if (mxf->header_written)
         return 1;
 
-    if (!sps)
+    if (!has_sps)
         sc->interlaced = st->codecpar->field_order != AV_FIELD_PROGRESSIVE ? 1 : 0;
     sc->codec_ul = NULL;
     frame_size = pkt->size + extra_size;
@@ -2260,7 +2261,7 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
             if (sc->interlaced)
                 sc->field_dominance = 1; // top field first is mandatory for AVC Intra
             break;
-        } else if (sps && mxf_h264_codec_uls[i].frame_size == 0 &&
+        } else if (has_sps && mxf_h264_codec_uls[i].frame_size == 0 &&
                    mxf_h264_codec_uls[i].profile == sps->profile_idc &&
                    (mxf_h264_codec_uls[i].intra_only < 0 ||
                     mxf_h264_codec_uls[i].intra_only == intra_only)) {
@@ -2271,8 +2272,6 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
         }
     }
 
-    av_free(sps);
-
     if (!sc->codec_ul) {
         av_log(s, AV_LOG_ERROR, "h264 profile not supported\n");
         return 0;