From patchwork Mon Jul 13 19:42:11 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 21000 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 0E0B2448AFC for ; Mon, 13 Jul 2020 22:48:42 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id DDEA468A8E9; Mon, 13 Jul 2020 22:48:41 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 9A99F68A615 for ; Mon, 13 Jul 2020 22:48:33 +0300 (EEST) Received: by mail-wr1-f65.google.com with SMTP id q5so18080554wru.6 for ; Mon, 13 Jul 2020 12:48:33 -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=qgF2/JcxRvGyf2Usd0N0wkdj0wI0GYZPjM2ljV4wrSU=; b=cMIlhzsg7ahXcXMJ54/G6qClN+6DpvmlgbRfT8CCdSBop4MbztBWN2QLY/TI0nObnx zfhtoylGfH+xwC45pIPejmski1OzTPhbFDGGRxBjtHr704XyaQiTI/r/JUGHQZVG6pQ/ V0+apT7f/DX0DR4O1LlQuF93BRcODhgKdYJWe5y+43YKgbfJdQmo/pcQHv54j0gUrLk1 BCUS9e2ndD5sEnUBTmFIJ1ndP+SXtj20FPkPyggIzmrxwRExFyFrqTe+EWeF81qent4Q q9+3E+4M028jIL7IdRJnlZFQHnS5GwTe0VRQFuuDflh84FAehD4oTJK1Vf5JBcGhkB3T y0mA== 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=qgF2/JcxRvGyf2Usd0N0wkdj0wI0GYZPjM2ljV4wrSU=; b=lEZKzFZzKBljuqnw3R+hdq8EISbuffwZUPAG82ndaiY4y7O0AcNyIuByUHdfYgkJDN ZfX5+uOkBu9d/FhGO/5WE6/tKmpPIODM4TnLia+MZd77VPHhBzOgB3ftgX6800PHTkQl GWG9IvNQDpI5pSLdRas7lrdyyhRBX9Pb1nzt0Y8gyALTb6+uolSBHbl1mwtW/QVMq2IF 4X2DJO0JPPRIzhS/UDFkK2RRxGRqktXyibXFvQTqf/jyi+bi3x2Ton8PY78O22kpxG3+ BCM04IuWUFVH/grsr9B3ucQj8q0B1zoO+R68EfOmUn3ArBRiRwzeKBiBpYECZSBIz91X qv4Q== X-Gm-Message-State: AOAM531gzUspacK322lK4Ah/rXAZb6uridUDWYAg3hh1VKNIve+nYZOh QqLp1+q5jm3eRa0yEokIVJ4nljI2 X-Google-Smtp-Source: ABdhPJwo4Wlkh/me73q/RgvtwU+c+Ewlct5lh8/iorOopxdmWTpLg1RtsvTGx08fcgbefnOVNtjjlQ== X-Received: by 2002:a05:6000:1cf:: with SMTP id t15mr1218358wrx.180.1594669712692; Mon, 13 Jul 2020 12:48:32 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc10296.dynamic.kabel-deutschland.de. [188.193.2.150]) by smtp.gmail.com with ESMTPSA id h5sm27532278wrc.97.2020.07.13.12.48.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Jul 2020 12:48:32 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Mon, 13 Jul 2020 21:42:11 +0200 Message-Id: <20200713194211.30244-4-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200713194211.30244-1-andreas.rheinhardt@gmail.com> References: <20200713194211.30244-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 4/4] avformat/au: Avoid allocation for metadata string 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" When there are potentially annotation (i.e. metadata) fields to write, au_get_annotations() is called to produce a string with them. To do so, it uses an AVBPrint which is finalized to create the string. This is wasteful, because it always leads to an allocation even if the string actually fits into the internal buffer of the AVBPrint. This commit changes this by making au_get_annotations() modify an AVBPrint that resides on the stack of the caller (i.e. of au_write_header()). Furthermore, the AVBPrint is now checked for truncation; limiting the allocations implicit in the AVBPrint allowed to offload the overflow checks. Notice that these were not correct before: The size parameter of avio_write() is an int, yet the string in the AVBPrint was allowed to grow bigger than INT_MAX. And if the length of the string was so near UINT_MAX that the length + 32 overflowed, the old code would write the first eight bytes of the string and nothing more, leading to an invalid file. Finally, the special case in which the metadata dictionary of the AVFormatContext is empty (in which case one still has to write eight binary zeroes) is now no longer treated specially, because this case no longer incurs any allocation. Signed-off-by: Andreas Rheinhardt --- libavformat/au.c | 50 ++++++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/libavformat/au.c b/libavformat/au.c index a8906a9db7..c09f4da4c9 100644 --- a/libavformat/au.c +++ b/libavformat/au.c @@ -35,8 +35,6 @@ /* if we don't know the size in advance */ #define AU_UNKNOWN_SIZE ((uint32_t)(~0)) -/* the specification requires an annotation field of at least eight bytes */ -#define AU_DEFAULT_HEADER_SIZE (24+8) static const AVCodecTag codec_au_tags[] = { { AV_CODEC_ID_PCM_MULAW, 1 }, @@ -241,7 +239,7 @@ typedef struct AUContext { #include "rawenc.h" -static int au_get_annotations(AVFormatContext *s, char **buffer) +static int au_get_annotations(AVFormatContext *s, AVBPrint *annotations) { static const char keys[][7] = { "Title", @@ -253,21 +251,19 @@ static int au_get_annotations(AVFormatContext *s, char **buffer) int cnt = 0; AVDictionary *m = s->metadata; AVDictionaryEntry *t = NULL; - AVBPrint bprint; - - av_bprint_init(&bprint, 64, AV_BPRINT_SIZE_UNLIMITED); for (int i = 0; i < FF_ARRAY_ELEMS(keys); i++) { t = av_dict_get(m, keys[i], NULL, 0); if (t != NULL) { if (cnt++) - av_bprint_chars(&bprint, '\n', 1); - av_bprintf(&bprint, "%s=%s", keys[i], t->value); + av_bprint_chars(annotations, '\n', 1); + av_bprintf(annotations, "%s=%s", keys[i], t->value); } } - /* pad with 0's */ - av_bprint_chars(&bprint, '\0', 8); - return av_bprint_finalize(&bprint, buffer); + /* The specification requires the annotation field to be zero-terminated + * and its length to be a multiple of eight, so pad with 0's */ + av_bprint_chars(annotations, '\0', 8); + return av_bprint_is_complete(annotations) ? 0 : AVERROR(ENOMEM); } static int au_write_header(AVFormatContext *s) @@ -276,9 +272,7 @@ static int au_write_header(AVFormatContext *s) AUContext *au = s->priv_data; AVIOContext *pb = s->pb; AVCodecParameters *par = s->streams[0]->codecpar; - char *annotations = NULL; - - au->header_size = AU_DEFAULT_HEADER_SIZE; + AVBPrint annotations; if (s->nb_streams != 1) { av_log(s, AV_LOG_ERROR, "only one stream is supported\n"); @@ -291,30 +285,24 @@ static int au_write_header(AVFormatContext *s) return AVERROR(EINVAL); } - if (av_dict_count(s->metadata) > 0) { - ret = au_get_annotations(s, &annotations); - if (ret < 0) - return ret; - if (annotations != NULL) { - au->header_size = (24 + strlen(annotations) + 8) & ~7; - if (au->header_size < AU_DEFAULT_HEADER_SIZE) - au->header_size = AU_DEFAULT_HEADER_SIZE; - } - } + av_bprint_init(&annotations, 0, INT_MAX - 24); + ret = au_get_annotations(s, &annotations); + if (ret < 0) + goto fail; + au->header_size = 24 + annotations.len & ~7; + ffio_wfourcc(pb, ".snd"); /* magic number */ avio_wb32(pb, au->header_size); /* header size */ avio_wb32(pb, AU_UNKNOWN_SIZE); /* data size */ avio_wb32(pb, par->codec_tag); /* codec ID */ avio_wb32(pb, par->sample_rate); avio_wb32(pb, par->channels); - if (annotations != NULL) { - avio_write(pb, annotations, au->header_size - 24); - av_freep(&annotations); - } else { - avio_wb64(pb, 0); /* annotation field */ - } + avio_write(pb, annotations.str, annotations.len & ~7); - return 0; +fail: + av_bprint_finalize(&annotations, NULL); + + return ret; } static int au_write_trailer(AVFormatContext *s)