From patchwork Wed Jun 24 17:15:14 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 20588 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 1762C44ABAE for ; Wed, 24 Jun 2020 20:16:00 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id EDDD068B597; Wed, 24 Jun 2020 20:15:59 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 53F0768B4DF for ; Wed, 24 Jun 2020 20:15:53 +0300 (EEST) Received: by mail-wr1-f43.google.com with SMTP id r12so3001450wrj.13 for ; Wed, 24 Jun 2020 10:15:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=AKNiJo3jJRZx7cVvL7q4V1vZxKkRDJRthH1WPebQdNY=; b=eMB8K4Vgfi6jDenOF5pnDCimmnQedwuegQhoynLG4MyopMhIQmtXg80fbnN28Y/0+M 76wsEMAwBMzbcOpgcSXjtNaLDgTyzN2/vraErfVrokaqU9gZkYo9zLE9k76W82SWTaF4 KHTIX+0iwAbz2TSJ/zazQtx+lK3WUKzSOD+4C48N+Cdt35UDpyuEELMzNs8dKCq87qiY KzYPF77SKeUB2/uY/OQW6Q+VSoFTJPdklYlNTg4fx+gFb/0GwFgy9caAfzFctWpR4iS3 fuepYCmQqzVjqEKV2DyUnNLg43ATo7n5XlT1OjQPBMlIfTeiWc35KY7j4jva+dT23vGb Y8wg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=AKNiJo3jJRZx7cVvL7q4V1vZxKkRDJRthH1WPebQdNY=; b=OwRBN1iD5sI3uUAH2mVHPAGcHmpQ28FtgGfri0RtkpIYlC32/Tj83aox4iTeXxr3zK Pzt0/axPku8qpjBy/i/QS/JAf9rhF0mIWQreNv6LpmJaRRz9FMn9RbsIHb+I28oSf23X TEMP6o8mm6MMXtLK5NKLet/mtnanXVfP80/tVqtAJsvuhlqLNBk2EfV59M6mFDT95T2K OK0uOjTDotQHtHNoWxApt3kQpj6vxGSR47IfP+01ymA73xYS2jXT51ToxxuW1l8vhDNu /feFcx6GWX25+DQLDuCVkoDnz9yDHYlDB8+TMkzt0XED0thcUkiwzwD3w/BItEhSefv0 vHVA== X-Gm-Message-State: AOAM532fjDOl9V5IUfL/CNT1/otST8+emBthaBrJlIclFPyv3YulsUi5 evb3wjxCoSw2X1DXPnChN9F8cP/K X-Google-Smtp-Source: ABdhPJxDITpWB2//stvvGJLgoXB40g7JcWa+lEHHs2ivX9Fv+ldDzd5bADtJ4FDIHhTTxl+RzV7FiA== X-Received: by 2002:adf:dd8d:: with SMTP id x13mr22385309wrl.362.1593018952155; Wed, 24 Jun 2020 10:15:52 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc10296.dynamic.kabel-deutschland.de. [188.193.2.150]) by smtp.gmail.com with ESMTPSA id n16sm21558852wrq.39.2020.06.24.10.15.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Jun 2020 10:15:50 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Wed, 24 Jun 2020 19:15:14 +0200 Message-Id: <20200624171514.23895-2-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200624171514.23895-1-andreas.rheinhardt@gmail.com> References: <20200624171514.23895-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 2/2] avformat/avc, mxfenc: Avoid allocation of H264 SPS structure, fix memleak X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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. Signed-off-by: Andreas Rheinhardt --- This commit only intends to fix the memleak in mxfenc; I leave any modifications that might need to be made for the case of several SPS to other people. Apropos better error codes: Error checking will necessitate modifications to the helper functions; i.e. get_ue_golomb does not allow to check for invalid data (e.g. 32 zero bits in a row). I am surprised that the fuzzer hasn't complained about this yet. libavformat/avc.c | 27 ++++++++++++--------------- libavformat/avc.h | 4 ++-- libavformat/mxfenc.c | 15 +++++++-------- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/libavformat/avc.c b/libavformat/avc.c index 975d37ae82..b5e2921388 100644 --- 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 5286d19d89..9792b77913 100644 --- 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 c3b6809e98..5a3a609bf6 100644 --- 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;