avcodec/movtextenc: Fix memleak on (re)allocation error

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).

Reviewed-by: Philip Langdale <philipl@overt.org>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>

1 files 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)