diff mbox series

[FFmpeg-devel] libavformat: fix incorrect handling of incomplete AVBPrint.

Message ID 20230727174051.9191-1-Reimar.Doeffinger@gmx.de
State Accepted
Commit c0f867bf503e79eba8ee52e1ac53322f88ec2929
Headers show
Series [FFmpeg-devel] libavformat: fix incorrect handling of incomplete AVBPrint. | expand

Checks

Context Check Description
yinshiyou/make_fate_loongarch64 success Make fate finished
yinshiyou/make_loongarch64 warning New warnings during build
andriy/make_fate_x86 success Make fate finished
andriy/make_x86 warning New warnings during build

Commit Message

Reimar Döffinger July 27, 2023, 5:40 p.m. UTC 
From: Reimar Döffinger <Reimar.Doeffinger@gmx.de>

Change some internal APIs a bit to make it harder to make
such mistakes.
In particular, have the read chunk functions return an error
when the result is incomplete.
This might be less flexible, but since there has been no
use-case for that so far, avoiding coding mistakes seems better.
Add a function to queue a AVBPrint directly (ff_subtitles_queue_insert_bprint).
Also fixes a leak in lrcdec when ff_subtitles_queue_insert fails.

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 libavformat/assdec.c         |  4 +++-
 libavformat/lrcdec.c         |  7 ++++++-
 libavformat/mpsubdec.c       |  5 +++--
 libavformat/realtextdec.c    |  6 +++++-
 libavformat/samidec.c        |  6 +++++-
 libavformat/srtdec.c         |  4 +++-
 libavformat/subtitles.c      | 19 +++++++++++++++----
 libavformat/subtitles.h      | 14 ++++++++++++--
 libavformat/tedcaptionsdec.c |  2 +-
 libavformat/webvttdec.c      |  4 +++-
 10 files changed, 56 insertions(+), 15 deletions(-)
diff mbox series

Patch

diff --git a/libavformat/assdec.c b/libavformat/assdec.c
index 0915f6fafd..bf7b8a73a2 100644
--- a/libavformat/assdec.c
+++ b/libavformat/assdec.c
@@ -73,6 +73,8 @@  static int read_dialogue(ASSContext *ass, AVBPrint *dst, const uint8_t *p,
 
         av_bprint_clear(dst);
         av_bprintf(dst, "%u,%d,%s", ass->readorder++, layer, p + pos);
+        if (!av_bprint_is_complete(dst))
+            return AVERROR(ENOMEM);
 
         /* right strip the buffer */
         while (dst->len > 0 &&
@@ -135,7 +137,7 @@  static int ass_read_header(AVFormatContext *s)
             av_bprintf(&header, "%s", line.str);
             continue;
         }
-        sub = ff_subtitles_queue_insert(&ass->q, rline.str, rline.len, 0);
+        sub = ff_subtitles_queue_insert_bprint(&ass->q, &rline, 0);
         if (!sub) {
             res = AVERROR(ENOMEM);
             goto end;
diff --git a/libavformat/lrcdec.c b/libavformat/lrcdec.c
index fff39495f8..83bb4a4b75 100644
--- a/libavformat/lrcdec.c
+++ b/libavformat/lrcdec.c
@@ -171,6 +171,8 @@  static int lrc_read_header(AVFormatContext *s)
 
     while(!avio_feof(s->pb)) {
         int64_t pos = read_line(&line, s->pb);
+        if (!av_bprint_is_complete(&line))
+            goto err_nomem_out;
         int64_t header_offset = find_header(line.str);
         if(header_offset >= 0) {
             char *comma_offset = strchr(line.str, ':');
@@ -205,7 +207,7 @@  static int lrc_read_header(AVFormatContext *s)
                 sub = ff_subtitles_queue_insert(&lrc->q, line.str + ts_strlength,
                                                 line.len - ts_strlength, 0);
                 if (!sub)
-                    return AVERROR(ENOMEM);
+                    goto err_nomem_out;
                 sub->pos = pos;
                 sub->pts = ts_start - lrc->ts_offset;
                 sub->duration = -1;
@@ -216,6 +218,9 @@  static int lrc_read_header(AVFormatContext *s)
     ff_metadata_conv_ctx(s, NULL, ff_lrc_metadata_conv);
     av_bprint_finalize(&line, NULL);
     return 0;
+err_nomem_out:
+    av_bprint_finalize(&line, NULL);
+    return AVERROR(ENOMEM);
 }
 
 const AVInputFormat ff_lrc_demuxer = {
diff --git a/libavformat/mpsubdec.c b/libavformat/mpsubdec.c
index d290a41fb9..0374563575 100644
--- a/libavformat/mpsubdec.c
+++ b/libavformat/mpsubdec.c
@@ -116,9 +116,10 @@  static int mpsub_read_header(AVFormatContext *s)
             AVPacket *sub;
             const int64_t pos = avio_tell(s->pb);
 
-            ff_subtitles_read_chunk(s->pb, &buf);
+            res = ff_subtitles_read_chunk(s->pb, &buf);
+            if (res < 0) goto end;
             if (buf.len) {
-                sub = ff_subtitles_queue_insert(&mpsub->q, buf.str, buf.len, 0);
+                sub = ff_subtitles_queue_insert_bprint(&mpsub->q, &buf, 0);
                 if (!sub) {
                     res = AVERROR(ENOMEM);
                     goto end;
diff --git a/libavformat/realtextdec.c b/libavformat/realtextdec.c
index c281dec346..7992a5b7fc 100644
--- a/libavformat/realtextdec.c
+++ b/libavformat/realtextdec.c
@@ -80,6 +80,10 @@  static int realtext_read_header(AVFormatContext *s)
         const int64_t pos = ff_text_pos(&tr) - (c != 0);
         int n = ff_smil_extract_next_text_chunk(&tr, &buf, &c);
 
+        if (n < 0) {
+            res = n;
+            goto end;
+        }
         if (n == 0)
             break;
 
@@ -103,7 +107,7 @@  static int realtext_read_header(AVFormatContext *s)
             /* if we just read a <time> tag, introduce a new event, otherwise merge
              * with the previous one */
             int merge = !av_strncasecmp(buf.str, "<time", 5) ? 0 : 1;
-            sub = ff_subtitles_queue_insert(&rt->q, buf.str, buf.len, merge);
+            sub = ff_subtitles_queue_insert_bprint(&rt->q, &buf, merge);
             if (!sub) {
                 res = AVERROR(ENOMEM);
                 goto end;
diff --git a/libavformat/samidec.c b/libavformat/samidec.c
index 0da299343d..070b623ebf 100644
--- a/libavformat/samidec.c
+++ b/libavformat/samidec.c
@@ -68,6 +68,10 @@  static int sami_read_header(AVFormatContext *s)
         const int64_t pos = ff_text_pos(&tr) - (c != 0);
         int is_sync, is_body, n = ff_smil_extract_next_text_chunk(&tr, &buf, &c);
 
+        if (n < 0) {
+            res = n;
+            goto end;
+        }
         if (n == 0)
             break;
 
@@ -84,7 +88,7 @@  static int sami_read_header(AVFormatContext *s)
         if (!got_first_sync_point) {
             av_bprintf(&hdr_buf, "%s", buf.str);
         } else {
-            sub = ff_subtitles_queue_insert(&sami->q, buf.str, buf.len, !is_sync);
+            sub = ff_subtitles_queue_insert_bprint(&sami->q, &buf, !is_sync);
             if (!sub) {
                 res = AVERROR(ENOMEM);
                 av_bprint_finalize(&hdr_buf, NULL);
diff --git a/libavformat/srtdec.c b/libavformat/srtdec.c
index bf02450555..635c1550b9 100644
--- a/libavformat/srtdec.c
+++ b/libavformat/srtdec.c
@@ -97,12 +97,14 @@  static int add_event(FFDemuxSubtitlesQueue *q, AVBPrint *buf, char *line_cache,
     if (append_cache && line_cache[0])
         av_bprintf(buf, "%s\n", line_cache);
     line_cache[0] = 0;
+    if (!av_bprint_is_complete(buf))
+        return AVERROR(ENOMEM);
 
     while (buf->len > 0 && buf->str[buf->len - 1] == '\n')
         buf->str[--buf->len] = 0;
 
     if (buf->len) {
-        AVPacket *sub = ff_subtitles_queue_insert(q, buf->str, buf->len, 0);
+        AVPacket *sub = ff_subtitles_queue_insert_bprint(q, buf, 0);
         if (!sub)
             return AVERROR(ENOMEM);
         av_bprint_clear(buf);
diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
index 3ba5e2b217..6c221e4b29 100644
--- a/libavformat/subtitles.c
+++ b/libavformat/subtitles.c
@@ -145,6 +145,14 @@  AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q,
     return sub;
 }
 
+AVPacket *ff_subtitles_queue_insert_bprint(FFDemuxSubtitlesQueue *q,
+                                           const AVBPrint *event, int merge)
+{
+    if (!av_bprint_is_complete(event))
+        return NULL;
+    return ff_subtitles_queue_insert(q, event->str, event->len, merge);
+}
+
 static int cmp_pkt_sub_ts_pos(const void *a, const void *b)
 {
     const AVPacket *s1 = *(const AVPacket **)a;
@@ -347,13 +355,15 @@  int ff_smil_extract_next_text_chunk(FFTextReader *tr, AVBPrint *buf, char *c)
     do {
         av_bprint_chars(buf, *c, 1);
         *c = ff_text_r8(tr);
+        if (i == INT_MAX)
+            return AVERROR_INVALIDDATA;
         i++;
     } while (*c != end_chr && *c);
     if (end_chr == '>') {
         av_bprint_chars(buf, '>', 1);
         *c = 0;
     }
-    return i;
+    return av_bprint_is_complete(buf) ? i : AVERROR(ENOMEM);
 }
 
 const char *ff_smil_get_attr_ptr(const char *s, const char *attr)
@@ -381,7 +391,7 @@  static inline int is_eol(char c)
     return c == '\r' || c == '\n';
 }
 
-void ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf)
+int ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf)
 {
     char eol_buf[5], last_was_cr = 0;
     int n = 0, i = 0, nb_eol = 0;
@@ -421,15 +431,16 @@  void ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf)
         av_bprint_chars(buf, c, 1);
         n++;
     }
+    return av_bprint_is_complete(buf) ? 0 : AVERROR(ENOMEM);
 }
 
-void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf)
+int ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf)
 {
     FFTextReader tr;
     tr.buf_pos = tr.buf_len = 0;
     tr.type = 0;
     tr.pb = pb;
-    ff_subtitles_read_text_chunk(&tr, buf);
+    return ff_subtitles_read_text_chunk(&tr, buf);
 }
 
 ptrdiff_t ff_subtitles_read_line(FFTextReader *tr, char *buf, size_t size)
diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h
index 4460efacf3..f6688b71d7 100644
--- a/libavformat/subtitles.h
+++ b/libavformat/subtitles.h
@@ -120,6 +120,12 @@  typedef struct {
 AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q,
                                     const uint8_t *event, size_t len, int merge);
 
+/**
+ * Same as ff_subtitles_queue_insert but takes an AVBPrint input.
+ * Avoids common errors like handling incomplete AVBPrint.
+ */
+AVPacket *ff_subtitles_queue_insert_bprint(FFDemuxSubtitlesQueue *q,
+                                           const AVBPrint *event, int merge);
 /**
  * Set missing durations, sort subtitles by PTS (and then byte position), and
  * drop duplicated events.
@@ -155,6 +161,7 @@  int ff_subtitles_read_close(AVFormatContext *s);
  * SMIL helper to load next chunk ("<...>" or untagged content) in buf.
  *
  * @param c cached character, to avoid a backward seek
+ * @return size of chunk or error, e.g. AVERROR(ENOMEM) on incomplete buf
  */
 int ff_smil_extract_next_text_chunk(FFTextReader *tr, AVBPrint *buf, char *c);
 
@@ -169,7 +176,8 @@  const char *ff_smil_get_attr_ptr(const char *s, const char *attr);
 /**
  * @brief Same as ff_subtitles_read_text_chunk(), but read from an AVIOContext.
  */
-void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf);
+av_warn_unused_result
+int ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf);
 
 /**
  * @brief Read a subtitles chunk from FFTextReader.
@@ -181,10 +189,12 @@  void ff_subtitles_read_chunk(AVIOContext *pb, AVBPrint *buf);
  *
  * @param tr  I/O context
  * @param buf an initialized buf where the chunk is written
+ * @return 0 on success, error value otherwise, e.g. AVERROR(ENOMEM) if buf is incomplete
  *
  * @note buf is cleared before writing into it.
  */
-void ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf);
+av_warn_unused_result
+int ff_subtitles_read_text_chunk(FFTextReader *tr, AVBPrint *buf);
 
 /**
  * Get the number of characters to increment to jump to the next line, or to
diff --git a/libavformat/tedcaptionsdec.c b/libavformat/tedcaptionsdec.c
index 31197349b0..a8aa9d9cf9 100644
--- a/libavformat/tedcaptionsdec.c
+++ b/libavformat/tedcaptionsdec.c
@@ -244,7 +244,7 @@  static int parse_file(AVIOContext *pb, FFDemuxSubtitlesQueue *subs)
             ret = AVERROR_INVALIDDATA;
             goto fail;
         }
-        pkt = ff_subtitles_queue_insert(subs, content.str, content.len, 0);
+        pkt = ff_subtitles_queue_insert_bprint(subs, &content, 0);
         if (!pkt) {
             ret = AVERROR(ENOMEM);
             goto fail;
diff --git a/libavformat/webvttdec.c b/libavformat/webvttdec.c
index 52320ba7d1..0b2fc77168 100644
--- a/libavformat/webvttdec.c
+++ b/libavformat/webvttdec.c
@@ -81,7 +81,9 @@  static int webvtt_read_header(AVFormatContext *s)
         size_t identifier_len, settings_len;
         int64_t ts_start, ts_end;
 
-        ff_subtitles_read_chunk(s->pb, &cue);
+        res = ff_subtitles_read_chunk(s->pb, &cue);
+        if (res < 0)
+            goto end;
 
         if (!cue.len)
             break;