From patchwork Sat Oct 17 07:37:41 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 23032 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 3610044A23D for ; Sat, 17 Oct 2020 10:38:34 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1C63F68B9C2; Sat, 17 Oct 2020 10:38:34 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id AFFD868B94D for ; Sat, 17 Oct 2020 10:38:24 +0300 (EEST) Received: by mail-wr1-f66.google.com with SMTP id x7so5940390wrl.3 for ; Sat, 17 Oct 2020 00:38:24 -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=zMDm9ZChhGIJ3V8EKnZD4+/aRN57c09RWo93R3Cqru0=; b=s1sKu692cX/rLvUlb038OAIgN6rsmKkm7dRUfgxTYZgDiHyOgF8aiesCPbWJVzZKtW pTxyktltHcq7T++VVK7/pq6imN7xfo7Edr/FH2IsRxQwrenk7d0Yeua04uEkDOEX9OFk QkOnGcw8wNQRqbvzBBYQoeACIGBwqvYLOKIj1w8hU2PPaosWGt+n1ArWq+6SB1uVRFkt +IvHaVXLahUpNkCsZ38EFaAOFkU4BlW7gnLyex3887c9hHZC0ogoUyozd5Nxdex5HNow rMJ9ZAk61YvYEGOByDEHFtjc6roSeXH7ORIBFAMA3yiMzf/JMQJq/hW/3lt7xpHjFNc0 rSLg== 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=zMDm9ZChhGIJ3V8EKnZD4+/aRN57c09RWo93R3Cqru0=; b=Fm+ZO1Um/BpFw95u7fZDu2R2eJWlRsGwysHjhrx0vQUdhWh8E+e7XpXfxdk7yaBqOe dHLXMevLAxA23zjEM/2EiXMrEDaxO84pR0nH6Q+gIU7Cm1Qap7hOw5izHH+RMboywOy+ l2AIG3nCX5hCGvBJQxaLuBDUFre/AwCNh+5lWps5dBA+VF1X+4LS1o00P3EMNZm0NdoK 0K5bQCdqb+7of7oR49eIPBkqBbE6JLelZkRCwhCTJRd4irHRRT5Mld0rcj+LeoR/OppB hg0/QdYn0+i4iHzYE2JyP4h6xSIBlrcwud2Ept/Z9mlwfo4YKqoNVHIntlsmTpEWwC2E 0UwQ== X-Gm-Message-State: AOAM532ayDL7I1nwHNEsS5KniXuEhUm2ZKjXQdftmXiSeVtKemwP6dbn glJhO0lVDg4Rp+d/jqQtNGIqHnj2hpo= X-Google-Smtp-Source: ABdhPJyZm95fjp9VNZLfcIU/NtiTSGhXclA8M1TFXvv4yfHTgvo+y/1oWEcbQAapkvA+ZL/HQPbBQg== X-Received: by 2002:adf:de89:: with SMTP id w9mr8728452wrl.212.1602920303680; Sat, 17 Oct 2020 00:38:23 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc1aa4b.dynamic.kabel-deutschland.de. [188.193.170.75]) by smtp.gmail.com with ESMTPSA id 24sm5946553wmg.8.2020.10.17.00.38.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 17 Oct 2020 00:38:22 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Sat, 17 Oct 2020 09:37:41 +0200 Message-Id: <20201017073745.403153-4-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20201017073745.403153-1-andreas.rheinhardt@gmail.com> References: <20201017073745.403153-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH v3 4/8] avcodec/movtextenc: Fix memleak on (re)allocation error 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, the mov_text encoder used the dynamic array API for its list of style attributes; it used the (horrible) av_dynarray_add() which works with an array of pointers; on error it frees its array but not the buffers referenced by the pointers said array contains. It also returns no error code, encouraging not to check for errors. These properties imply that this function may only be used if the buffers referenced by the list either need not be freed at all or if they are freed by other means (i.e. if the list contains non-ownership pointers). In this case, the style attributes are owned by the pointers of the dynamic list. Ergo the old style attributes leak on a subsequent reallocation failure. But given that the (re)allocation isn't checked for success, the style attribute intended to be added to the list also leaks because the only pointer to it gets overwritten in the belief that it is now owned by the list. This commit fixes this by switching to av_fast_realloc() and an array containing the styles directly instead of pointers to individually allocated style attributes. The current style attributes are now no longer individually allocated, instead they are part of the context. Furthermore, av_fast_realloc() allows to easily distinguish between valid and allocated elements, thereby allowing to reuse the array (which up until now has always been freed after processing an AVSubtitleRect). Signed-off-by: Andreas Rheinhardt --- The check "s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes)" below is both good and bad: It is good because the compiler can see for arches where size_t is 64bit and unsigned is 32bit that this is always true, so that it can be optimized away; it is bad, because compilers emit warnings for this. The warning from Clang could be shut up by first checking whether SIZE_MAX / sizeof(*s->style_attributes) exceeds UINT_MAX; but GCC still warns in this case. Unfortunately one can't use sizeof in preprocessor checks. Notice that av_fast_realloc() ensures that s->count * sizeof(*s->style_attributes) is <= UINT_MAX, so that s->count + 1 can't overflow. libavcodec/movtextenc.c | 124 ++++++++++++++++------------------------ 1 file changed, 50 insertions(+), 74 deletions(-) diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c index dcdbf16e08..73d998d080 100644 --- a/libavcodec/movtextenc.c +++ b/libavcodec/movtextenc.c @@ -73,12 +73,13 @@ typedef struct { ASSSplitContext *ass_ctx; ASSStyle *ass_dialog_style; + StyleBox *style_attributes; + unsigned count; + unsigned style_attributes_bytes_allocated; + StyleBox style_attributes_temp; AVBPrint buffer; - StyleBox **style_attributes; - StyleBox *style_attributes_temp; HighlightBox hlit; HilightcolorBox hclr; - int count; uint8_t box_flags; StyleBox d; uint16_t text_pos; @@ -96,22 +97,12 @@ typedef struct { static void mov_text_cleanup(MovTextContext *s) { - int j; - if (s->box_flags & STYL_BOX) { - for (j = 0; j < s->count; j++) { - av_freep(&s->style_attributes[j]); - } - av_freep(&s->style_attributes); - s->count = 0; - } - if (s->style_attributes_temp) { - *s->style_attributes_temp = s->d; - } + s->count = 0; + s->style_attributes_temp = s->d; } static void encode_styl(MovTextContext *s, uint32_t tsmb_type) { - int j; uint32_t tsmb_size; uint16_t style_entries; if ((s->box_flags & STYL_BOX) && s->count) { @@ -124,20 +115,20 @@ static void encode_styl(MovTextContext *s, uint32_t tsmb_type) av_bprint_append_any(&s->buffer, &tsmb_size, 4); av_bprint_append_any(&s->buffer, &tsmb_type, 4); av_bprint_append_any(&s->buffer, &style_entries, 2); - for (j = 0; j < s->count; j++) { + for (unsigned j = 0; j < s->count; j++) { uint16_t style_start, style_end, style_fontID; uint32_t style_color; - style_start = AV_RB16(&s->style_attributes[j]->style_start); - style_end = AV_RB16(&s->style_attributes[j]->style_end); - style_color = AV_RB32(&s->style_attributes[j]->style_color); - style_fontID = AV_RB16(&s->style_attributes[j]->style_fontID); + style_start = AV_RB16(&s->style_attributes[j].style_start); + style_end = AV_RB16(&s->style_attributes[j].style_end); + style_color = AV_RB32(&s->style_attributes[j].style_color); + style_fontID = AV_RB16(&s->style_attributes[j].style_fontID); av_bprint_append_any(&s->buffer, &style_start, 2); av_bprint_append_any(&s->buffer, &style_end, 2); av_bprint_append_any(&s->buffer, &style_fontID, 2); - av_bprint_append_any(&s->buffer, &s->style_attributes[j]->style_flag, 1); - av_bprint_append_any(&s->buffer, &s->style_attributes[j]->style_fontsize, 1); + av_bprint_append_any(&s->buffer, &s->style_attributes[j].style_flag, 1); + av_bprint_append_any(&s->buffer, &s->style_attributes[j].style_fontsize, 1); av_bprint_append_any(&s->buffer, &style_color, 4); } } @@ -186,17 +177,10 @@ const static size_t box_count = FF_ARRAY_ELEMS(box_types); static int mov_text_encode_close(AVCodecContext *avctx) { MovTextContext *s = avctx->priv_data; - int i; ff_ass_split_free(s->ass_ctx); - if (s->style_attributes) { - for (i = 0; i < s->count; i++) { - av_freep(&s->style_attributes[i]); - } - av_freep(&s->style_attributes); - } + av_freep(&s->style_attributes); av_freep(&s->fonts); - av_freep(&s->style_attributes_temp); av_bprint_finalize(&s->buffer, NULL); return 0; } @@ -368,12 +352,6 @@ static av_cold int mov_text_encode_init(AVCodecContext *avctx) av_bprint_init(&s->buffer, 0, AV_BPRINT_SIZE_UNLIMITED); - s->style_attributes_temp = av_mallocz(sizeof(*s->style_attributes_temp)); - if (!s->style_attributes_temp) { - ret = AVERROR(ENOMEM); - goto fail; - } - s->ass_ctx = ff_ass_split(avctx->subtitle_header); if (!s->ass_ctx) { ret = AVERROR_INVALIDDATA; @@ -394,30 +372,34 @@ fail: static int mov_text_style_start(MovTextContext *s) { // there's an existing style entry - if (s->style_attributes_temp->style_start == s->text_pos) + if (s->style_attributes_temp.style_start == s->text_pos) // Still at same text pos, use same entry return 1; - if (s->style_attributes_temp->style_flag != s->d.style_flag || - s->style_attributes_temp->style_color != s->d.style_color || - s->style_attributes_temp->style_fontID != s->d.style_fontID || - s->style_attributes_temp->style_fontsize != s->d.style_fontsize) { + if (s->style_attributes_temp.style_flag != s->d.style_flag || + s->style_attributes_temp.style_color != s->d.style_color || + s->style_attributes_temp.style_fontID != s->d.style_fontID || + s->style_attributes_temp.style_fontsize != s->d.style_fontsize) { + StyleBox *tmp; + // last style != defaults, end the style entry and start a new one - s->box_flags |= STYL_BOX; - s->style_attributes_temp->style_end = s->text_pos; - av_dynarray_add(&s->style_attributes, &s->count, s->style_attributes_temp); - s->style_attributes_temp = av_malloc(sizeof(*s->style_attributes_temp)); - if (!s->style_attributes_temp) { + if (s->count + 1 > SIZE_MAX / sizeof(*s->style_attributes) || + !(tmp = av_fast_realloc(s->style_attributes, + &s->style_attributes_bytes_allocated, + (s->count + 1) * sizeof(*s->style_attributes)))) { mov_text_cleanup(s); av_bprint_clear(&s->buffer); s->box_flags &= ~STYL_BOX; return 0; } - - *s->style_attributes_temp = s->d; - s->style_attributes_temp->style_start = s->text_pos; + s->style_attributes = tmp; + s->style_attributes_temp.style_end = s->text_pos; + s->style_attributes[s->count++] = s->style_attributes_temp; + s->box_flags |= STYL_BOX; + s->style_attributes_temp = s->d; + s->style_attributes_temp.style_start = s->text_pos; } else { // style entry matches defaults, drop entry - *s->style_attributes_temp = s->d; - s->style_attributes_temp->style_start = s->text_pos; + s->style_attributes_temp = s->d; + s->style_attributes_temp.style_start = s->text_pos; } return 1; } @@ -442,13 +424,12 @@ static uint8_t mov_text_style_to_flag(const char style) static void mov_text_style_set(MovTextContext *s, uint8_t style_flags) { - if (!s->style_attributes_temp || - !((s->style_attributes_temp->style_flag & style_flags) ^ style_flags)) { + if (!((s->style_attributes_temp.style_flag & style_flags) ^ style_flags)) { // setting flags that that are already set return; } if (mov_text_style_start(s)) - s->style_attributes_temp->style_flag |= style_flags; + s->style_attributes_temp.style_flag |= style_flags; } static void mov_text_style_cb(void *priv, const char style, int close) @@ -456,29 +437,27 @@ static void mov_text_style_cb(void *priv, const char style, int close) MovTextContext *s = priv; uint8_t style_flag = mov_text_style_to_flag(style); - if (!s->style_attributes_temp || - !!(s->style_attributes_temp->style_flag & style_flag) != close) { + if (!!(s->style_attributes_temp.style_flag & style_flag) != close) { // setting flag that is already set return; } if (mov_text_style_start(s)) { if (!close) - s->style_attributes_temp->style_flag |= style_flag; + s->style_attributes_temp.style_flag |= style_flag; else - s->style_attributes_temp->style_flag &= ~style_flag; + s->style_attributes_temp.style_flag &= ~style_flag; } } static void mov_text_color_set(MovTextContext *s, uint32_t color) { - if (!s->style_attributes_temp || - (s->style_attributes_temp->style_color & 0xffffff00) == color) { + if ((s->style_attributes_temp.style_color & 0xffffff00) == color) { // color hasn't changed return; } if (mov_text_style_start(s)) - s->style_attributes_temp->style_color = (color & 0xffffff00) | - (s->style_attributes_temp->style_color & 0xff); + s->style_attributes_temp.style_color = (color & 0xffffff00) | + (s->style_attributes_temp.style_color & 0xff); } static void mov_text_color_cb(void *priv, unsigned int color, unsigned int color_id) @@ -491,7 +470,7 @@ static void mov_text_color_cb(void *priv, unsigned int color, unsigned int color } else if (color_id == 2) { //secondary color changes if (!(s->box_flags & HCLR_BOX)) // Highlight alpha not set yet, use current primary alpha - s->hclr.color = s->style_attributes_temp->style_color; + s->hclr.color = s->style_attributes_temp.style_color; if (!(s->box_flags & HLIT_BOX) || s->hlit.start == s->text_pos) { s->box_flags |= HCLR_BOX; s->box_flags |= HLIT_BOX; @@ -510,14 +489,13 @@ static void mov_text_color_cb(void *priv, unsigned int color, unsigned int color static void mov_text_alpha_set(MovTextContext *s, uint8_t alpha) { - if (!s->style_attributes_temp || - (s->style_attributes_temp->style_color & 0xff) == alpha) { + if ((s->style_attributes_temp.style_color & 0xff) == alpha) { // color hasn't changed return; } if (mov_text_style_start(s)) - s->style_attributes_temp->style_color = - (s->style_attributes_temp->style_color & 0xffffff00) | alpha; + s->style_attributes_temp.style_color = + (s->style_attributes_temp.style_color & 0xffffff00) | alpha; } static void mov_text_alpha_cb(void *priv, int alpha, int alpha_id) @@ -530,7 +508,7 @@ static void mov_text_alpha_cb(void *priv, int alpha, int alpha_id) else if (alpha_id == 2) { //secondary alpha changes if (!(s->box_flags & HCLR_BOX)) // Highlight color not set yet, use current primary color - s->hclr.color = s->style_attributes_temp->style_color; + s->hclr.color = s->style_attributes_temp.style_color; if (!(s->box_flags & HLIT_BOX) || s->hlit.start == s->text_pos) { s->box_flags |= HCLR_BOX; s->box_flags |= HLIT_BOX; @@ -556,13 +534,12 @@ static uint16_t find_font_id(MovTextContext *s, const char *name) static void mov_text_font_name_set(MovTextContext *s, const char *name) { int fontID = find_font_id(s, name); - if (!s->style_attributes_temp || - s->style_attributes_temp->style_fontID == fontID) { + if (s->style_attributes_temp.style_fontID == fontID) { // color hasn't changed return; } if (mov_text_style_start(s)) - s->style_attributes_temp->style_fontID = fontID; + s->style_attributes_temp.style_fontID = fontID; } static void mov_text_font_name_cb(void *priv, const char *name) @@ -573,13 +550,12 @@ static void mov_text_font_name_cb(void *priv, const char *name) static void mov_text_font_size_set(MovTextContext *s, int size) { size = FONTSIZE_SCALE(s, size); - if (!s->style_attributes_temp || - s->style_attributes_temp->style_fontsize == size) { + if (s->style_attributes_temp.style_fontsize == size) { // color hasn't changed return; } if (mov_text_style_start(s)) - s->style_attributes_temp->style_fontsize = size; + s->style_attributes_temp.style_fontsize = size; } static void mov_text_font_size_cb(void *priv, int size)