diff mbox series

[FFmpeg-devel,05/11] avcodec/movtextdec: Fix leaks on (re)allocation failure

Message ID 20201017182248.577108-5-andreas.rheinhardt@gmail.com
State Accepted
Commit 94ad68ee17420996c9b003f142717d82b52c0915
Headers show
Series [FFmpeg-devel,01/11] avcodec/movtextdec: Reset counter of fonts when freeing them
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Oct. 17, 2020, 6:22 p.m. UTC
Up until now, the 3GPP Timed Text decoder used av_dynarray_add()
for a list of style entries. Said entries are inidiviually allocated
and owned by the pointers in the dynamic array and are therefore
unsuitable for av_dynarray_add() which simply frees the array,
but not the entries on error. In this case the intended new entry
also leaks because it has been forgotten to free it.

This commit fixes this. It is now allocated in one go and not
reallocated multiple times (and it won't be overallocated any more).
After all, the final number of elements (pending errors) is already
known in advance.

Furthermore, the style entries are now the entries of the new array,
i.e. they are no longer allocated separately. This also removes one
level of indirection.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
If there are several styl boxes in the same packet, the earlier code
would add them to the list (and increment count_s), but lateron use
style_entries (which only contains the number of entries in the last
styl box) as count. So it used the number of the last successfully
parsed styl box, but it used the entries from the earliest styl boxes.
Weird. Anyway, given that one styl box can contain an entry for each
character I only made sure that there are no memleaks when multiple styl
boxes are present.

 libavcodec/movtextdec.c | 84 ++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 48 deletions(-)
diff mbox series

Patch

diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c
index 974118c4c1..e9df979e92 100644
--- a/libavcodec/movtextdec.c
+++ b/libavcodec/movtextdec.c
@@ -98,8 +98,7 @@  typedef struct {
 
 typedef struct {
     AVClass *class;
-    StyleBox **s;
-    StyleBox *s_temp;
+    StyleBox *s;
     HighlightBox h;
     HilightcolorBox c;
     FontRecord *ftab;
@@ -109,7 +108,6 @@  typedef struct {
     uint16_t style_entries, ftab_entries;
     uint64_t tracksize;
     int size_var;
-    int count_s;
     int readorder;
     int frame_width;
     int frame_height;
@@ -123,13 +121,8 @@  typedef struct {
 
 static void mov_text_cleanup(MovTextContext *m)
 {
-    int i;
     if (m->box_flags & STYL_BOX) {
-        for(i = 0; i < m->count_s; i++) {
-            av_freep(&m->s[i]);
-        }
         av_freep(&m->s);
-        m->count_s = 0;
         m->style_entries = 0;
     }
 }
@@ -283,50 +276,45 @@  static int decode_styl(const uint8_t *tsmb, MovTextContext *m, AVPacket *avpkt)
 {
     int i;
     int style_entries = AV_RB16(tsmb);
+    StyleBox *tmp;
     tsmb += 2;
     // A single style record is of length 12 bytes.
     if (m->tracksize + m->size_var + 2 + style_entries * 12 > avpkt->size)
         return -1;
 
+    tmp = av_realloc_array(m->s, style_entries, sizeof(*m->s));
+    if (!tmp)
+        return AVERROR(ENOMEM);
+    m->s             = tmp;
     m->style_entries = style_entries;
 
     m->box_flags |= STYL_BOX;
     for(i = 0; i < m->style_entries; i++) {
-        m->s_temp = av_malloc(sizeof(*m->s_temp));
-        if (!m->s_temp) {
-            mov_text_cleanup(m);
-            return AVERROR(ENOMEM);
-        }
-        m->s_temp->style_start = AV_RB16(tsmb);
+        StyleBox *style = &m->s[i];
+        style->style_start = AV_RB16(tsmb);
         tsmb += 2;
-        m->s_temp->style_end = AV_RB16(tsmb);
+        style->style_end = AV_RB16(tsmb);
 
-        if (   m->s_temp->style_end < m->s_temp->style_start
-            || (m->count_s && m->s_temp->style_start < m->s[m->count_s - 1]->style_end)) {
-            av_freep(&m->s_temp);
+        if (   style->style_end < style->style_start
+            || (i && style->style_start < m->s[i - 1].style_end)) {
             mov_text_cleanup(m);
             return AVERROR(ENOMEM);
         }
 
         tsmb += 2;
-        m->s_temp->style_fontID = AV_RB16(tsmb);
+        style->style_fontID = AV_RB16(tsmb);
         tsmb += 2;
-        m->s_temp->style_flag = AV_RB8(tsmb);
-        m->s_temp->bold = !!(m->s_temp->style_flag & STYLE_FLAG_BOLD);
-        m->s_temp->italic = !!(m->s_temp->style_flag & STYLE_FLAG_ITALIC);
-        m->s_temp->underline = !!(m->s_temp->style_flag & STYLE_FLAG_UNDERLINE);
+        style->style_flag = AV_RB8(tsmb);
+        style->bold      = !!(style->style_flag & STYLE_FLAG_BOLD);
+        style->italic    = !!(style->style_flag & STYLE_FLAG_ITALIC);
+        style->underline = !!(style->style_flag & STYLE_FLAG_UNDERLINE);
         tsmb++;
-        m->s_temp->fontsize = AV_RB8(tsmb);
+        style->fontsize = AV_RB8(tsmb);
         tsmb++;
-        m->s_temp->color = AV_RB24(tsmb);
+        style->color = AV_RB24(tsmb);
         tsmb += 3;
-        m->s_temp->alpha = AV_RB8(tsmb);
+        style->alpha = AV_RB8(tsmb);
         tsmb++;
-        av_dynarray_add(&m->s, &m->count_s, m->s_temp);
-        if(!m->s) {
-            mov_text_cleanup(m);
-            return AVERROR(ENOMEM);
-        }
     }
     return 0;
 }
@@ -376,29 +364,30 @@  static int text_to_ass(AVBPrint *buf, const char *text, const char *text_end,
         int len;
 
         if ((m->box_flags & STYL_BOX) && entry < m->style_entries) {
-            if (text_pos == m->s[entry]->style_start) {
+            const StyleBox *style = &m->s[entry];
+            if (text_pos == style->style_start) {
                 style_active = 1;
-                if (m->s[entry]->bold ^ m->d.bold)
-                    av_bprintf(buf, "{\\b%d}", m->s[entry]->bold);
-                if (m->s[entry]->italic ^ m->d.italic)
-                    av_bprintf(buf, "{\\i%d}", m->s[entry]->italic);
-                if (m->s[entry]->underline ^ m->d.underline)
-                    av_bprintf(buf, "{\\u%d}", m->s[entry]->underline);
-                if (m->s[entry]->fontsize != m->d.fontsize)
-                    av_bprintf(buf, "{\\fs%d}", m->s[entry]->fontsize);
-                if (m->s[entry]->style_fontID != m->d.fontID)
+                if (style->bold ^ m->d.bold)
+                    av_bprintf(buf, "{\\b%d}", style->bold);
+                if (style->italic ^ m->d.italic)
+                    av_bprintf(buf, "{\\i%d}", style->italic);
+                if (style->underline ^ m->d.underline)
+                    av_bprintf(buf, "{\\u%d}", style->underline);
+                if (style->fontsize != m->d.fontsize)
+                    av_bprintf(buf, "{\\fs%d}", style->fontsize);
+                if (style->style_fontID != m->d.fontID)
                     for (i = 0; i < m->ftab_entries; i++) {
-                        if (m->s[entry]->style_fontID == m->ftab[i].fontID)
+                        if (style->style_fontID == m->ftab[i].fontID)
                             av_bprintf(buf, "{\\fn%s}", m->ftab[i].font);
                     }
-                if (m->d.color != m->s[entry]->color) {
-                    color = m->s[entry]->color;
+                if (m->d.color != style->color) {
+                    color = style->color;
                     av_bprintf(buf, "{\\1c&H%X&}", RGB_TO_BGR(color));
                 }
-                if (m->d.alpha != m->s[entry]->alpha)
-                    av_bprintf(buf, "{\\1a&H%02X&}", 255 - m->s[entry]->alpha);
+                if (m->d.alpha != style->alpha)
+                    av_bprintf(buf, "{\\1a&H%02X&}", 255 - style->alpha);
             }
-            if (text_pos == m->s[entry]->style_end) {
+            if (text_pos == style->style_end) {
                 if (style_active) {
                     av_bprintf(buf, "{\\r}");
                     style_active = 0;
@@ -526,7 +515,6 @@  static int mov_text_decode_frame(AVCodecContext *avctx,
     m->tracksize = 2 + text_length;
     m->style_entries = 0;
     m->box_flags = 0;
-    m->count_s = 0;
     // Note that the spec recommends lines be no longer than 2048 characters.
     av_bprint_init(&buf, 0, AV_BPRINT_SIZE_UNLIMITED);
     if (text_length + 2 != avpkt->size) {